morluto/rea

Fork 0

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

[PR #6778] fix(javascript): keep unisolated program-var route values unknown #7411

New issue

Open

opened 2026-11-01 21:47:40 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:47:40 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6778
Author: @gongfpp
Created: 10/23/2026
Status: 🔄 Open

Base: main ← Head: fix/javascript-index-route-constants-6642

📝 Commits (3)

9cfa2f8 fix(javascript): mark unisolated program-var route values unproven
e2317c5 fix(javascript): keep literal child routes behind an unisolated children array
5e35ded test(javascript): bound the index-route CLI acceptance matrix

📊 Changes

4 files changed (+403 additions, -25 deletions)

View changed files

📝 src/domain/javascript/javascriptRouteInventory.test.ts (+55 -0)
📝 src/domain/javascript/javascriptRouteScan.test.ts (+142 -2)
📝 src/domain/javascript/javascriptRouteScan.ts (+94 -23)
📝 tests/acceptance/javascriptRoutesCli.test.ts (+112 -0)

📄 Description

Summary

Follow-up to #6642. At the maintainer's request this was reworked: the unisolated program-scope var mark now lives in the shared resolver (#resolve), so every consumer degrades from one fact instead of one consumer opting in. Main independently implements the original stable-index feature; the remaining net production change is this runtime-authority guard.

Problem and expected behavior

A program-scope var in a script without a proven isolated runtime is a property of the global object, so globalThis.NAME or an alias such as const host=globalThis can replace it after the source initializer. A source-only proof must not equate that binding with an isolated lexical binding.

Main asserted values from those bindings for every consumer. Observed on the unmodified base:

var INDEX=true;globalThis.INDEX=false;createBrowserRouter([{index:INDEX}]) emitted / with complete enumeration.
var PATH='/a';globalThis.PATH='/b';createBrowserRouter([{path:PATH}]) emitted /a.
var TARGET='/t';globalThis.TARGET='/u';router.push(TARGET) emitted /t.

Expected: no value fact from an unisolated program var. Source parsing stays complete, route interpretation is explicitly incomplete, and independent literal facts (child routes, sibling routes) remain available. Runtime-proven CommonJS/module bindings, lexical bindings and function-local variables keep their existing behavior.

Change and scope

JavaScriptRouteScan.#resolve now returns { node, unproven }. It records the first binding in the chain that is a program-scope var while the existing parser runtime-context owner returns unknown, then continues following the source initializer. No consumer opts in; each one decides what the same fact means:

parent/route segment and navigation destination: unknown interpretation, no candidate;
index flag: unknown, no candidate (unchanged from the previous head);
route container (#walk root, #indexRouteParents): keep walking the source array/object and record an incomplete-interpretation fact on that frame, so var routes=[...] keeps its routes instead of losing them. A children value read from an unisolated var keeps the child records it carries: only the ancestry is unproven, so the literal child paths stay reported next to the interpretation gap instead of being rejected with the parent;
alias selection (#selectedValues) and named-parent resolution: treat as unproven and stop trusting it;
property-value ownership registries: still record the source node, because ownership does not assert runtime identity.

Reuse javaScriptRuntimeContext; Babel's syntactic sourceType is not runtime authority, including CommonJS and type-only TypeScript syntax. This is deliberately conservative rather than a new global-property/variable mutation join. No provider, evaluator, route grammar, target execution or fallback is added.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.

Contract and boundary impact

Semantic owner and earliest changed stage: JavaScriptRouteScan.#resolve in src/domain/javascript/javascriptRouteScan.ts, before inventory and CLI/MCP view projection.
Contribution scope fit and linked maintainer decision: ordinary fail-closed correctness fix within existing static analysis; this follows the maintainer's rework request on this PR ("mark a value from an unisolated program-scope var as unproven for every consumer"). It is not a claim of maintainer approval.
CLI and MCP/tool-catalog contract: arguments and schemas unchanged. Unproven values use the existing route-interpretation unknown; candidate enumeration is partial where a value or container identity is unproven.
Provider, bridge, target-format, or platform compatibility: existing parser runtime-context and binding owners; explicit .cjs/.cts and .mjs/.mts isolation controls, lexical bindings and function-local controls are covered. A bare .js without proven runtime isolation is not assumed to have a CommonJS wrapper.
Evidence, artifact, provenance, or reconstruction contract: independent route facts are retained, source parsing stays complete, and the interpretation gap is projected as an unknown. No captured Evidence is rewritten and no new schema is introduced.
Target selection, authority, upstream behavior: selection and managed worker lifecycle unchanged; analyzed application code is not executed.
Generated metadata, package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

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.

The checked process effect is the existing managed analysis worker, not target execution. The checked file effect is explicit Evidence export and isolated test fixtures, not mutation of the selected application.

Evidence and regression coverage

Tests added or updated: scanner regressions for direct and aliased global writes, false-to-true writes, hoisted var, chained aliases, TypeScript wrappers and type-only exports; value-consumer regressions for a program-var parent segment and navigation destination (string and {pathname} form); container regressions asserting the retained route set plus the incomplete-interpretation fact; controls for lexical bindings, runtime-proven CJS/modules and function-local var; inventory checks for retained child facts plus explicit interpretation gaps; compiled public CLI analysis/export/view coverage with typed summary and unknown projections.

Behavior probe, same fixtures on unmodified main 63b66fa49 versus this head 3721442b0:

fixture	main	`d6cee7159`
`var PATH='/a';globalThis.PATH='/b';...[ {path:PATH} ]`	`/a` (medium)	no candidate, parent-segment unknown
`var TARGET='/t';globalThis.TARGET='/u';router.push(TARGET)`	`/t` (low)	no candidate, unisolated-program-variable unknown
`var NAV={pathname:'/x'};...;router.push(NAV)`	`/x` (low)	no candidate, same unknown
`var INDEX=true;globalThis.INDEX=false;...[ {index:INDEX} ]`	`/`	no candidate, index-flag unknown
`var routes=[{path:'/reports',children:[{path:'child'}]}];createBrowserRouter(routes)`	both routes, no gap	both routes plus incomplete-interpretation unknown
`var children=[{path:'/kept',Component:View}];const config={path:'/parent',children}`	`/parent`, `/kept`	`/parent`, `/kept` plus incomplete-interpretation unknown (the previous head `d6cee7159` dropped both; fixed in `3721442b0`)
`const PATH='/a';globalThis.PATH='/b'`	`/a`	`/a`, no unknown (unchanged)
the same `var` in `routes.mjs`	`/a`	`/a`, no unknown (unchanged)

Validation performed

Node 22.22.2, npm 10.9.7, Windows 11 x64: npx vitest run --project domain src/domain/javascript/javascriptRouteScan.test.ts — 284 passed; the javascript domain subtree — 4798 passed.
Reverting only the production file to main and keeping the new tests: 14 failed, 269 passed (7 index, 2 container, 3 segment, 2 navigation), so the fixtures test this change rather than the harness.
Reverting only #indexRouteParents to the previous head d6cee7159 while keeping the new child-route regression: 1 failed (expected [] to deeply equal [ '/parent', '/kept' ]), so the fixture covers the regression rather than the harness.
Full --project domain: 9447 passed, 0 failed across 433 files on this head. An earlier run of the previous head had one src/contracts/analysisDetail.test.ts 30s timeout under full-suite load; it does not reproduce here and is not claimed as green CI either way.
tsc --noEmit: only the three Playwright type errors that this checkout also has on main; no error in the changed file.
oxfmt --check: clean. oxlint: 25 warnings / 0 errors versus 23 on main — two added complexity advisories on #resolve and #walk.
tests/acceptance/javascriptRoutesCli.test.ts could not run on this host: the compiled CLI reports REA Windows native controls are unavailable: Missing packaged Windows native file native/windows/build/manifest.json. The CI acceptance lane is the first real run of that file for this head.
GitHub Actions for 3721442b0 were still running when this description was written; no workflow result is claimed here.

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.

Compatibility, safety, and release

Breaking changes or migration steps: no argument/schema migration; deliberately conservative behavior for unproven program-var values as described above.
Real Hopper/Ghidra, browser, or OS coverage: not needed for this static parser change.
Package or release metadata impact: none.
Security, privacy, process, or containment review: selected code is parsed, not executed. No network, injection, authority or cleanup relaxation.

Author readiness

Human owner: @gongfpp

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/6778 **Author:** [@gongfpp](https://github.com/gongfpp) **Created:** 10/23/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/javascript-index-route-constants-6642` --- ### 📝 Commits (3) - [`9cfa2f8`](https://github.com/morluto/rea/commit/9cfa2f80fedc9031a91746942e8b8c3dc8a7063e) fix(javascript): mark unisolated program-var route values unproven - [`e2317c5`](https://github.com/morluto/rea/commit/e2317c5f869700c390808dfeeb26b02ba6d594f6) fix(javascript): keep literal child routes behind an unisolated children array - [`5e35ded`](https://github.com/morluto/rea/commit/5e35ded04d8713cf95fd7b3a7a7a19a4ce232a7d) test(javascript): bound the index-route CLI acceptance matrix ### 📊 Changes **4 files changed** (+403 additions, -25 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptRouteInventory.test.ts` (+55 -0) 📝 `src/domain/javascript/javascriptRouteScan.test.ts` (+142 -2) 📝 `src/domain/javascript/javascriptRouteScan.ts` (+94 -23) 📝 `tests/acceptance/javascriptRoutesCli.test.ts` (+112 -0) </details> ### 📄 Description ## Summary Follow-up to #6642. At the maintainer's request this was reworked: the unisolated program-scope `var` mark now lives in the shared resolver (`#resolve`), so every consumer degrades from one fact instead of one consumer opting in. Main independently implements the original stable-index feature; the remaining net production change is this runtime-authority guard. ## Problem and expected behavior A program-scope `var` in a script without a proven isolated runtime is a property of the global object, so `globalThis.NAME` or an alias such as `const host=globalThis` can replace it after the source initializer. A source-only proof must not equate that binding with an isolated lexical binding. Main asserted values from those bindings for every consumer. Observed on the unmodified base: - `var INDEX=true;globalThis.INDEX=false;createBrowserRouter([{index:INDEX}])` emitted `/` with complete enumeration. - `var PATH='/a';globalThis.PATH='/b';createBrowserRouter([{path:PATH}])` emitted `/a`. - `var TARGET='/t';globalThis.TARGET='/u';router.push(TARGET)` emitted `/t`. Expected: no value fact from an unisolated program `var`. Source parsing stays complete, route interpretation is explicitly incomplete, and independent literal facts (child routes, sibling routes) remain available. Runtime-proven CommonJS/module bindings, lexical bindings and function-local variables keep their existing behavior. ## Change and scope `JavaScriptRouteScan.#resolve` now returns `{ node, unproven }`. It records the first binding in the chain that is a program-scope `var` while the existing parser runtime-context owner returns `unknown`, then continues following the source initializer. No consumer opts in; each one decides what the same fact means: - parent/route segment and navigation destination: unknown interpretation, no candidate; - `index` flag: unknown, no candidate (unchanged from the previous head); - route container (`#walk` root, `#indexRouteParents`): keep walking the source array/object and record an incomplete-interpretation fact on that frame, so `var routes=[...]` keeps its routes instead of losing them. A `children` value read from an unisolated `var` keeps the child records it carries: only the ancestry is unproven, so the literal child paths stay reported next to the interpretation gap instead of being rejected with the parent; - alias selection (`#selectedValues`) and named-parent resolution: treat as unproven and stop trusting it; - property-value ownership registries: still record the source node, because ownership does not assert runtime identity. Reuse `javaScriptRuntimeContext`; Babel's syntactic `sourceType` is not runtime authority, including CommonJS and type-only TypeScript syntax. This is deliberately conservative rather than a new global-property/variable mutation join. No provider, evaluator, route grammar, target execution or fallback is added. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the readiness and merge checklists unchecked. Maintainers can help resolve open questions. ## Contract and boundary impact - Semantic owner and earliest changed stage: `JavaScriptRouteScan.#resolve` in `src/domain/javascript/javascriptRouteScan.ts`, before inventory and CLI/MCP view projection. - Contribution scope fit and linked maintainer decision: ordinary fail-closed correctness fix within existing static analysis; this follows the maintainer's rework request on this PR ("mark a value from an unisolated program-scope `var` as unproven for every consumer"). It is not a claim of maintainer approval. - CLI and MCP/tool-catalog contract: arguments and schemas unchanged. Unproven values use the existing route-interpretation unknown; candidate enumeration is partial where a value or container identity is unproven. - Provider, bridge, target-format, or platform compatibility: existing parser runtime-context and binding owners; explicit `.cjs`/`.cts` and `.mjs`/`.mts` isolation controls, lexical bindings and function-local controls are covered. A bare `.js` without proven runtime isolation is not assumed to have a CommonJS wrapper. - Evidence, artifact, provenance, or reconstruction contract: independent route facts are retained, source parsing stays complete, and the interpretation gap is projected as an unknown. No captured Evidence is rewritten and no new schema is introduced. - Target selection, authority, upstream behavior: selection and managed worker lifecycle unchanged; analyzed application code is not executed. - Generated metadata, package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [x] 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. The checked process effect is the existing managed analysis worker, not target execution. The checked file effect is explicit Evidence export and isolated test fixtures, not mutation of the selected application. ## Evidence and regression coverage - Tests added or updated: scanner regressions for direct and aliased global writes, false-to-true writes, hoisted `var`, chained aliases, TypeScript wrappers and type-only exports; value-consumer regressions for a program-`var` parent segment and navigation destination (string and `{pathname}` form); container regressions asserting the retained route set plus the incomplete-interpretation fact; controls for lexical bindings, runtime-proven CJS/modules and function-local `var`; inventory checks for retained child facts plus explicit interpretation gaps; compiled public CLI analysis/export/view coverage with typed summary and unknown projections. Behavior probe, same fixtures on unmodified main `63b66fa49` versus this head `3721442b0`: | fixture | main | d6cee7159 | | --- | --- | --- | | `var PATH='/a';globalThis.PATH='/b';...[ {path:PATH} ]` | `/a` (medium) | no candidate, parent-segment unknown | | `var TARGET='/t';globalThis.TARGET='/u';router.push(TARGET)` | `/t` (low) | no candidate, unisolated-program-variable unknown | | `var NAV={pathname:'/x'};...;router.push(NAV)` | `/x` (low) | no candidate, same unknown | | `var INDEX=true;globalThis.INDEX=false;...[ {index:INDEX} ]` | `/` | no candidate, index-flag unknown | | `var routes=[{path:'/reports',children:[{path:'child'}]}];createBrowserRouter(routes)` | both routes, no gap | both routes **plus** incomplete-interpretation unknown | | `var children=[{path:'/kept',Component:View}];const config={path:'/parent',children}` | `/parent`, `/kept` | `/parent`, `/kept` plus incomplete-interpretation unknown (the previous head `d6cee7159` dropped both; fixed in `3721442b0`) | | `const PATH='/a';globalThis.PATH='/b'` | `/a` | `/a`, no unknown (unchanged) | | the same `var` in `routes.mjs` | `/a` | `/a`, no unknown (unchanged) | ## Validation performed - Node **22.22.2**, npm **10.9.7**, Windows 11 x64: `npx vitest run --project domain src/domain/javascript/javascriptRouteScan.test.ts` — **284 passed**; the javascript domain subtree — **4798 passed**. - Reverting only the production file to main and keeping the new tests: **14 failed, 269 passed** (7 index, 2 container, 3 segment, 2 navigation), so the fixtures test this change rather than the harness. - Reverting only `#indexRouteParents` to the previous head `d6cee7159` while keeping the new child-route regression: **1 failed** (`expected [] to deeply equal [ '/parent', '/kept' ]`), so the fixture covers the regression rather than the harness. - Full `--project domain`: **9447 passed, 0 failed** across 433 files on this head. An earlier run of the previous head had one `src/contracts/analysisDetail.test.ts` 30s timeout under full-suite load; it does not reproduce here and is not claimed as green CI either way. - `tsc --noEmit`: only the three Playwright type errors that this checkout also has on main; no error in the changed file. - `oxfmt --check`: clean. `oxlint`: **25 warnings / 0 errors** versus 23 on main — two added `complexity` advisories on `#resolve` and `#walk`. - `tests/acceptance/javascriptRoutesCli.test.ts` could not run on this host: the compiled CLI reports `REA Windows native controls are unavailable: Missing packaged Windows native file native/windows/build/manifest.json`. The CI acceptance lane is the first real run of that file for this head. - GitHub Actions for `3721442b0` were still running when this description was written; no workflow result is claimed here. 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. ## Compatibility, safety, and release - Breaking changes or migration steps: no argument/schema migration; deliberately conservative behavior for unproven program-var values as described above. - Real Hopper/Ghidra, browser, or OS coverage: not needed for this static parser change. - Package or release metadata impact: none. - Security, privacy, process, or containment review: selected code is parsed, not executed. No network, injection, authority or cleanup relaxation. ## Author readiness Human owner: @gongfpp - [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 - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-11-01 21:47:40 +00:00

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

No description provided.

Rows
Columns