[PR #2702] [CLOSED] fix(javascript): preserve Node package lookup and artifact path identity #5327

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

📋 Pull Request Information

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

Base: main ← Head: fix/node-package-main-resolution


📝 Commits (10+)

  • 4d4e30d fix(javascript): match Node package main resolution
  • f6802f5 Merge upstream main for current package main verification
  • a0424a2 chore: merge upstream CI and fixture fixes
  • 87bdcf8 chore: merge upstream parameter provenance test fix
  • 443b404 test(apple): bound cold archive fixture compilation
  • 79ba2a8 chore: merge upstream CI and provenance test fixes
  • a765cae fix(javascript): resolve package main indexes at artifact root
  • e439438 test(release): scope preflight schema to selected jobs
  • 97ec5a5 test(ci): include cold-cache Apple fixture timeout correction
  • cd0afee test(ci): merge shared release and Apple fixture fixes

📊 Changes

15 files changed (+422 additions, -101 deletions)

View changed files

📝 src/application/javascript/ElectronBoundaryAnalysis.ts (+18 -7)
📝 src/application/javascript/ElectronBoundaryGraphNative.ts (+2 -1)
📝 src/application/javascript/ElectronBoundaryGraphWindows.ts (+2 -2)
📝 src/application/javascript/JavaScriptArtifactGraphBuilder.ts (+67 -7)
📝 src/application/javascript/JavaScriptArtifactGraphContext.ts (+6 -3)
📝 src/application/javascript/JavaScriptArtifactGraphDocuments.ts (+1 -1)
📝 src/application/javascript/JavaScriptArtifactGraphFindings.ts (+4 -4)
📝 src/application/javascript/JavaScriptArtifactGraphStructure.ts (+1 -1)
📝 src/application/javascript/JavaScriptArtifactPathResolution.test.ts (+77 -25)
📝 src/application/javascript/JavaScriptArtifactPathResolution.ts (+60 -17)
📝 src/application/javascript/JavaScriptArtifactReconstruction.ts (+8 -6)
📝 src/application/javascript/JavaScriptModuleRelationships.ts (+1 -1)
📝 src/domain/javascript/javascriptArtifactFiles.ts (+6 -0)
📝 tests/acceptance/applications/JavaScriptModuleLookupCli.test.ts (+9 -26)
➕ tests/acceptance/applications/packagePathInventoryCli.test.ts (+160 -0)

📄 Description

Summary

Match Node package-main lookup while preserving artifact directory ownership and unknown symlink targets in offline JavaScript reconstruction.

Problem and expected behavior

Node ignores a non-string main and can resolve an index. CommonJS probes normalized main paths; legacy ESM also finalizes the original main plus its selected suffix. Collapsing those stages creates false or missing dependencies.

Retained directory paths also need their container identity: an outer directory cannot establish a package inside a nested ASAR. The inventory's root spelling must match resolver candidates. A symlink at a selected final target, such as lib/.js for main ./lib/, establishes uncertainty rather than absence.

The slash-main case was reported in the resolver discussion; the retained-path gaps were raised in review. No separate issue was identified for these fixes.

Change and scope

  • Interpret non-string main as absent only for Node loaders and keep generic filesystem policy separate.
  • Preserve loader-specific probes, finalization failures, and CommonJS ancestor-stop behavior. Root-pointing mains cannot select root dotfile decoys.
  • Project retained directory and symlink facts with container digests. Normalize the inventory root once and share those facts with graph relationships and the Electron summary.
  • Check directory ownership before fallback inference. Recognize nested container roots explicitly.
  • Preserve unknown symlink targets at file, metadata, and main-selection boundaries. Do not follow links or substitute a later candidate. Propagate the limitation through module and source-map consumers.

Existing upstream integrations and the bounded Apple fixture compiler timeout are retained.

Contract and boundary impact

  • Semantic owners: retained inventory path projection, package metadata interpretation, and Node package-main resolution.
  • Contribution scope: static correctness within existing boundaries; a new maintainer authority decision is not applicable.
  • CLI/MCP: public schemas/catalog unchanged; relationships retain loader mode, selected candidate, and unavailable limitations.
  • Compatibility: generic metadata/confinement policy remains intact. Symlink targets remain unestablished for both Node loaders.
  • Evidence: raw declarations and artifact identities remain retained; foreign-container paths and unknown targets cannot establish a dependency.
  • Authority/cleanup: no new target execution, symlink traversal, or resource ownership.
  • Generated metadata, dependencies, installation, or migrations: none.

Expected effects:

  • Read existing files or captured data.

Evidence and regression coverage

Existing CLI/stdio MCP and native-control sources cover non-string main, trailing slash, root/index fallback, dotfile decoys, and ESM finalization. Added regression sources cover outer versus nested ASAR directories, empty directories, artifact-root identity without metadata/index, foreign-container path facts, symlink finalization, unknown earlier probes, and linked parent directories.

The new symlink fixture is excluded on Windows because it creates filesystem links; platform-independent source-level cases still cover retained symlink ownership. No new cases have been executed.

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity and source provenance remain preserved.
  • Unsupported and unavailable outcomes remain visible.

Validation performed

Merged current main (cf0a38ad9). An ESM subpath that normalizes to the artifact root is now admitted to directory-form rejection instead of being rejected as an empty declaration. Retained path owners come from a container map lookup. Executed on Node 24.19.0 (macOS arm64): JavaScriptArtifactPathResolution.test.ts, packagePathInventoryCli, JavaScriptModuleLookupCli and packageSpecifierSuffixCli, 175/175 passed through CLI and stdio MCP, including native Node controls; tsc --noEmit and npm run lint have 0 errors. Not run: Windows, Node 22 native controls (they expect Node 24 loader codes), the full suite, real Electron apps, large-input timing.

Compatibility, safety, and release

No public schema or dependency change. Same-container admission remains mandatory for retained paths. REA does not follow or execute symlinks. Latest runtime and cross-platform behavior remain unverified.

Author readiness

Human owner: @rohitg00

  • 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

  • Concrete problem, scope, related discussion, and semantic owners identified.
  • Retained path consumers and regression sources updated.
  • Final source diff reviewed for unrelated changes and unsupported claims.
  • Latest changes verified by required automated checks.

🔄 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/2702 **Author:** [@rohitg00](https://github.com/rohitg00) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/node-package-main-resolution` --- ### 📝 Commits (10+) - [`4d4e30d`](https://github.com/morluto/rea/commit/4d4e30da8c238594f5144c8a8dde3ef606319a29) fix(javascript): match Node package main resolution - [`f6802f5`](https://github.com/morluto/rea/commit/f6802f52a1824dee557bd7b8a48717e8d17c0e24) Merge upstream main for current package main verification - [`a0424a2`](https://github.com/morluto/rea/commit/a0424a2dbbe6fe24b5dec7f09b9db09449d7feea) chore: merge upstream CI and fixture fixes - [`87bdcf8`](https://github.com/morluto/rea/commit/87bdcf84425abaa39b620414f6cb29a123fd7740) chore: merge upstream parameter provenance test fix - [`443b404`](https://github.com/morluto/rea/commit/443b404caed1391f5348e4b5a21cc586b49bbb8c) test(apple): bound cold archive fixture compilation - [`79ba2a8`](https://github.com/morluto/rea/commit/79ba2a89d7d0a414e4f41b6bbd721e4d2a1cdec5) chore: merge upstream CI and provenance test fixes - [`a765cae`](https://github.com/morluto/rea/commit/a765cae3365d50f6473d8f9cc2301998a64d018e) fix(javascript): resolve package main indexes at artifact root - [`e439438`](https://github.com/morluto/rea/commit/e4394384a97c5d7cd0d560b546786965a9c283b5) test(release): scope preflight schema to selected jobs - [`97ec5a5`](https://github.com/morluto/rea/commit/97ec5a57f191018a9e581cea1fe747da6ba39d86) test(ci): include cold-cache Apple fixture timeout correction - [`cd0afee`](https://github.com/morluto/rea/commit/cd0afeebb4dbc01f9e7a6d5c7fe2abcd25834cf9) test(ci): merge shared release and Apple fixture fixes ### 📊 Changes **15 files changed** (+422 additions, -101 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/ElectronBoundaryAnalysis.ts` (+18 -7) 📝 `src/application/javascript/ElectronBoundaryGraphNative.ts` (+2 -1) 📝 `src/application/javascript/ElectronBoundaryGraphWindows.ts` (+2 -2) 📝 `src/application/javascript/JavaScriptArtifactGraphBuilder.ts` (+67 -7) 📝 `src/application/javascript/JavaScriptArtifactGraphContext.ts` (+6 -3) 📝 `src/application/javascript/JavaScriptArtifactGraphDocuments.ts` (+1 -1) 📝 `src/application/javascript/JavaScriptArtifactGraphFindings.ts` (+4 -4) 📝 `src/application/javascript/JavaScriptArtifactGraphStructure.ts` (+1 -1) 📝 `src/application/javascript/JavaScriptArtifactPathResolution.test.ts` (+77 -25) 📝 `src/application/javascript/JavaScriptArtifactPathResolution.ts` (+60 -17) 📝 `src/application/javascript/JavaScriptArtifactReconstruction.ts` (+8 -6) 📝 `src/application/javascript/JavaScriptModuleRelationships.ts` (+1 -1) 📝 `src/domain/javascript/javascriptArtifactFiles.ts` (+6 -0) 📝 `tests/acceptance/applications/JavaScriptModuleLookupCli.test.ts` (+9 -26) ➕ `tests/acceptance/applications/packagePathInventoryCli.test.ts` (+160 -0) </details> ### 📄 Description ## Summary Match Node package-main lookup while preserving artifact directory ownership and unknown symlink targets in offline JavaScript reconstruction. ## Problem and expected behavior Node ignores a non-string main and can resolve an index. CommonJS probes normalized main paths; legacy ESM also finalizes the original main plus its selected suffix. Collapsing those stages creates false or missing dependencies. Retained directory paths also need their container identity: an outer directory cannot establish a package inside a nested ASAR. The inventory's root spelling must match resolver candidates. A symlink at a selected final target, such as `lib/.js` for main `./lib/`, establishes uncertainty rather than absence. The slash-main case was reported in [the resolver discussion](https://github.com/morluto/rea/pull/2306#issuecomment-6132773314); the retained-path gaps were raised in review. No separate issue was identified for these fixes. ## Change and scope - Interpret non-string main as absent only for Node loaders and keep generic filesystem policy separate. - Preserve loader-specific probes, finalization failures, and CommonJS ancestor-stop behavior. Root-pointing mains cannot select root dotfile decoys. - Project retained directory and symlink facts with container digests. Normalize the inventory root once and share those facts with graph relationships and the Electron summary. - Check directory ownership before fallback inference. Recognize nested container roots explicitly. - Preserve unknown symlink targets at file, metadata, and main-selection boundaries. Do not follow links or substitute a later candidate. Propagate the limitation through module and source-map consumers. Existing upstream integrations and the bounded Apple fixture compiler timeout are retained. ## Contract and boundary impact - Semantic owners: retained inventory path projection, package metadata interpretation, and Node package-main resolution. - Contribution scope: static correctness within existing boundaries; a new maintainer authority decision is not applicable. - CLI/MCP: public schemas/catalog unchanged; relationships retain loader mode, selected candidate, and unavailable limitations. - Compatibility: generic metadata/confinement policy remains intact. Symlink targets remain unestablished for both Node loaders. - Evidence: raw declarations and artifact identities remain retained; foreign-container paths and unknown targets cannot establish a dependency. - Authority/cleanup: no new target execution, symlink traversal, or resource ownership. - Generated metadata, dependencies, installation, or migrations: none. Expected effects: - [x] Read existing files or captured data. ## Evidence and regression coverage Existing CLI/stdio MCP and native-control sources cover non-string main, trailing slash, root/index fallback, dotfile decoys, and ESM finalization. Added regression sources cover outer versus nested ASAR directories, empty directories, artifact-root identity without metadata/index, foreign-container path facts, symlink finalization, unknown earlier probes, and linked parent directories. The new symlink fixture is excluded on Windows because it creates filesystem links; platform-independent source-level cases still cover retained symlink ownership. No new cases have been executed. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity and source provenance remain preserved. - [x] Unsupported and unavailable outcomes remain visible. ## Validation performed Merged current main (cf0a38ad9). An ESM subpath that normalizes to the artifact root is now admitted to directory-form rejection instead of being rejected as an empty declaration. Retained path owners come from a container map lookup. Executed on Node 24.19.0 (macOS arm64): JavaScriptArtifactPathResolution.test.ts, packagePathInventoryCli, JavaScriptModuleLookupCli and packageSpecifierSuffixCli, 175/175 passed through CLI and stdio MCP, including native Node controls; `tsc --noEmit` and `npm run lint` have 0 errors. Not run: Windows, Node 22 native controls (they expect Node 24 loader codes), the full suite, real Electron apps, large-input timing. ## Compatibility, safety, and release No public schema or dependency change. Same-container admission remains mandatory for retained paths. REA does not follow or execute symlinks. Latest runtime and cross-platform behavior remain unverified. ## Author readiness Human owner: @rohitg00 - [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] Concrete problem, scope, related discussion, and semantic owners identified. - [x] Retained path consumers and regression sources updated. - [x] Final source diff reviewed for unrelated changes and unsupported claims. - [ ] Latest changes verified by required automated checks. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:05:46 +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#5327
No description provided.