morluto/rea

Fork 0

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

[PR #3560] [CLOSED] fix(reference): stop resolving a bare extensionless directory index #5824

New issue

Closed

opened 2026-10-31 01:09:42 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:42 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3560
Author: @eco-null
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/reference-source-extensionless-index

📝 Commits (1)

74a2678 fix(reference): stop resolving a bare extensionless directory index

📊 Changes

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

View changed files

📝 src/application/ReferenceSourceImportEntries.test.ts (+70 -0)
📝 src/application/ReferenceSourceImportEntries.ts (+12 -5)

📄 Description

Summary

import-reference-source recorded a CommonJS dependency on an extensionless dep/index file even when the loadable sibling dep.js existed, so the persisted graph named the wrong module implementation.

Before: require("./dep") with dep.js and dep/index present -> to: "dep/index", resolution: "internal".
After: the same call -> to: "dep.js", which is what Node resolves it to.

Fixes #3381

Problem and expected behavior

Smallest trigger: a source tree containing main.cjs = require("./dep");, a dep.js file, and an extensionless dep/index file.

resolveInternalSpecifier built one interleaved candidate list per suffix, and the empty suffix produced a bare directory index:

dep, dep/index, dep.ts, dep/index.ts, dep.js, dep/index.js, ...

dep/index therefore matched before any file extension, so dep.js was never probed. Node's loader probes the exact path and every file extension (LOAD_AS_FILE) before a directory index (LOAD_AS_DIRECTORY -> LOAD_INDEX), and it never loads a bare extensionless index: it resolves this call to dep.js and reports MODULE_NOT_FOUND for dep/index.

Change and scope

src/application/ReferenceSourceImportEntries.ts: stop generating the bare extensionless index candidate, and keep the existing per-extension priority (exact path first, then for each of .ts, .js, .mjs, .cjs, .tsx, .jsx: the sibling file, then that extension's index).

Deliberately not included:

Reordering every file probe ahead of every index probe. Maintainers rejected that in #2770 ("Changes ordinary CommonJS candidate priority"), and it would let a source-only dep.cjs sibling outrank dep/index.js, which Node does not do because .cjs is not a require extension.
Package resolution (package.json#main/exports), which this importer does not model (tracked separately, e.g. #3383).

Contract and boundary impact

Semantic owner and earliest changed stage: specifier resolution for reference-source imports (resolveInternalSpecifier), which owns to/resolution for parsed internal relationships.
Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries — not applicable.
CLI and MCP/tool-catalog contract: none. The import-reference-source result shape is unchanged; only the selected to path can differ, and only in the case above.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: resolution: "internal" still marks only inventoried targets, and a specifier with no match stays unresolved.
Target selection, authority, upstream behavior, and cleanup details: none. The importer reads files only; nothing is executed.
Generated metadata (docs/public/product-catalog.json), 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.

Evidence and regression coverage

Tests added in src/application/ReferenceSourceImportEntries.test.ts:

prefers a sibling file over an extensionless directory index — the reported case; fails on the merge-base revision, passes with the fix.
keeps a directory index ahead of a source-only sibling extension — pins the preserved priority (dep/index.js over dep.cjs), matching Node.
resolves a directory spelling to its JavaScript index — require("./dep/") still selects dep/index.js.

Base reproduction — source file reverted to the merge-base revision 1438422df with these tests present, npm run test:focused:

FAIL  src/application/ReferenceSourceImportEntries.test.ts > prefers a sibling file over an extensionless directory index
AssertionError: expected [ { from_path: 'main.cjs', …(4) } ] to deep equally contain { from_path: 'main.cjs', …(4) }

- Expected:
{
  "from_path": "main.cjs",
  "kind": "requires",
  "parse_state": "parsed",
  "resolution": "internal",
  "to": "dep.js",
}

+ Received:
[
  {
    "from_path": "main.cjs",
    "kind": "requires",
    "parse_state": "parsed",
    "resolution": "internal",
    "to": "dep/index",
  },
]

 Test Files  1 failed (1)
      Tests  1 failed | 5 passed (6)

Node controls for the same fixtures (createRequire(...).resolve(...) on this host):

Fixture	Node result
`./dep` with `dep.js` + `dep/index`	`dep.js`
`./dep` with `dep.cjs` + `dep/index.js`	`dep/index.js`
`./dep` with only `dep/index`	`MODULE_NOT_FOUND`
`./dep/` with `dep/index.js`	`dep/index.js`

Remaining proof gaps: import-reference-source opens files without following links, which this Windows host reports as unavailable, so there is no compiled CLI run and no tests/boundary/cli/referenceSourceDirectoryImports.test.ts run (that suite is also skipIf(win32)); both need a Linux, macOS, or WSL host.

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. (The unchecked box records that the compiled CLI path could not be exercised on this host; the gap is stated above.)

Validation performed

npm run test:focused -- src/application/ReferenceSourceImportEntries.test.ts — 6 passed.
npm run check:fast -- --force — typecheck and lint, 2 successful / 2 total, 0 cached (10.1s; cache bypassed so the result reflects the current revision).
Merge-base check with the source file reverted to 1438422df — 1 failed / 5 passed, failure quoted above; the file was then restored (sha256 c0c22d97…) and the working tree is clean.
npm run test:local over all 249 src/domain and src/application test files, in 7 batches of 40 — 243 files passed, 6 failed (10 tests), all in the first batch: Doctor.test.ts, LinuxHopper.test.ts, MacHopper.test.ts, MacHopperPaths.test.ts, Setup.planning.test.ts, BinarySessionLiveness.test.ts. A control run of exactly those 6 files with the source file at the current revision and at the merge-base produced an identical 6-file / 10-test failure set, so they are host-related (macOS/Linux Hopper install and discovery paths on a Windows host, plus one session-liveness timing case) and not caused by this change.
npm run verify:test-discovery — all 1284 test files discovered exactly once.
npm run format:check — all matched files use the correct format.

Toolchain for every command above: Node 24.18.0 and npm 11.16.0, matching .nvmrc and package.json#packageManager.

Compatibility, safety, and release

Breaking changes or migration steps: none. A previously wrong to is re-resolved only where an extensionless directory index shadowed a file candidate; no schema, option, or CLI surface changes.
Real Hopper/Ghidra, browser, or OS coverage: none applicable — this path is platform-independent source parsing — but the CLI-level import could not run on this host as noted above.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none; no target interaction, no execution, no new I/O.

Author readiness

Human owner: @eco-null

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.

The change was developed with AI assistance. The human owner named above reviews the diff and the evidence, then completes these declarations before requesting 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/3560 **Author:** [@eco-null](https://github.com/eco-null) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/reference-source-extensionless-index` --- ### 📝 Commits (1) - [`74a2678`](https://github.com/morluto/rea/commit/74a2678e2332378cb3375ef393a479e5659af1fd) fix(reference): stop resolving a bare extensionless directory index ### 📊 Changes **2 files changed** (+82 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ReferenceSourceImportEntries.test.ts` (+70 -0) 📝 `src/application/ReferenceSourceImportEntries.ts` (+12 -5) </details> ### 📄 Description ## Summary `import-reference-source` recorded a CommonJS dependency on an extensionless `dep/index` file even when the loadable sibling `dep.js` existed, so the persisted graph named the wrong module implementation. - Before: `require("./dep")` with `dep.js` and `dep/index` present -> `to: "dep/index"`, `resolution: "internal"`. - After: the same call -> `to: "dep.js"`, which is what Node resolves it to. Fixes #3381 ## Problem and expected behavior Smallest trigger: a source tree containing `main.cjs` = `require("./dep");`, a `dep.js` file, and an extensionless `dep/index` file. `resolveInternalSpecifier` built one interleaved candidate list per suffix, and the empty suffix produced a bare directory index: ```text dep, dep/index, dep.ts, dep/index.ts, dep.js, dep/index.js, ... ``` `dep/index` therefore matched before any file extension, so `dep.js` was never probed. Node's loader probes the exact path and every file extension (`LOAD_AS_FILE`) before a directory index (`LOAD_AS_DIRECTORY` -> `LOAD_INDEX`), and it never loads a bare extensionless index: it resolves this call to `dep.js` and reports `MODULE_NOT_FOUND` for `dep/index`. ## Change and scope `src/application/ReferenceSourceImportEntries.ts`: stop generating the bare extensionless `index` candidate, and keep the existing per-extension priority (exact path first, then for each of `.ts`, `.js`, `.mjs`, `.cjs`, `.tsx`, `.jsx`: the sibling file, then that extension's index). Deliberately not included: - Reordering every file probe ahead of every index probe. Maintainers rejected that in #2770 ("Changes ordinary CommonJS candidate priority"), and it would let a source-only `dep.cjs` sibling outrank `dep/index.js`, which Node does not do because `.cjs` is not a `require` extension. - Package resolution (`package.json#main`/`exports`), which this importer does not model (tracked separately, e.g. #3383). ## Contract and boundary impact - Semantic owner and earliest changed stage: specifier resolution for reference-source imports (`resolveInternalSpecifier`), which owns `to`/`resolution` for parsed `internal` relationships. - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries — not applicable. - CLI and MCP/tool-catalog contract: none. The `import-reference-source` result shape is unchanged; only the selected `to` path can differ, and only in the case above. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: `resolution: "internal"` still marks only inventoried targets, and a specifier with no match stays `unresolved`. - Target selection, authority, upstream behavior, and cleanup details: none. The importer reads files only; nothing is executed. - Generated metadata (`docs/public/product-catalog.json`), 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. - [ ] 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 in `src/application/ReferenceSourceImportEntries.test.ts`: 1. **prefers a sibling file over an extensionless directory index** — the reported case; fails on the merge-base revision, passes with the fix. 2. **keeps a directory index ahead of a source-only sibling extension** — pins the preserved priority (`dep/index.js` over `dep.cjs`), matching Node. 3. **resolves a directory spelling to its JavaScript index** — `require("./dep/")` still selects `dep/index.js`. Base reproduction — source file reverted to the merge-base revision `1438422df` with these tests present, `npm run test:focused`: ```text FAIL src/application/ReferenceSourceImportEntries.test.ts > prefers a sibling file over an extensionless directory index AssertionError: expected [ { from_path: 'main.cjs', …(4) } ] to deep equally contain { from_path: 'main.cjs', …(4) } - Expected: { "from_path": "main.cjs", "kind": "requires", "parse_state": "parsed", "resolution": "internal", "to": "dep.js", } + Received: [ { "from_path": "main.cjs", "kind": "requires", "parse_state": "parsed", "resolution": "internal", "to": "dep/index", }, ] Test Files 1 failed (1) Tests 1 failed | 5 passed (6) ``` Node controls for the same fixtures (`createRequire(...).resolve(...)` on this host): | Fixture | Node result | | --- | --- | | `./dep` with `dep.js` + `dep/index` | `dep.js` | | `./dep` with `dep.cjs` + `dep/index.js` | `dep/index.js` | | `./dep` with only `dep/index` | `MODULE_NOT_FOUND` | | `./dep/` with `dep/index.js` | `dep/index.js` | - Remaining proof gaps: `import-reference-source` opens files without following links, which this Windows host reports as unavailable, so there is no compiled CLI run and no `tests/boundary/cli/referenceSourceDirectoryImports.test.ts` run (that suite is also `skipIf(win32)`); both need a Linux, macOS, or WSL host. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [ ] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. (The unchecked box records that the compiled CLI path could not be exercised on this host; the gap is stated above.) ## Validation performed - `npm run test:focused -- src/application/ReferenceSourceImportEntries.test.ts` — 6 passed. - `npm run check:fast -- --force` — `typecheck` and `lint`, 2 successful / 2 total, 0 cached (10.1s; cache bypassed so the result reflects the current revision). - Merge-base check with the source file reverted to `1438422df` — 1 failed / 5 passed, failure quoted above; the file was then restored (sha256 `c0c22d97…`) and the working tree is clean. - `npm run test:local` over all 249 `src/domain` and `src/application` test files, in 7 batches of 40 — 243 files passed, 6 failed (10 tests), all in the first batch: `Doctor.test.ts`, `LinuxHopper.test.ts`, `MacHopper.test.ts`, `MacHopperPaths.test.ts`, `Setup.planning.test.ts`, `BinarySessionLiveness.test.ts`. A control run of exactly those 6 files with the source file at the current revision and at the merge-base produced an identical 6-file / 10-test failure set, so they are host-related (macOS/Linux Hopper install and discovery paths on a Windows host, plus one session-liveness timing case) and not caused by this change. - `npm run verify:test-discovery` — all 1284 test files discovered exactly once. - `npm run format:check` — all matched files use the correct format. Toolchain for every command above: Node 24.18.0 and npm 11.16.0, matching `.nvmrc` and `package.json#packageManager`. ## Compatibility, safety, and release - Breaking changes or migration steps: none. A previously wrong `to` is re-resolved only where an extensionless directory index shadowed a file candidate; no schema, option, or CLI surface changes. - Real Hopper/Ghidra, browser, or OS coverage: none applicable — this path is platform-independent source parsing — but the CLI-level import could not run on this host as noted above. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; no target interaction, no execution, no new I/O. ## Author readiness Human owner: @eco-null - [ ] 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. The change was developed with AI assistance. The human owner named above reviews the diff and the evidence, then completes these declarations before requesting 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:09:42 +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#5824

No description provided.

Rows
Columns