morluto/rea

Fork 0

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

[PR #4164] [MERGED] fix(reference): resolve extensionless CommonJS requires to JSON files #6102

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4164
Author: @kaoru0822-kitauji
Created: 10/15/2026
Status: ✅ Merged
Merged: 10/15/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/3915-reference-require-json

📝 Commits (1)

f003702 fix(reference): resolve extensionless CommonJS requires to JSON files

📊 Changes

3 files changed (+94 additions, -9 deletions)

View changed files

📝 src/application/ReferenceSourceImportEntries.ts (+35 -9)
📝 tests/boundary/cli/referenceSourceDirectoryImports.test.ts (+16 -0)
📝 tests/boundary/filesystem/referenceSourceDynamicImports.test.ts (+43 -0)

📄 Description

Summary

Fixes #3915. import-reference-source left require("./data") unresolved when the source tree had data.json, although Node's CommonJS loader selects that file. A requires edge now resolves X.json, X/index.json, and a JSON package main in Node's order. ESM and TypeScript imports still need an explicit .json extension.

Problem and expected behavior

resolveInternalSpecifier in src/application/ReferenceSourceImportEntries.ts probed "", .ts, .js, .mjs, .cjs, .tsx, .jsx for every edge kind and never tried .json. Node's CommonJS LOAD_AS_FILE tries X.js and then X.json before LOAD_AS_DIRECTORY (X/index.js, then X/index.json).

Issue fixture through the CLI (import-reference-source --json):

fixture	main	this PR
`main.cjs`: `require("./data")` + `data.json`	`data` unresolved, 2 files hashed, 1,192 bytes, 437 ms	`data.json` internal, 2 hashed, 1,195 bytes, 397 ms
`require("./data.json")` + `data.json`	`data.json` internal, 1,195 bytes	unchanged
`require("./data")`, no `data.json`	`data` unresolved, 975 bytes	unchanged
`main.mjs`: `import "./data"` + `data.json`	`data` unresolved, 1,191 bytes	unchanged (ESM requires the extension)

Timings are single cold CLI runs.

Change and scope

Probe suffixes are now extension slots. Within a slot, files come before directory indexes. For requires edges, .json joins the .js slot, which gives the order X.js, X.json, X/index.js, X/index.json. An existing X.js still wins, and a .json file beats a directory index, as in Node.
The same slots drive package-main resolution, so {"main":"config"} selects config.json for a require.
imports and other edge kinds keep the previous candidate list and order, so ESM and TypeScript resolution is unchanged.

Contract and boundary impact

Semantic owner and earliest changed stage: the reference-source resolver owns specifier resolution; the parser's requires kind selects CommonJS loader semantics.
Contribution scope fit and linked maintainer decision: a bug fix within the existing static historical-source import; not applicable.
CLI and MCP/tool-catalog contract: schemas unchanged.
Provider, bridge, target-format, or platform compatibility: provider-independent; reference-source reading still requires no-follow opens (macOS/Linux).
Evidence, artifact, provenance, or reconstruction contract: a valid local JSON dependency edge is no longer reported as unresolved. File inventory and hashes are unchanged.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.

Evidence and regression coverage

tests/boundary/cli/referenceSourceDirectoryImports.test.ts adds six require cases. Node's own createRequire(...).resolve checks each expected target before the CLI runs:
- ./data resolves to data.json;
- the explicit ./data.json resolves to data.json;
- ./conf resolves to conf.json over conf/index.js;
- ./both resolves to both.js over both.json;
- ./settings resolves to settings/index.json;
- ./jpkg with {"main":"config"} resolves to jpkg/config.json.
On a main build, the four cases that need JSON probing failed and the two control cases passed.
tests/boundary/filesystem/referenceSourceDynamicImports.test.ts adds the issue's pair: require("./data") resolves to internal data.json, while import "./data" from .mjs stays unresolved, and all three files stay hashed. It fails without the fix.
Real data: npm's installed source tree (/opt/homebrew/lib/node_modules/npm) and its bundled dependencies, median of 3 CLI runs:

tree	entries	relationships	relationship digest (main = PR)	output bytes	main	this PR
npm	450	671	`a0380d00f040421b`	192,175	572 ms	606 ms
npm `node_modules`	1,399	1,874 (21 internal JSON requires)	`751e1f990fc769be`	568,756	1,118 ms	1,102–1,174 ms

The relationship graphs are byte-identical and the timing difference is within noise. A scan of the repository's dependencies and the global npm tree found no package that uses an extensionless JSON require. The construct is uncommon in published packages, so the issue fixture and the tests above are the only places this change was exercised.

Remaining proof gaps: the existing slot order puts X/index.ts before X.js, which Node orders the other way for runtime files. This PR keeps that unchanged and only places .json in Node's order relative to .js. A package main is still consulted only when the package also has an index.*, for JavaScript and JSON mains alike. That gap predates this PR and is #3896, which has an open fix in #3973. Once that lands, mains reached without an index will use the CommonJS slots added here. The application analyzer's separate CommonJS probing (#2021, #2022) is out of scope.

Validation performed

npx vitest run tests/boundary/filesystem/referenceSourceDynamicImports.test.ts tests/boundary/cli/referenceSourceDirectoryImports.test.ts — 3 passed after building; on a main build the CLI test failed for the four JSON cases, and the filesystem test failed with the resolver change removed.
npx vitest run src/application/ReferenceSourceImportEntries.test.ts src/application/ReferenceSourceImportTypes.test.ts src/domain/referenceSourceGraph.test.ts src/domain/referenceSourceSummary.test.ts tests/boundary/cli/referenceSource tests/boundary/filesystem/referenceSource tests/boundary/mcp/referenceSource — 25 files, 132 passed, 2 skipped.
npm run check:fast — passed.
CLI runs on the issue fixtures and the npm trees above, on both builds.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; macOS arm64, Node 26.5.
Package or release metadata impact: none.
Security, privacy, process, or containment review: the change only adds lookups in the already-read file inventory.

Author readiness

Human owner:

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.

🤖 Generated with Claude Code

_{🔄 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/4164 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/3915-reference-require-json` --- ### 📝 Commits (1) - [`f003702`](https://github.com/morluto/rea/commit/f0037029e9e8700635a72638be9bfd0c38f706bc) fix(reference): resolve extensionless CommonJS requires to JSON files ### 📊 Changes **3 files changed** (+94 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ReferenceSourceImportEntries.ts` (+35 -9) 📝 `tests/boundary/cli/referenceSourceDirectoryImports.test.ts` (+16 -0) 📝 `tests/boundary/filesystem/referenceSourceDynamicImports.test.ts` (+43 -0) </details> ### 📄 Description ## Summary Fixes #3915. `import-reference-source` left `require("./data")` unresolved when the source tree had `data.json`, although Node's CommonJS loader selects that file. A `requires` edge now resolves `X.json`, `X/index.json`, and a JSON package `main` in Node's order. ESM and TypeScript imports still need an explicit `.json` extension. ## Problem and expected behavior `resolveInternalSpecifier` in `src/application/ReferenceSourceImportEntries.ts` probed `"", .ts, .js, .mjs, .cjs, .tsx, .jsx` for every edge kind and never tried `.json`. Node's CommonJS `LOAD_AS_FILE` tries `X.js` and then `X.json` before `LOAD_AS_DIRECTORY` (`X/index.js`, then `X/index.json`). Issue fixture through the CLI (`import-reference-source --json`): | fixture | main | this PR | |---|---|---| | `main.cjs`: `require("./data")` + `data.json` | `data` **unresolved**, 2 files hashed, 1,192 bytes, 437 ms | `data.json` internal, 2 hashed, 1,195 bytes, 397 ms | | `require("./data.json")` + `data.json` | `data.json` internal, 1,195 bytes | unchanged | | `require("./data")`, no `data.json` | `data` unresolved, 975 bytes | unchanged | | `main.mjs`: `import "./data"` + `data.json` | `data` unresolved, 1,191 bytes | unchanged (ESM requires the extension) | Timings are single cold CLI runs. ## Change and scope - Probe suffixes are now extension slots. Within a slot, files come before directory indexes. For `requires` edges, `.json` joins the `.js` slot, which gives the order `X.js`, `X.json`, `X/index.js`, `X/index.json`. An existing `X.js` still wins, and a `.json` file beats a directory index, as in Node. - The same slots drive package-`main` resolution, so `{"main":"config"}` selects `config.json` for a require. - `imports` and other edge kinds keep the previous candidate list and order, so ESM and TypeScript resolution is unchanged. ## Contract and boundary impact - Semantic owner and earliest changed stage: the reference-source resolver owns specifier resolution; the parser's `requires` kind selects CommonJS loader semantics. - Contribution scope fit and linked maintainer decision: a bug fix within the existing static historical-source import; not applicable. - CLI and MCP/tool-catalog contract: schemas unchanged. - Provider, bridge, target-format, or platform compatibility: provider-independent; reference-source reading still requires no-follow opens (macOS/Linux). - Evidence, artifact, provenance, or reconstruction contract: a valid local JSON dependency edge is no longer reported as unresolved. File inventory and hashes are unchanged. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - `tests/boundary/cli/referenceSourceDirectoryImports.test.ts` adds six `require` cases. Node's own `createRequire(...).resolve` checks each expected target before the CLI runs: - `./data` resolves to `data.json`; - the explicit `./data.json` resolves to `data.json`; - `./conf` resolves to `conf.json` over `conf/index.js`; - `./both` resolves to `both.js` over `both.json`; - `./settings` resolves to `settings/index.json`; - `./jpkg` with `{"main":"config"}` resolves to `jpkg/config.json`. On a main build, the four cases that need JSON probing failed and the two control cases passed. - `tests/boundary/filesystem/referenceSourceDynamicImports.test.ts` adds the issue's pair: `require("./data")` resolves to internal `data.json`, while `import "./data"` from `.mjs` stays unresolved, and all three files stay hashed. It fails without the fix. - Real data: npm's installed source tree (`/opt/homebrew/lib/node_modules/npm`) and its bundled dependencies, median of 3 CLI runs: | tree | entries | relationships | relationship digest (main = PR) | output bytes | main | this PR | |---|---|---|---|---|---|---| | npm | 450 | 671 | `a0380d00f040421b` | 192,175 | 572 ms | 606 ms | | npm `node_modules` | 1,399 | 1,874 (21 internal JSON requires) | `751e1f990fc769be` | 568,756 | 1,118 ms | 1,102–1,174 ms | The relationship graphs are byte-identical and the timing difference is within noise. A scan of the repository's dependencies and the global npm tree found no package that uses an extensionless JSON `require`. The construct is uncommon in published packages, so the issue fixture and the tests above are the only places this change was exercised. - Remaining proof gaps: the existing slot order puts `X/index.ts` before `X.js`, which Node orders the other way for runtime files. This PR keeps that unchanged and only places `.json` in Node's order relative to `.js`. A package `main` is still consulted only when the package also has an `index.*`, for JavaScript and JSON mains alike. That gap predates this PR and is #3896, which has an open fix in #3973. Once that lands, mains reached without an index will use the CommonJS slots added here. The application analyzer's separate CommonJS probing (#2021, #2022) is out of scope. ## Validation performed - `npx vitest run tests/boundary/filesystem/referenceSourceDynamicImports.test.ts tests/boundary/cli/referenceSourceDirectoryImports.test.ts` — 3 passed after building; on a main build the CLI test failed for the four JSON cases, and the filesystem test failed with the resolver change removed. - `npx vitest run src/application/ReferenceSourceImportEntries.test.ts src/application/ReferenceSourceImportTypes.test.ts src/domain/referenceSourceGraph.test.ts src/domain/referenceSourceSummary.test.ts tests/boundary/cli/referenceSource tests/boundary/filesystem/referenceSource tests/boundary/mcp/referenceSource` — 25 files, 132 passed, 2 skipped. - `npm run check:fast` — passed. - CLI runs on the issue fixtures and the npm trees above, on both builds. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; macOS arm64, Node 26.5. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the change only adds lookups in the already-read file inventory. ## Author readiness Human owner:  - [ ] 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <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:52 +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#6102

No description provided.

Rows
Columns