morluto/rea

Fork 0

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

[PR #4173] [CLOSED] fix(javascript): prune unreachable constant return branches #6109

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4173
Author: @JNTMTMTM
Created: 10/15/2026
Status: ❌ Closed

Base: main ← Head: codex/fix-4168-known-branches

📝 Commits (10+)

1053456 fix(javascript): prune unreachable constant return branches
4301f08 test(javascript): use unknown choices for candidate budget fixtures
ec2800f test(javascript): import runtime fixtures through file URLs
b27e974 chore: merge main and resolve fixture import conflict
534bbf9 fix(javascript): respect source reads when selecting branches
74ed00c fix(javascript): evaluate branch operands at their source reads
ab75f2d fix(javascript): preserve branch operand capture timing
60eb2c6 fix(javascript): unify branch reads with main snapshot ownership
3d65610 fix(javascript): share capture sensitivity across state queries
8aa0052 fix(javascript): order nested reads and bound dependency checks

📊 Changes

14 files changed (+1744 additions, -271 deletions)

View changed files

📝 src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts (+22 -5)
📝 src/domain/javascript/javascriptSemanticAnalysis.reference_lifetimes.test.ts (+93 -0)
📝 src/domain/javascript/javascriptSemanticAnalysis.timers.test.ts (+15 -0)
📝 src/domain/javascript/javascriptSemanticAnalysis.unions.test.ts (+454 -23)
📝 src/domain/javascript/javascriptSemanticAsyncEffects.ts (+3 -5)
📝 src/domain/javascript/javascriptSemanticDependencies.ts (+13 -0)
📝 src/domain/javascript/javascriptSemanticMemberMutations.ts (+1 -1)
📝 src/domain/javascript/javascriptSemanticMutationInitializers.test.ts (+1 -1)
📝 src/domain/javascript/javascriptSemanticMutationInitializers.ts (+104 -45)
📝 src/domain/javascript/javascriptSemanticValues.memo.test.ts (+369 -131)
📝 src/domain/javascript/javascriptSemanticValues.ts (+163 -35)
📝 tests/acceptance/applications/applicationWorkflowCli.test.ts (+475 -9)
📝 tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts (+23 -9)
📝 tests/fixtures/javascriptPrimitiveExpansion.ts (+8 -7)

📄 Description

Summary

Fixes #4168. Configuration values selected by known primitive conditionals and logical operators now compare equal to their inlined return shapes through CLI and stdio MCP: zero added, removed, changed, or unknown fields.

Problem and expected behavior

Unreachable operands previously contributed candidate values. Branch folding must also respect initialization order and capture timing: future lexical reads remain uncertain, while initialized exports, saved primitive snapshots, and valid deferred captures retain exact values.

Change and scope

Fold reachable operands of ?:, ||, &&, and ?? using the shared value evaluator.
Extend main's semanticBindingAtRead owner to distinguish ordered, deferred, and unresolved reads across blocks, loops, function/field activations, exported declarations, same-case switch sequences, and class static blocks. Preserve declarator order inside exports and for headers. Timer cancellation consumes the same deferred-capture proof.
Reuse query-owned ReadSnapshotValues, including initializer identity, source-read mode, capture point, exact expression depth, dependency proofs, depth dependence, and whole-binding cache guards. Superseded source-read caches and ordering implementations are removed.
Retain one capture-sensitive dependency proof per state. Known appended member/escape effects extend it incrementally while invalidating values; unqualified invalidation rebuilds it. Snapshot checks intersect proofs without repeatedly scanning every effectful binding.
Integrate main at 3ad1032b1, including upstream cache-publication fix #4824. Its validated pre-build plan and per-task availability manifest tolerate evicted cache entries; consumers rebuild misses. Required build outputs, tests, malformed-input checks, and non-file/I/O failure checks remain enforced.

Latest addressed findings: switch-case constants, exported constants, index rebuilding during writes, and future loop-local values.

Contract and boundary impact

Ownership: domain initializer ordering, value snapshots, and effect-derived sensitivity; existing shared CLI/MCP workflows consume those facts.
Inputs, public schemas, source identities, provenance, uncertainty, and resource-limit reasons are preserved.
Routine static-analysis correctness fix. Product analysis reads source files; owned test fixtures independently execute in Node. No new target-execution authority or installer/dependency changes.

Expected product effects:

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

Owned Node fixtures and CLI/MCP comparisons cover known branches, saved captures before later mutation, exported constants/declarators, future declarations, all five loop forms, initialized loop headers, valid deferred captures, same-case reads, and skipped-case TDZ uncertainty.
Genuine primitive/getter exhaustion retains deduplicated limitation reasons and partial coverage; known-selector controls do not invent candidate exhaustion.
Exported-read and incremental-index regressions failed on db15da996; switch/static-block regressions failed on 6ec73164d before repair. The 16/32-object write fixture visited 608/2,240 binding-index entries before repair; the final regression rejects quadratic growth and checks retained values.
Cache eviction was reproduced with the installed Turbo's 1 MB limit: the build succeeded, a later dry run evicted its archive, and the old publisher failed with ENOENT while the build output remained valid. Upstream's adopted tests cover disappearance during copying and real workflow restore/rebuild behavior.
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

macOS, Node 24.18.0/npm 11.16.0:

npm run test:fast — 606 files; 9,723 passed, 13 skipped.
npm run test:focused -- tests/acceptance/applications/applicationWorkflowCli.test.ts tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts tests/boundary/filesystem/javascriptExportShapeComparison.test.ts tests/boundary/mcp/applicationExportShapeMcp.test.ts src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts tests/acceptance/applications/javascriptCapturedParameters.test.ts — fresh build; 6 files / 185 passed.
Before the switch-case follow-up, the broader focused set at 6ec73164d also passed: 9 files / 225 tests, including portable cache, CI lanes, and Electron launch rejection boundaries.
At 6ec73164d, executed the unchanged CI producer sequence: validated dry-plan key, npm run test:prepare, then snapshot from .cache/ci-build-plan.json — all six task caches staged successfully.
npm run check and git diff --check — passed; lint reports warnings. npm run docs:check also passed at 6ec73164d; docs were unchanged by the switch-case follow-up.
At 6ec73164d, npm run verify:test-discovery — all 1,545 files discovered exactly once; no files added since.
Current-head CI, dd6adcf8f — passed: build package, source tests, static checks, docs, all five portable shards, real JavaScript recovery, native package checks, Portable tests, and CI required. Optional lanes were skipped according to workflow applicability.

Cost verification for incremental sensitivity

Compared db15da996 and 7c929c82a with complete module analysis of 512 independent branch-containing objects followed by member writes. Returned binding values and coverage had identical digests. Three paired runs alternated order; each fresh Node/tsx process performed six complete analyses with fresh IR states. Medians:

Measurement	Before	After
First analysis, ms	118.3	87.6
Warm mean of five analyses, ms	84.0	52.7
Process elapsed, ms	686.5	505.8
Node CPU, ms	958.8	753.1
Estimated process-family peak RSS, MiB	288.1	281.1

A separate public analyze-javascript-application --json workload analyzed 128 such objects. Two alternating pairs compared compiled db15da996 with 6ec73164d. Retained graph facts and coverage matched: 1,153 semantic nodes, 896 relations, and 1,383,250 reply bytes.

CLI measurement	Before	After
Process elapsed, ms	1,445.0	1,237.1
CPU from time user + system, ms	1,610	1,605
Estimated process-family peak RSS, MiB	520.3	520.7

Source first-analysis timing excludes imports; process elapsed includes imports and sampling. CLI runs use fresh processes, including their worker. OS/tsx caches were not cleared. Family RSS is a lower-bound estimate from 25 ms samples and the reported process peak; transient peaks may be missed. Source CPU covers Node; CLI CPU uses the host time utility. Profiler-helper CPU and agent/transport tokens were not measured. These synthetic workloads and small samples do not establish a general application speedup.

Compatibility, safety, and release

Public inputs and schemas require no migration. Full installed-package/coverage gates, Windows/other OS runtimes, optional real providers, and live-agent token measurements were not run locally. Real Ghidra/JVM behavior is outside this static fix's verification scope. Initial human readiness declarations below are preserved.

Author readiness

Human owner: @JNTMTMTM

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/4173 **Author:** [@JNTMTMTM](https://github.com/JNTMTMTM) **Created:** 10/15/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `codex/fix-4168-known-branches` --- ### 📝 Commits (10+) - [`1053456`](https://github.com/morluto/rea/commit/1053456ff0372e642672fa34dac2979f0b9da0d1) fix(javascript): prune unreachable constant return branches - [`4301f08`](https://github.com/morluto/rea/commit/4301f08eed2a985060092fade42a3e1cfc7675d0) test(javascript): use unknown choices for candidate budget fixtures - [`ec2800f`](https://github.com/morluto/rea/commit/ec2800f1d4a870bdea400ea768438679eb973dc3) test(javascript): import runtime fixtures through file URLs - [`b27e974`](https://github.com/morluto/rea/commit/b27e97437befe900bf4479443124ff5d9f125b84) chore: merge main and resolve fixture import conflict - [`534bbf9`](https://github.com/morluto/rea/commit/534bbf9d96ff7302332cb9f58df1e146136911ce) fix(javascript): respect source reads when selecting branches - [`74ed00c`](https://github.com/morluto/rea/commit/74ed00c0d946647e6d4c948862242ef0b0683ce5) fix(javascript): evaluate branch operands at their source reads - [`ab75f2d`](https://github.com/morluto/rea/commit/ab75f2dbae0e3f6c705d13863b4fdf87e1cd26c7) fix(javascript): preserve branch operand capture timing - [`60eb2c6`](https://github.com/morluto/rea/commit/60eb2c61fa31988ee84bbd04d825f6355feb858a) fix(javascript): unify branch reads with main snapshot ownership - [`3d65610`](https://github.com/morluto/rea/commit/3d65610afd448222ac7f4be7c7065ff168d06a96) fix(javascript): share capture sensitivity across state queries - [`8aa0052`](https://github.com/morluto/rea/commit/8aa00522825e2c4c801934650cd5f4b1389382f0) fix(javascript): order nested reads and bound dependency checks ### 📊 Changes **14 files changed** (+1744 additions, -271 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts` (+22 -5) 📝 `src/domain/javascript/javascriptSemanticAnalysis.reference_lifetimes.test.ts` (+93 -0) 📝 `src/domain/javascript/javascriptSemanticAnalysis.timers.test.ts` (+15 -0) 📝 `src/domain/javascript/javascriptSemanticAnalysis.unions.test.ts` (+454 -23) 📝 `src/domain/javascript/javascriptSemanticAsyncEffects.ts` (+3 -5) 📝 `src/domain/javascript/javascriptSemanticDependencies.ts` (+13 -0) 📝 `src/domain/javascript/javascriptSemanticMemberMutations.ts` (+1 -1) 📝 `src/domain/javascript/javascriptSemanticMutationInitializers.test.ts` (+1 -1) 📝 `src/domain/javascript/javascriptSemanticMutationInitializers.ts` (+104 -45) 📝 `src/domain/javascript/javascriptSemanticValues.memo.test.ts` (+369 -131) 📝 `src/domain/javascript/javascriptSemanticValues.ts` (+163 -35) 📝 `tests/acceptance/applications/applicationWorkflowCli.test.ts` (+475 -9) 📝 `tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts` (+23 -9) 📝 `tests/fixtures/javascriptPrimitiveExpansion.ts` (+8 -7) </details> ### 📄 Description ## Summary Fixes #4168. Configuration values selected by known primitive conditionals and logical operators now compare equal to their inlined return shapes through CLI and stdio MCP: zero added, removed, changed, or unknown fields. ## Problem and expected behavior Unreachable operands previously contributed candidate values. Branch folding must also respect initialization order and capture timing: future lexical reads remain uncertain, while initialized exports, saved primitive snapshots, and valid deferred captures retain exact values. ## Change and scope - Fold reachable operands of `?:`, `||`, `&&`, and `??` using the shared value evaluator. - Extend main's `semanticBindingAtRead` owner to distinguish ordered, deferred, and unresolved reads across blocks, loops, function/field activations, exported declarations, same-case switch sequences, and class static blocks. Preserve declarator order inside exports and `for` headers. Timer cancellation consumes the same deferred-capture proof. - Reuse query-owned `ReadSnapshotValues`, including initializer identity, source-read mode, capture point, exact expression depth, dependency proofs, depth dependence, and whole-binding cache guards. Superseded source-read caches and ordering implementations are removed. - Retain one capture-sensitive dependency proof per state. Known appended member/escape effects extend it incrementally while invalidating values; unqualified invalidation rebuilds it. Snapshot checks intersect proofs without repeatedly scanning every effectful binding. - Integrate main at `3ad1032b1`, including upstream cache-publication fix #4824. Its validated pre-build plan and per-task availability manifest tolerate evicted cache entries; consumers rebuild misses. Required build outputs, tests, malformed-input checks, and non-file/I/O failure checks remain enforced. Latest addressed findings: [switch-case constants](https://github.com/morluto/rea/pull/4173#discussion_r4303804750), [exported constants](https://github.com/morluto/rea/pull/4173#discussion_r4303093448), [index rebuilding during writes](https://github.com/morluto/rea/pull/4173#discussion_r4303093456), and [future loop-local values](https://github.com/morluto/rea/pull/4173#discussion_r4301504530). ## Contract and boundary impact - Ownership: domain initializer ordering, value snapshots, and effect-derived sensitivity; existing shared CLI/MCP workflows consume those facts. - Inputs, public schemas, source identities, provenance, uncertainty, and resource-limit reasons are preserved. - Routine static-analysis correctness fix. Product analysis reads source files; owned test fixtures independently execute in Node. No new target-execution authority or installer/dependency changes. Expected product effects: - [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 - Owned Node fixtures and CLI/MCP comparisons cover known branches, saved captures before later mutation, exported constants/declarators, future declarations, all five loop forms, initialized loop headers, valid deferred captures, same-case reads, and skipped-case TDZ uncertainty. - Genuine primitive/getter exhaustion retains deduplicated limitation reasons and partial coverage; known-selector controls do not invent candidate exhaustion. - Exported-read and incremental-index regressions failed on `db15da996`; switch/static-block regressions failed on `6ec73164d` before repair. The 16/32-object write fixture visited 608/2,240 binding-index entries before repair; the final regression rejects quadratic growth and checks retained values. - Cache eviction was reproduced with the installed Turbo's 1 MB limit: the build succeeded, a later dry run evicted its archive, and the old publisher failed with ENOENT while the build output remained valid. Upstream's adopted tests cover disappearance during copying and real workflow restore/rebuild behavior. - [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 macOS, Node 24.18.0/npm 11.16.0: - `npm run test:fast` — 606 files; 9,723 passed, 13 skipped. - `npm run test:focused -- tests/acceptance/applications/applicationWorkflowCli.test.ts tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts tests/boundary/filesystem/javascriptExportShapeComparison.test.ts tests/boundary/mcp/applicationExportShapeMcp.test.ts src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts tests/acceptance/applications/javascriptCapturedParameters.test.ts` — fresh build; 6 files / 185 passed. - Before the switch-case follow-up, the broader focused set at `6ec73164d` also passed: 9 files / 225 tests, including portable cache, CI lanes, and Electron launch rejection boundaries. - At `6ec73164d`, executed the unchanged CI producer sequence: validated dry-plan key, `npm run test:prepare`, then snapshot from `.cache/ci-build-plan.json` — all six task caches staged successfully. - `npm run check` and `git diff --check` — passed; lint reports warnings. `npm run docs:check` also passed at `6ec73164d`; docs were unchanged by the switch-case follow-up. - At `6ec73164d`, `npm run verify:test-discovery` — all 1,545 files discovered exactly once; no files added since. - [Current-head CI](https://github.com/morluto/rea/actions/runs/39056148202), `dd6adcf8f` — passed: build package, source tests, static checks, docs, all five portable shards, real JavaScript recovery, native package checks, `Portable tests`, and `CI required`. Optional lanes were skipped according to workflow applicability. <details> <summary>Cost verification for incremental sensitivity</summary> Compared `db15da996` and `7c929c82a` with complete module analysis of 512 independent branch-containing objects followed by member writes. Returned binding values and coverage had identical digests. Three paired runs alternated order; each fresh Node/tsx process performed six complete analyses with fresh IR states. Medians: | Measurement | Before | After | | --- | ---: | ---: | | First analysis, ms | 118.3 | 87.6 | | Warm mean of five analyses, ms | 84.0 | 52.7 | | Process elapsed, ms | 686.5 | 505.8 | | Node CPU, ms | 958.8 | 753.1 | | Estimated process-family peak RSS, MiB | 288.1 | 281.1 | A separate public `analyze-javascript-application --json` workload analyzed 128 such objects. Two alternating pairs compared compiled `db15da996` with `6ec73164d`. Retained graph facts and coverage matched: 1,153 semantic nodes, 896 relations, and 1,383,250 reply bytes. | CLI measurement | Before | After | | --- | ---: | ---: | | Process elapsed, ms | 1,445.0 | 1,237.1 | | CPU from time user + system, ms | 1,610 | 1,605 | | Estimated process-family peak RSS, MiB | 520.3 | 520.7 | Source first-analysis timing excludes imports; process elapsed includes imports and sampling. CLI runs use fresh processes, including their worker. OS/tsx caches were not cleared. Family RSS is a lower-bound estimate from 25 ms samples and the reported process peak; transient peaks may be missed. Source CPU covers Node; CLI CPU uses the host time utility. Profiler-helper CPU and agent/transport tokens were not measured. These synthetic workloads and small samples do not establish a general application speedup. </details> ## Compatibility, safety, and release Public inputs and schemas require no migration. Full installed-package/coverage gates, Windows/other OS runtimes, optional real providers, and live-agent token measurements were not run locally. Real Ghidra/JVM behavior is outside this static fix's verification scope. Initial human readiness declarations below are preserved. ## Author readiness Human owner: @JNTMTMTM - [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

2026-10-31 01:11: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#6109

No description provided.

Rows
Columns