[PR #4511] [MERGED] fix(source): retain CommonJS requires spelled as exact no-substitution templates #6279

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4511
Author: @hemv-857
Created: 10/16/2026
Status: ✅ Merged
Merged: 10/21/2026
Merged by: @morluto

Base: main ← Head: fix-4382-template-require


📝 Commits (2)

  • c60dea6 fix(source): retain CommonJS requires spelled as exact no-substitution templates
  • c09ab40 fix(source): admit TypeScript-wrapped exact require arguments via the shared literal reader

📊 Changes

3 files changed (+113 additions, -7 deletions)

View changed files

📝 src/domain/referenceSourceImportParsing.ts (+9 -7)
📝 tests/boundary/filesystem/referenceSourceComputedRequire.test.ts (+7 -0)
➕ tests/boundary/filesystem/referenceSourceTemplateRequire.test.ts (+97 -0)

📄 Description

Summary

Fixes #4382. The historical-source graph producer silently drops CommonJS dependencies spelled as exact no-substitution templates: require(`mod`) yields {relationships: [], parse_failures: [], limitations: []} while require("mod") yields a parsed requires edge.

Problem and expected behavior

Smallest trigger: a .cjs source file containing require(`mod`). Violated invariant: the supported historical-source CommonJS parser retains a dependency for an unshadowed loader called with an exact string, and an expression-free template supplies one exact string.

Root cause (source-inspected): in src/domain/referenceSourceImportParsing.ts, the require branch is gated by isModuleExpression, which narrows to StringLiteral | CallExpression. A no-substitution TemplateLiteral passes the callee and unshadowed-binding checks, then is filtered at that gate before moduleSpecifierFromExpression can run, so it exits with no relationship, no parse failure, and no limitation. The extractor and safeModuleName are likewise StringLiteral-only.

Change and scope

  • Admit expression-free TemplateLiteral arguments at the CommonJS require gate (isExactTemplateExpression).
  • Read exact string arguments through the existing shared exact-literal reader readExactJavaScriptLiteral (src/domain/javascript/javascriptAstValues.ts), which already treats zero-expression templates as exact strings.
  • Per Codex review (P2, addressed in c09ab40fd): the require-branch gate matched the outer AST node while the reader unwraps runtime-transparent TS wrappers, so require(pkg as const) and require(pkg!) were still omitted. The gate was removed; the shared reader alone owns argument exactness, which also retains TS-wrapped quoted requires (same root cause, superset of the typed outcome).

Intentionally not included, per the issue's typed outcome:

  • Dynamic import() keeps the explicit <dynamic-import> partial with no invented target (template or computed).
  • Computed template requires (require(`./${name}.js`)) stay silent rather than guessing a target.
  • Static ESM template declarations keep their Babel parse failures.
  • safeModuleName and ImportExpression handling are untouched.

Contribution scope: not applicable — ordinary fix within existing domain boundaries; no provider, capability, or maintainer scope decision involved.

Contract and boundary impact

  • Semantic owner and earliest changed stage: src/domain/referenceSourceImportParsing.ts (domain graph producer); earliest changed stage is reference-source relationship emission during artifact/evidence registration.
  • Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries — not applicable.
  • CLI and MCP/tool-catalog contract: no new tool, option, or schema; observable effect is that the reference-source graph now includes the previously omitted requires edge for exact-template spellings (relationship sets can gain entries relative to 6.5.0).
  • Provider, bridge, target-format, or platform compatibility: none affected.
  • Evidence, artifact, provenance, or reconstruction contract: no invented targets; computed specifiers remain unknown/partial; quoted spellings unchanged.
  • Target selection, authority, upstream behavior, and cleanup details: not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none; no generated files touched.

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: new tests/boundary/filesystem/referenceSourceTemplateRequire.test.ts (template + quoted controls, computed require and computed dynamic-import controls); tests/boundary/filesystem/referenceSourceComputedRequire.test.ts gains the now-correct require(`./dep.js`) edge in unknown.cjs (previously encoded the silent omission this issue reports; member-callee variants remain gated by mutation indexing and produce no new edges).
  • Base reproduction: on the base revision, the new test fails on the templated assertion only (quoted control passes, all four issue controls pass), matching the issue's probe output. The issue's probe asserts the pre-fix omission, so the fixed behavior was verified by asserting its typed outcome instead (below).
  • User-visible CLI/MCP output (if applicable): reference-source graph relationships only; no new tool output.
  • Remaining proof gaps: no full CLI/MCP end-to-end import run for this spelling; evidence is the boundary workflow plus direct parser assertions.

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.

Validation performed

  • npm ci — locked dependency installation refreshed (main's lockfile added tsx).
  • npm run test:focused -- tests/boundary/filesystem/referenceSourceTemplateRequire.test.ts — RED before fix (template edge missing, quoted control passing), 1/1 pass after.
  • npm run test:focused -- tests/boundary/filesystem/referenceSourceTemplateRequire.test.ts tests/boundary/filesystem/referenceSourceComputedRequire.test.ts tests/boundary/filesystem/referenceSourceDynamicImports.test.ts tests/boundary/filesystem/referenceSourceShadowedRequire.test.ts tests/boundary/cli/referenceSourceDirectoryImports.test.ts tests/boundary/filesystem/referenceSourceImport.test.ts — 6 files, 26/26 pass after updating the stale unknown.cjs expectation.
  • Direct parser probe (tsx, parseReferenceSourceImports on fixture.cjs): require(\mod`)returns{to: "mod", kind: "requires", resolution: "external", parse_state: "parsed", resolutionStrategy: "runtime-require"}; computed require returns no relationship; import(`mod`)stayspartial;import(`./${name}.js`)stays resolutionunknown`; static ESM template import/export still report parse failures — all issue controls asserted, exit 0.
  • Direct TS-wrapper parser probe (tsx): require(pkg as const), require("pkg" as const), require(pkg!), and require(pkg satisfies string) each return to: "pkg", parse_state: "parsed"; require(a${x}c as const) stays silent; import(pkg as const) stays <dynamic-import> partial.
  • npm run test:focused after review fix (c09ab40fd) — 6 files, 27/27 pass (adds .ts + createRequire boundary coverage: plain, as const, !, wrapped-string all retained; computed wrapped template silent).
  • npm run check:fast — typecheck + lint, 2/2 tasks successful (rerun after review fix).
  • Node v24.18.0, macOS arm64. No native provider involved (synthetic source bytes).

Compatibility, safety, and release

  • Breaking changes or migration steps: none intended; graphs may gain correct requires entries for template spellings relative to earlier releases.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable — source-boundary parser change, no provider or target interaction.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new I/O, execution, or authority; only parsing of already-selected source bytes.

Author readiness

Human owner: @hemv-857

  • 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.

Maintainer blocker verification

The require-wrapper finding is already fixed at the producing parser: exactness delegates to readExactJavaScriptLiteral through moduleSpecifierFromExpression rather than an AST-kind gate. On this exact head, the focused referenceSourceTemplateRequire and referenceSourceComputedRequire suites passed 3/3, including as-const/non-null wrappers and unresolved computed-template controls. No additional source edit was needed.


🔄 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/4511 **Author:** [@hemv-857](https://github.com/hemv-857) **Created:** 10/16/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix-4382-template-require` --- ### 📝 Commits (2) - [`c60dea6`](https://github.com/morluto/rea/commit/c60dea6be50eccf92de10e66d716313d0b7ce931) fix(source): retain CommonJS requires spelled as exact no-substitution templates - [`c09ab40`](https://github.com/morluto/rea/commit/c09ab40fd42217b5c6555bd6c68ee380d0f4ed81) fix(source): admit TypeScript-wrapped exact require arguments via the shared literal reader ### 📊 Changes **3 files changed** (+113 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/referenceSourceImportParsing.ts` (+9 -7) 📝 `tests/boundary/filesystem/referenceSourceComputedRequire.test.ts` (+7 -0) ➕ `tests/boundary/filesystem/referenceSourceTemplateRequire.test.ts` (+97 -0) </details> ### 📄 Description ## Summary Fixes #4382. The historical-source graph producer silently drops CommonJS dependencies spelled as exact no-substitution templates: `` require(`mod`) `` yields `{relationships: [], parse_failures: [], limitations: []}` while `require("mod")` yields a parsed `requires` edge. ## Problem and expected behavior Smallest trigger: a `.cjs` source file containing `` require(`mod`) ``. Violated invariant: the supported historical-source CommonJS parser retains a dependency for an unshadowed loader called with an exact string, and an expression-free template supplies one exact string. Root cause (source-inspected): in `src/domain/referenceSourceImportParsing.ts`, the require branch is gated by `isModuleExpression`, which narrows to `StringLiteral | CallExpression`. A no-substitution `TemplateLiteral` passes the callee and unshadowed-binding checks, then is filtered at that gate before `moduleSpecifierFromExpression` can run, so it exits with no relationship, no parse failure, and no limitation. The extractor and `safeModuleName` are likewise StringLiteral-only. ## Change and scope - Admit expression-free `TemplateLiteral` arguments at the CommonJS require gate (`isExactTemplateExpression`). - Read exact string arguments through the existing shared exact-literal reader `readExactJavaScriptLiteral` (`src/domain/javascript/javascriptAstValues.ts`), which already treats zero-expression templates as exact strings. - Per Codex review (P2, addressed in `c09ab40fd`): the require-branch gate matched the outer AST node while the reader unwraps runtime-transparent TS wrappers, so `require(`pkg` as const)` and `require(`pkg`!)` were still omitted. The gate was removed; the shared reader alone owns argument exactness, which also retains TS-wrapped quoted requires (same root cause, superset of the typed outcome). Intentionally not included, per the issue's typed outcome: - Dynamic `import()` keeps the explicit `<dynamic-import>` partial with no invented target (template or computed). - Computed template requires (`` require(`./${name}.js`) ``) stay silent rather than guessing a target. - Static ESM template declarations keep their Babel parse failures. - `safeModuleName` and `ImportExpression` handling are untouched. Contribution scope: not applicable — ordinary fix within existing domain boundaries; no provider, capability, or maintainer scope decision involved. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/domain/referenceSourceImportParsing.ts` (domain graph producer); earliest changed stage is reference-source relationship emission during artifact/evidence registration. - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries — not applicable. - CLI and MCP/tool-catalog contract: no new tool, option, or schema; observable effect is that the reference-source graph now includes the previously omitted `requires` edge for exact-template spellings (relationship sets can gain entries relative to 6.5.0). - Provider, bridge, target-format, or platform compatibility: none affected. - Evidence, artifact, provenance, or reconstruction contract: no invented targets; computed specifiers remain unknown/partial; quoted spellings unchanged. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none; no generated files touched. 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: new `tests/boundary/filesystem/referenceSourceTemplateRequire.test.ts` (template + quoted controls, computed require and computed dynamic-import controls); `tests/boundary/filesystem/referenceSourceComputedRequire.test.ts` gains the now-correct `` require(`./dep.js`) `` edge in `unknown.cjs` (previously encoded the silent omission this issue reports; member-callee variants remain gated by mutation indexing and produce no new edges). - Base reproduction: on the base revision, the new test fails on the `templated` assertion only (quoted control passes, all four issue controls pass), matching the issue's probe output. The issue's probe asserts the pre-fix omission, so the fixed behavior was verified by asserting its typed outcome instead (below). - User-visible CLI/MCP output (if applicable): reference-source graph relationships only; no new tool output. - Remaining proof gaps: no full CLI/MCP end-to-end import run for this spelling; evidence is the boundary workflow plus direct parser assertions. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm ci` — locked dependency installation refreshed (main's lockfile added `tsx`). - `npm run test:focused -- tests/boundary/filesystem/referenceSourceTemplateRequire.test.ts` — RED before fix (template edge missing, quoted control passing), 1/1 pass after. - `npm run test:focused -- tests/boundary/filesystem/referenceSourceTemplateRequire.test.ts tests/boundary/filesystem/referenceSourceComputedRequire.test.ts tests/boundary/filesystem/referenceSourceDynamicImports.test.ts tests/boundary/filesystem/referenceSourceShadowedRequire.test.ts tests/boundary/cli/referenceSourceDirectoryImports.test.ts tests/boundary/filesystem/referenceSourceImport.test.ts` — 6 files, 26/26 pass after updating the stale `unknown.cjs` expectation. - Direct parser probe (`tsx`, `parseReferenceSourceImports` on `fixture.cjs`): `require(\`mod\`)` returns `{to: "mod", kind: "requires", resolution: "external", parse_state: "parsed", resolutionStrategy: "runtime-require"}`; computed require returns no relationship; `import(\`mod\`)` stays `<dynamic-import>` partial; `import(\`./${name}.js\`)` stays resolution `unknown`; static ESM template import/export still report parse failures — all issue controls asserted, exit 0. - Direct TS-wrapper parser probe (`tsx`): `require(`pkg` as const)`, `require("pkg" as const)`, `require(`pkg`!)`, and `require(`pkg` satisfies string)` each return `to: "pkg"`, `parse_state: "parsed"`; `require(`a${x}c` as const)` stays silent; `import(`pkg` as const)` stays `<dynamic-import>` partial. - `npm run test:focused` after review fix (`c09ab40fd`) — 6 files, 27/27 pass (adds `.ts` + `createRequire` boundary coverage: plain, `as const`, `!`, wrapped-string all retained; computed wrapped template silent). - `npm run check:fast` — typecheck + lint, 2/2 tasks successful (rerun after review fix). - Node v24.18.0, macOS arm64. No native provider involved (synthetic source bytes). ## Compatibility, safety, and release - Breaking changes or migration steps: none intended; graphs may gain correct `requires` entries for template spellings relative to earlier releases. - Real Hopper/Ghidra, browser, or OS coverage: not applicable — source-boundary parser change, no provider or target interaction. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new I/O, execution, or authority; only parsing of already-selected source bytes. ## Author readiness Human owner: @hemv-857 - [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] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. <!-- maintainer-blocker-review --> ### Maintainer blocker verification The require-wrapper finding is already fixed at the producing parser: exactness delegates to readExactJavaScriptLiteral through moduleSpecifierFromExpression rather than an AST-kind gate. On this exact head, the focused referenceSourceTemplateRequire and referenceSourceComputedRequire suites passed 3/3, including as-const/non-null wrappers and unresolved computed-template controls. No additional source edit was needed. --- <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
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#6279
No description provided.