morluto/rea

Fork 0

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

[PR #4510] [CLOSED] fix(javascript): preserve raw ESM path classification #6277

New issue

Closed

opened 2026-10-31 01:13:14 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:13:14 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4510
Author: @dakjdakd
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/esm-specifier-classification

📝 Commits (1)

6bbe3a5 fix(javascript): preserve raw ESM path classification

📊 Changes

2 files changed (+72 additions, -5 deletions)

View changed files

📝 src/application/javascript/JavaScriptArtifactPathResolution.ts (+4 -5)
📝 tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts (+68 -0)

📄 Description

Summary

Fixes #4509.

Preserve the original ESM specifier's relative/absolute path classification before applying URL text preprocessing. A request such as " ./dep.mjs" now keeps an unresolved relationship instead of acquiring a false dependency on the local dep.mjs file. Already-relative requests with trailing spaces or internal tabs/newlines continue to select the files Node loads.

The change is in local commit 6bbe3a5b6f5fee7fcdfa953f6dde4dcc6a70c3d2, based directly on 180ba03bfddd5916251e81e97a500b1ede7a9eea. It changes one production resolver and adds one real graph/Node regression to an existing boundary test file.

Problem and expected behavior

With an existing dep.mjs, REA previously treated both of these requests as resolved local imports:

import value from "./dep.mjs";
import other from " ./dep.mjs";

Node accepts the first and rejects the second with ERR_MODULE_NOT_FOUND. A leading space changes the classification input: the raw request does not start with relative module syntax. Normalizing it first silently changes the request being resolved.

This affects the graph's static-import edge and its semantic import relationship. Both use the shared resolver, retain the original specifier, and previously attached it to the local module. Preserving the source text in edge properties did not prevent an incorrect resolved target from being published.

The invariant is that URL preprocessing may normalize the lookup text of an admitted ESM module path, but it cannot decide whether the original request was a module path. The distinction also explains why the positive controls must remain valid: "./dep.mjs ", "./de\np.mjs", and "./de\tp.mjs" already have relative syntax before normalization.

Change and scope

contextualCandidate now computes one classification fact before changing declared:

const isModulePath =
  isRelativeJavaScriptModuleSpecifier(declared) || declared.startsWith("/");

The existing bare/local branch consumes that fact after the existing URL text preprocessing and declaration validation:

if (!isModulePath) return bareModuleCandidate(input);

The resolver still normalizes ESM URL text once. Validation continues to inspect the appropriate lookup text before suffix handling and the existing percent-decoding pass. This is why the fix retains working trailing-space/control handling instead of replacing it with an early raw-string rejection.

The file URL, scheme, exact package-import alias, package-target, and CommonJS branches remain in their existing positions. In particular, this change does not move bare classification ahead of absolute URL/scheme handling or attempt to broaden artifact mapping for absolute file URLs. The original declaration is still retained as evidence.

Both import projections already reach this owner through resolveArtifactPathByContext. The surrounding graph, Electron boundary, and runtime/package resolution consumers were traced; the correction belongs at their shared classification boundary, so there are no consumer-specific guards or duplicate normalizers.

This is an ordinary static-analysis correctness fix within existing boundaries. A new maintainer scope decision for execution, mutation, instrumentation, or protection recovery is not applicable.

Background

#3214 and #3254 fixed a different problem: valid relative ESM specifiers containing internal tabs/newlines were not resolved. The original PR's patch normalized inside the already-relative branch, and its description explicitly preserved raw classification for leading spaces.

The main integration in 7e5cffa applied normalization before classification. This PR keeps the original positive behavior while repairing the integration's false-positive boundary. It does not revert URL preprocessing or repeat the earlier internal-whitespace fix.

Contract and boundary impact

Semantic owner and earliest changed stage: contextualCandidate, during module-path resolution in JavaScript application graph construction.
Contribution scope: ordinary defect correction over selected local artifacts; no new provider or target authority.
CLI and MCP/tool-catalog contract: no command, tool, argument, schema, or catalog changes. The shared graph's existing resolution properties become accurate for the affected inputs.
Provider, bridge, format, and platform compatibility: no provider protocol or supported-format changes; the production edit is string classification in the shared resolver. Runtime verification was on Windows x64.
Evidence and provenance: negative cases retain their raw specifiers and unresolved observations, but no longer claim a resolved local file. Positive cases retain their resolved paths. Artifact identity and source locations are unchanged.
Target selection and cleanup: existing read-only artifact selection and resource ownership remain unchanged. The test's Node subprocess runs only the authored inert fixture used as a loader oracle.
Generated metadata, installation, and release: no catalog, packaged skill, dependency, install configuration, or release metadata changes.

Expected production 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

The new test, classifies raw specifiers before normalizing relative module URL paths, uses actual temporary files and the existing relationshipGraph helper. It passes those files through canonical inventory, artifact acquisition, AST analysis, and validated graph construction. A separate process running the same Node executable independently imports each specifier.

The eight controls cover:

Specifier	Native loader result	Graph assertion after this fix
`" ./dep.mjs"`	`ERR_MODULE_NOT_FOUND`	Null local path; unresolved status
`"\n./dep.mjs"`	`ERR_MODULE_NOT_FOUND`	Null local path; unresolved status
`"\t./dep.mjs"`	`ERR_MODULE_NOT_FOUND`	Null local path; unresolved status
`".\n/dep.mjs"`	`ERR_INVALID_MODULE_SPECIFIER`	Null local path; unresolved status
`"./dep.mjs"`	Loads authored module	`dep.mjs`, resolved
`"./dep.mjs "`	Loads authored module	`dep.mjs`, resolved
`"./de\np.mjs"`	Loads authored module	`dep.mjs`, resolved
`"./de\tp.mjs"`	Loads authored module	`dep.mjs`, resolved

The test requires exactly two import edges for each original specifier and explicitly checks both the static-import and semantic import projections. It therefore catches a correction applied to only one consumer as well as accidental loss of the original declaration.

The regression was executed against the affected production source before the edit. The native oracle matched expectations, while the graph returned dep.mjs where the negative case required null. The same regression passes with this change. The observations are executed boundary evidence; potential downstream tracing effects remain inferred.

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

Validation performed

Node v24.18.0, Windows x64, locked repository dependencies:

npm.cmd run test:focused -- src/application/javascript/JavaScriptArtifactPathResolution.test.ts src/application/javascript/JavaScriptHtmlBaseResolution.test.ts src/domain/javascript/javascriptPackageImportAliases.test.ts src/domain/artifactPathSyntax.test.ts

4 files, 231 tests passed. This includes existing CommonJS, percent-byte, package resolution, file URL, HTML base, and path syntax controls.

node node_modules/vitest/vitest.mjs run --project boundary tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts -t "classifies raw specifiers|deduplicates"

3 tests passed; 8 unselected tests skipped. The new regression is one of the three executed tests.

npm.cmd run check:fast — typecheck and lint passed, 2/2 tasks.
npm.cmd run verify:test-discovery — all 1418 test files discovered exactly once.
Changed-file oxfmt --check and git diff --check — passed.

Compatibility, safety, and release

No migration or schema change is required. The visible correction is that invalid local-path interpretations become unresolved edges; consumers still receive the original import observation and its resolution reason.

Verification covers the source production-graph pipeline and a separate Node loader process on Windows x64. The results above describe those selected tests and static checks; full CLI/MCP worker, macOS/Linux, external-provider, and complete-suite coverage remain for their respective CI and review lanes.

Author readiness

Human owner: @dakjdakd

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR and all contributed code, tests, and documentation.
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 one concrete graph-correctness defect.
The correction stays within existing static-analysis scope and authority.
The title follows type(scope): outcome.
The issue placeholder above has been replaced with the filed issue number.
Regression coverage checks changed outcomes and valid whitespace controls.
No owning schema, generated metadata, or installation change is required.
Representative graph properties and verification limits are included above.
The human owner has completed final review for unrelated changes, secrets, 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/4510 **Author:** [@dakjdakd](https://github.com/dakjdakd) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/esm-specifier-classification` --- ### 📝 Commits (1) - [`6bbe3a5`](https://github.com/morluto/rea/commit/6bbe3a5b6f5fee7fcdfa953f6dde4dcc6a70c3d2) fix(javascript): preserve raw ESM path classification ### 📊 Changes **2 files changed** (+72 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptArtifactPathResolution.ts` (+4 -5) 📝 `tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts` (+68 -0) </details> ### 📄 Description ## Summary Fixes #4509. Preserve the original ESM specifier's relative/absolute path classification before applying URL text preprocessing. A request such as `" ./dep.mjs"` now keeps an unresolved relationship instead of acquiring a false dependency on the local `dep.mjs` file. Already-relative requests with trailing spaces or internal tabs/newlines continue to select the files Node loads. The change is in local commit `6bbe3a5b6f5fee7fcdfa953f6dde4dcc6a70c3d2`, based directly on `180ba03bfddd5916251e81e97a500b1ede7a9eea`. It changes one production resolver and adds one real graph/Node regression to an existing boundary test file. ## Problem and expected behavior With an existing `dep.mjs`, REA previously treated both of these requests as resolved local imports: ```js import value from "./dep.mjs"; import other from " ./dep.mjs"; ``` Node accepts the first and rejects the second with `ERR_MODULE_NOT_FOUND`. A leading space changes the classification input: the raw request does not start with relative module syntax. Normalizing it first silently changes the request being resolved. This affects the graph's static-import edge and its semantic import relationship. Both use the shared resolver, retain the original specifier, and previously attached it to the local module. Preserving the source text in edge properties did not prevent an incorrect resolved target from being published. The invariant is that URL preprocessing may normalize the lookup text of an admitted ESM module path, but it cannot decide whether the original request was a module path. The distinction also explains why the positive controls must remain valid: `"./dep.mjs "`, `"./de\np.mjs"`, and `"./de\tp.mjs"` already have relative syntax before normalization. ## Change and scope `contextualCandidate` now computes one classification fact before changing `declared`: ```ts const isModulePath = isRelativeJavaScriptModuleSpecifier(declared) || declared.startsWith("/"); ``` The existing bare/local branch consumes that fact after the existing URL text preprocessing and declaration validation: ```ts if (!isModulePath) return bareModuleCandidate(input); ``` The resolver still normalizes ESM URL text once. Validation continues to inspect the appropriate lookup text before suffix handling and the existing percent-decoding pass. This is why the fix retains working trailing-space/control handling instead of replacing it with an early raw-string rejection. The file URL, scheme, exact package-import alias, package-target, and CommonJS branches remain in their existing positions. In particular, this change does not move bare classification ahead of absolute URL/scheme handling or attempt to broaden artifact mapping for absolute file URLs. The original declaration is still retained as evidence. Both import projections already reach this owner through `resolveArtifactPathByContext`. The surrounding graph, Electron boundary, and runtime/package resolution consumers were traced; the correction belongs at their shared classification boundary, so there are no consumer-specific guards or duplicate normalizers. This is an ordinary static-analysis correctness fix within existing boundaries. A new maintainer scope decision for execution, mutation, instrumentation, or protection recovery is not applicable. ## Background [#3214](https://github.com/morluto/rea/issues/3214) and [#3254](https://github.com/morluto/rea/pull/3254) fixed a different problem: valid relative ESM specifiers containing internal tabs/newlines were not resolved. The original PR's patch normalized inside the already-relative branch, and its description explicitly preserved raw classification for leading spaces. The main integration in [7e5cffa](https://github.com/morluto/rea/commit/7e5cffa2ae9dfc9afaeca0715110d4dd41c0dcdd) applied normalization before classification. This PR keeps the original positive behavior while repairing the integration's false-positive boundary. It does not revert URL preprocessing or repeat the earlier internal-whitespace fix. ## Contract and boundary impact - **Semantic owner and earliest changed stage:** `contextualCandidate`, during module-path resolution in JavaScript application graph construction. - **Contribution scope:** ordinary defect correction over selected local artifacts; no new provider or target authority. - **CLI and MCP/tool-catalog contract:** no command, tool, argument, schema, or catalog changes. The shared graph's existing resolution properties become accurate for the affected inputs. - **Provider, bridge, format, and platform compatibility:** no provider protocol or supported-format changes; the production edit is string classification in the shared resolver. Runtime verification was on Windows x64. - **Evidence and provenance:** negative cases retain their raw specifiers and unresolved observations, but no longer claim a resolved local file. Positive cases retain their resolved paths. Artifact identity and source locations are unchanged. - **Target selection and cleanup:** existing read-only artifact selection and resource ownership remain unchanged. The test's Node subprocess runs only the authored inert fixture used as a loader oracle. - **Generated metadata, installation, and release:** no catalog, packaged skill, dependency, install configuration, or release metadata changes. Expected production 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 The new test, `classifies raw specifiers before normalizing relative module URL paths`, uses actual temporary files and the existing `relationshipGraph` helper. It passes those files through canonical inventory, artifact acquisition, AST analysis, and validated graph construction. A separate process running the same Node executable independently imports each specifier. The eight controls cover: | Specifier | Native loader result | Graph assertion after this fix | | --- | --- | --- | | `" ./dep.mjs"` | `ERR_MODULE_NOT_FOUND` | Null local path; unresolved status | | `"\n./dep.mjs"` | `ERR_MODULE_NOT_FOUND` | Null local path; unresolved status | | `"\t./dep.mjs"` | `ERR_MODULE_NOT_FOUND` | Null local path; unresolved status | | `".\n/dep.mjs"` | `ERR_INVALID_MODULE_SPECIFIER` | Null local path; unresolved status | | `"./dep.mjs"` | Loads authored module | `dep.mjs`, resolved | | `"./dep.mjs "` | Loads authored module | `dep.mjs`, resolved | | `"./de\np.mjs"` | Loads authored module | `dep.mjs`, resolved | | `"./de\tp.mjs"` | Loads authored module | `dep.mjs`, resolved | The test requires exactly two import edges for each original specifier and explicitly checks both the static-import and semantic import projections. It therefore catches a correction applied to only one consumer as well as accidental loss of the original declaration. The regression was executed against the affected production source before the edit. The native oracle matched expectations, while the graph returned `dep.mjs` where the negative case required null. The same regression passes with this change. The observations are executed boundary evidence; potential downstream tracing effects remain inferred. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and original specifiers remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed Node `v24.18.0`, Windows x64, locked repository dependencies: ```text npm.cmd run test:focused -- src/application/javascript/JavaScriptArtifactPathResolution.test.ts src/application/javascript/JavaScriptHtmlBaseResolution.test.ts src/domain/javascript/javascriptPackageImportAliases.test.ts src/domain/artifactPathSyntax.test.ts ``` **4 files, 231 tests passed.** This includes existing CommonJS, percent-byte, package resolution, file URL, HTML base, and path syntax controls. ```text node node_modules/vitest/vitest.mjs run --project boundary tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts -t "classifies raw specifiers|deduplicates" ``` **3 tests passed; 8 unselected tests skipped.** The new regression is one of the three executed tests. - `npm.cmd run check:fast` — typecheck and lint passed, **2/2 tasks**. - `npm.cmd run verify:test-discovery` — **all 1418 test files discovered exactly once**. - Changed-file `oxfmt --check` and `git diff --check` — passed. ## Compatibility, safety, and release No migration or schema change is required. The visible correction is that invalid local-path interpretations become unresolved edges; consumers still receive the original import observation and its resolution reason. Verification covers the source production-graph pipeline and a separate Node loader process on Windows x64. The results above describe those selected tests and static checks; full CLI/MCP worker, macOS/Linux, external-provider, and complete-suite coverage remain for their respective CI and review lanes. ## Author readiness Human owner: @dakjdakd - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR and all contributed code, tests, and documentation. - [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 one concrete graph-correctness defect. - [x] The correction stays within existing static-analysis scope and authority. - [x] The title follows `type(scope): outcome`. - [ ] The issue placeholder above has been replaced with the filed issue number. - [x] Regression coverage checks changed outcomes and valid whitespace controls. - [x] No owning schema, generated metadata, or installation change is required. - [x] Representative graph properties and verification limits are included above. - [ ] The human owner has completed final review for unrelated changes, secrets, 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:13:14 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:46:30 +00:00

[PR #6277] [MERGED] fix(windows): report missing native launch controls as unavailable capabilities #7250

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

No description provided.

Rows
Columns