[PR #4017] [CLOSED] fix(reference-source): honor Node require extension order #5992

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

📋 Pull Request Information

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

Base: main ← Head: fix/3895-reference-source-file-priority


📝 Commits (6)

  • 114e47c fix(reference-source): prefer files over directory indexes
  • 3cabfc7 fix(reference-source): respect Node require extensions
  • d23475a fix(reference-source): preserve TypeScript import-equals links
  • 3b88200 fix(reference-source): retain Node package main probes
  • 0534d6a fix(reference-source): align TypeScript candidate ordering
  • 6d52072 fix(reference-source): unify TypeScript import resolution

📊 Changes

5 files changed (+399 additions, -38 deletions)

View changed files

📝 src/application/ReferenceSourceImportEntries.test.ts (+221 -0)
📝 src/application/ReferenceSourceImportEntries.ts (+112 -21)
📝 src/application/javascript/JavaScriptArtifactPathResolution.ts (+12 -16)
➕ src/domain/javascript/nodeModulePathCandidates.ts (+27 -0)
📝 src/domain/referenceSourceImportParsing.ts (+27 -1)

📄 Description

Summary

Fixes #3895. Make reference-source require() edges follow Node's CommonJS extension order so sibling .cjs files do not incorrectly outrank directory indexes.

Problem and expected behavior

For require("./dep") with both dep.cjs and dep/index.js present, Node resolves the directory index because .cjs is not an implicit CommonJS extension probe. The previous resolver selected dep.cjs; a similar fixture could resolve fallback.cjs to itself instead of fallback/index.js.

Change and scope

Use shared Node file/index candidate helpers for runtime CommonJS require and package main resolution, with probes .js, .json, and .node. The parser retains an internal strategy hint for TypeScript import = require; those source edges keep extensionless .ts and index.ts resolution, including package main fallback. The hint is removed before graph projection, so the public schema is unchanged. Reuse the Node helper in JavaScriptArtifactPathResolution to avoid duplicated runtime extension semantics.

This is a focused fix within existing static reference-source analysis; no new capability or target interaction is introduced. Contribution scope decision: not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: application reference-source relationship resolution.
  • Contribution scope fit and linked maintainer decision: in-scope bug fix; no new capability requiring a scope decision.
  • CLI and MCP/tool-catalog contract: no schema, CLI, MCP, or catalog changes.
  • Provider, bridge, target-format, or platform compatibility: no provider or bridge changes.
  • Evidence, artifact, provenance, or reconstruction contract: only the resolved target of CommonJS require edges changes; source inventory and provenance remain unchanged.
  • Target selection, authority, upstream behavior, and cleanup details: no executable target interaction; imported source is not executed.
  • Generated metadata/package/installation impact: none.

Expected 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

  • Tests added or updated: src/application/ReferenceSourceImportEntries.test.ts covers runtime CommonJS candidates, TypeScript import-equals and static imports preferring source over emitted JS, package main JSON fallback, and .cts imports ignoring implicit .cjs/.mjs/.cts/.mts siblings. tests/boundary/cli/referenceSourceDirectoryImports.test.ts provides Node createRequire().resolve oracles for .cjs sibling/index collision and self-resolution.
  • Base reproduction or other evidence: fixtures now cover source-only TypeScript targets, source-vs-emitted package mains, non-probed sibling suffixes, and JSON package-main probes; the runtime boundary retains its Node resolver oracle.
  • User-visible CLI/MCP output: not applicable; graph edges use source candidates for TypeScript import syntax and Node candidates for runtime require.
  • Remaining proof gaps: full suite and npm run check:pr were not run. GitHub Actions have not run on the latest head; this fork PR requires maintainer approval before workflows can report required checks. The Turbo check:fast wrapper timed out under shared-worktree load; its typecheck and lint components passed when run directly. The compiled boundary test skips Windows because its existing no-follow reader is unsupported there.

Validation performed

  • npm run test:local -- src/application/ReferenceSourceImportEntries.test.ts — passed, 7 tests.
  • npm run test:changed -- --base origin/main — passed, 3 files and 207 tests.
  • npm run typecheck — passed.
  • npm run lint:details — passed with 0 errors; 1,444 warnings reported and module boundaries passed.
  • npm run compile and the compiled CLI boundary test — passed (1 test).
  • oxfmt --check on the 3 changed files — passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; Windows boundary coverage is skipped as noted above.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new execution, mutation, or authority; this remains static path resolution.

Author readiness

Human owner: @LuckVd

  • 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/4017 **Author:** [@LuckVd](https://github.com/LuckVd) **Created:** 10/15/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/3895-reference-source-file-priority` --- ### 📝 Commits (6) - [`114e47c`](https://github.com/morluto/rea/commit/114e47cfaaf2e383a406c50669dae1c4b4aadcd8) fix(reference-source): prefer files over directory indexes - [`3cabfc7`](https://github.com/morluto/rea/commit/3cabfc727f4c9e39f1c108829294ed429db0186e) fix(reference-source): respect Node require extensions - [`d23475a`](https://github.com/morluto/rea/commit/d23475a010747e9605cc3a8b9de77b1c7141d919) fix(reference-source): preserve TypeScript import-equals links - [`3b88200`](https://github.com/morluto/rea/commit/3b88200ec81a592ad398789e56ac9113e1606ad3) fix(reference-source): retain Node package main probes - [`0534d6a`](https://github.com/morluto/rea/commit/0534d6a2e206f77d03b42076d35904f99a8b5f22) fix(reference-source): align TypeScript candidate ordering - [`6d52072`](https://github.com/morluto/rea/commit/6d52072edbf72eea605b7bbe7576f3f6691e4a25) fix(reference-source): unify TypeScript import resolution ### 📊 Changes **5 files changed** (+399 additions, -38 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ReferenceSourceImportEntries.test.ts` (+221 -0) 📝 `src/application/ReferenceSourceImportEntries.ts` (+112 -21) 📝 `src/application/javascript/JavaScriptArtifactPathResolution.ts` (+12 -16) ➕ `src/domain/javascript/nodeModulePathCandidates.ts` (+27 -0) 📝 `src/domain/referenceSourceImportParsing.ts` (+27 -1) </details> ### 📄 Description ## Summary Fixes #3895. Make reference-source `require()` edges follow Node's CommonJS extension order so sibling `.cjs` files do not incorrectly outrank directory indexes. ## Problem and expected behavior For `require("./dep")` with both `dep.cjs` and `dep/index.js` present, Node resolves the directory index because `.cjs` is not an implicit CommonJS extension probe. The previous resolver selected `dep.cjs`; a similar fixture could resolve `fallback.cjs` to itself instead of `fallback/index.js`. ## Change and scope Use shared Node file/index candidate helpers for runtime CommonJS `require` and package `main` resolution, with probes `.js`, `.json`, and `.node`. The parser retains an internal strategy hint for TypeScript `import = require`; those source edges keep extensionless `.ts` and `index.ts` resolution, including package `main` fallback. The hint is removed before graph projection, so the public schema is unchanged. Reuse the Node helper in `JavaScriptArtifactPathResolution` to avoid duplicated runtime extension semantics. This is a focused fix within existing static reference-source analysis; no new capability or target interaction is introduced. Contribution scope decision: not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: application reference-source relationship resolution. - Contribution scope fit and linked maintainer decision: in-scope bug fix; no new capability requiring a scope decision. - CLI and MCP/tool-catalog contract: no schema, CLI, MCP, or catalog changes. - Provider, bridge, target-format, or platform compatibility: no provider or bridge changes. - Evidence, artifact, provenance, or reconstruction contract: only the resolved target of CommonJS `require` edges changes; source inventory and provenance remain unchanged. - Target selection, authority, upstream behavior, and cleanup details: no executable target interaction; imported source is not executed. - Generated metadata/package/installation impact: none. Expected 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 - Tests added or updated: `src/application/ReferenceSourceImportEntries.test.ts` covers runtime CommonJS candidates, TypeScript import-equals and static imports preferring source over emitted JS, package `main` JSON fallback, and `.cts` imports ignoring implicit `.cjs`/`.mjs`/`.cts`/`.mts` siblings. `tests/boundary/cli/referenceSourceDirectoryImports.test.ts` provides Node `createRequire().resolve` oracles for `.cjs` sibling/index collision and self-resolution. - Base reproduction or other evidence: fixtures now cover source-only TypeScript targets, source-vs-emitted package mains, non-probed sibling suffixes, and JSON package-main probes; the runtime boundary retains its Node resolver oracle. - User-visible CLI/MCP output: not applicable; graph edges use source candidates for TypeScript import syntax and Node candidates for runtime `require`. - Remaining proof gaps: full suite and `npm run check:pr` were not run. GitHub Actions have not run on the latest head; this fork PR requires maintainer approval before workflows can report required checks. The Turbo `check:fast` wrapper timed out under shared-worktree load; its typecheck and lint components passed when run directly. The compiled boundary test skips Windows because its existing no-follow reader is unsupported there. ## Validation performed - `npm run test:local -- src/application/ReferenceSourceImportEntries.test.ts` — passed, 7 tests. - `npm run test:changed -- --base origin/main` — passed, 3 files and 207 tests. - `npm run typecheck` — passed. - `npm run lint:details` — passed with 0 errors; 1,444 warnings reported and module boundaries passed. - `npm run compile` and the compiled CLI boundary test — passed (1 test). - `oxfmt --check` on the 3 changed files — passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; Windows boundary coverage is skipped as noted above. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new execution, mutation, or authority; this remains static path resolution. ## Author readiness Human owner: @LuckVd - [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:01 +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#5992
No description provided.