[PR #2306] [MERGED] fix(javascript): preserve CommonJS lookup and analysis provenance #5152

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2306
Author: @rohitg00
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/commonjs-loader-resolution


📝 Commits (10+)

  • 56a0aee fix(javascript): preserve CommonJS directory and ancestor lookup
  • 8b6aa7a Merge upstream main preserving CommonJS directory semantics
  • 6cddcfa chore: merge upstream CI and fixture fixes
  • 8ad1a1b chore: merge upstream parameter provenance test fix
  • 443b404 test(apple): bound cold archive fixture compilation
  • 79ba2a8 chore: merge upstream CI and provenance test fixes
  • e439438 test(release): scope preflight schema to selected jobs
  • 97ec5a5 test(ci): include cold-cache Apple fixture timeout correction
  • f04875e test(ci): merge shared release and Apple fixture fixes
  • 4af185b chore: merge upstream release preflight correction

📊 Changes

7 files changed (+397 additions, -41 deletions)

View changed files

📝 src/application/javascript/JavaScriptApplicationEvidence.ts (+6 -0)
📝 src/application/javascript/JavaScriptApplicationService.ts (+1 -20)
📝 src/application/javascript/JavaScriptArtifactPathResolution.ts (+34 -8)
📝 src/application/javascript/JavaScriptArtifactReconstruction.ts (+1 -7)
➕ tests/acceptance/applications/javascriptAnalysisLimits.test.ts (+156 -0)
➕ tests/acceptance/applications/javascriptCommonJsDirectoryLookup.test.ts (+188 -0)
📝 tests/boundary/filesystem/interfaceBuilderBundle.test.ts (+11 -6)

📄 Description

Summary

Preserve the caller's CommonJS lookup intent and requested resource limits throughout JavaScript application analysis. Static graph edges now agree with Node's loader for directory requests and dependency ancestors, and completed or interrupted analysis retains the requested limits in Evidence.

Problem and expected behavior

With both dep.js and dep/index.js present, require("./dep/.") previously selected the file after path normalization erased directory intent. Node selects dep/index.js. CommonJS analysis could also select a decoy under node_modules/node_modules, an ancestor position Node skips for this loader. ESM has a different search order and must keep it.

Separately, max_heap_mb and analysis_timeout_ms reached the analysis worker but disappeared from the Evidence parameter projection. Retained and exported results must preserve supplied settings, including partial results after timeout, while leaving omitted settings absent.

These are directly reproduced correctness defects; there is no separate tracking issue.

Change and scope

Keep directory intent at the shared path-resolution owner and use a loader-specific ancestor iterator. Apply the directory guard to ordinary file and TypeScript source candidates. Preserve existing package-main, container, confinement, and unresolved-result behavior.

Project both optional resource limits at the shared Evidence owner and forward the validated request directly through the service and partial-result path, removing redundant option reconstruction. The existing Apple fixture correction gives Swift compilation a bounded timeout while retaining all assertions.

Contract and boundary impact

  • Semantic owner and earliest changed stage: JavaScript artifact candidate selection and application Evidence parameter projection.
  • Contribution scope and maintainer decision: ordinary fixes within existing offline analysis; a new scope decision is not applicable.
  • CLI and MCP/tool catalog: input schemas unchanged; both graph projections receive corrected module paths and Evidence retains supplied limits.
  • Provider/platform compatibility: CommonJS and ESM checked independently against Node. Resource defaults and enforcement are unchanged.
  • Evidence/provenance: raw requests, omission semantics, retained parent Evidence, partial results, and authenticated bundle round trips remain preserved. Recorded limits do not independently prove enforcement.
  • Target authority and cleanup: the existing owned static-analysis worker is used; analyzed application source is not executed. No new network, mutation, or cleanup behavior.
  • Generated metadata, package, installation impact: none.

Expected effects:

  • Read existing files or captured data.
  • Launch or attach to a process.

Process launch is limited to the existing analysis worker. Native Node resolution controls and Swift fixture generation are tests against owned fixtures.

Evidence and regression coverage

Four compiled CLI/stdio MCP cases compare both graph projections with native Node resolution. They cover trailing slash/dot/double-dot requests, scoped and nested dependencies, missing directories, nearest dependency precedence, package-main behavior, a TypeScript decoy, and the differing ESM target.

Six public provenance cases cover both limits, either limit, omitted limits despite ambient NODE_OPTIONS, retained parent Evidence, export/import, and an actual one-millisecond timeout followed by a successful MCP ping. Existing memory-pressure and cancellation regressions also pass.

Representative output is resolved_path: "dep/index.js" for ./dep/.. An analysis requested with both controls retains {"format":"auto","integrity_policy":"fail","max_heap_mb":128,"analysis_timeout_ms":30000} in Evidence. The timeout case retains analysis_timeout_ms: 1 in partial Evidence.

The original CommonJS public regressions and requested-limit projection regression failed before their respective fixes.

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, provenance, and failed attempts remain preserved.
  • Incomplete, unavailable, and uncertain outcomes remain visible.

Validation performed

Pinned Node 24.18.0/npm 11.16.0 on macOS arm64, against upstream fb28ec1:

  • npm run test:focused -- src/application/javascript/JavaScriptArtifactPathResolution.test.ts tests/acceptance/applications/javascriptCommonJsDirectoryLookup.test.ts tests/acceptance/applications/javascriptAnalysisLimits.test.ts tests/boundary/mcp/javascriptAnalysisResources.test.ts tests/boundary/mcp/javascriptAnalysisCancellation.test.ts tests/boundary/filesystem/interfaceBuilderBundle.test.ts: build passed; 190 tests passed across six files, with one explicit Xcode ibtool prerequisite skip.
  • npm run check:ci: all eight tasks and architecture guards passed; duplication 2.98%.
  • npm run docs:check: passed, including two README consistency tests.
  • npm run verify:test-discovery: all 1,058 files discovered exactly once.
  • git diff --check: passed.

Compatibility, safety, and release

No breaking changes, migration, dependencies, or installation changes. Actual compiled CLI, stdio MCP, isolated Node worker, bundle persistence, and Swift Foundation fixtures ran locally. The Xcode-compiled storyboard case was unavailable because ibtool was not configured. Windows and Bun resource behavior were not exercised locally. Local verification does not establish hosted CI success.

Before merge

  • Concrete problem, focused outcome, and issue-link rationale stated.
  • Scope and authority resolved.
  • Tests cover public behavior, omission, partial failure, and persistence.
  • Contracts and generated metadata reviewed; no updates required.
  • Representative output and remaining platform limits described.
  • Final diff reviewed for secrets, unrelated changes, 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/2306 **Author:** [@rohitg00](https://github.com/rohitg00) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/commonjs-loader-resolution` --- ### 📝 Commits (10+) - [`56a0aee`](https://github.com/morluto/rea/commit/56a0aee78a1aee45683674e819264107fe7bae11) fix(javascript): preserve CommonJS directory and ancestor lookup - [`8b6aa7a`](https://github.com/morluto/rea/commit/8b6aa7a91d1b12f6579d7b16b960a00373ae2bbf) Merge upstream main preserving CommonJS directory semantics - [`6cddcfa`](https://github.com/morluto/rea/commit/6cddcfa983a2325790e9e7a5c73b6760eda88fa4) chore: merge upstream CI and fixture fixes - [`8ad1a1b`](https://github.com/morluto/rea/commit/8ad1a1b0171ece72538091c7274a5d2f573fe216) 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 - [`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 - [`f04875e`](https://github.com/morluto/rea/commit/f04875e7acc9ad8eb6fbb66f25875054039e1a68) test(ci): merge shared release and Apple fixture fixes - [`4af185b`](https://github.com/morluto/rea/commit/4af185b49f261491fa40dd5a0f8f977950f8b8ed) chore: merge upstream release preflight correction ### 📊 Changes **7 files changed** (+397 additions, -41 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptApplicationEvidence.ts` (+6 -0) 📝 `src/application/javascript/JavaScriptApplicationService.ts` (+1 -20) 📝 `src/application/javascript/JavaScriptArtifactPathResolution.ts` (+34 -8) 📝 `src/application/javascript/JavaScriptArtifactReconstruction.ts` (+1 -7) ➕ `tests/acceptance/applications/javascriptAnalysisLimits.test.ts` (+156 -0) ➕ `tests/acceptance/applications/javascriptCommonJsDirectoryLookup.test.ts` (+188 -0) 📝 `tests/boundary/filesystem/interfaceBuilderBundle.test.ts` (+11 -6) </details> ### 📄 Description ## Summary Preserve the caller's CommonJS lookup intent and requested resource limits throughout JavaScript application analysis. Static graph edges now agree with Node's loader for directory requests and dependency ancestors, and completed or interrupted analysis retains the requested limits in Evidence. ## Problem and expected behavior With both `dep.js` and `dep/index.js` present, `require("./dep/.")` previously selected the file after path normalization erased directory intent. Node selects `dep/index.js`. CommonJS analysis could also select a decoy under `node_modules/node_modules`, an ancestor position Node skips for this loader. ESM has a different search order and must keep it. Separately, `max_heap_mb` and `analysis_timeout_ms` reached the analysis worker but disappeared from the Evidence parameter projection. Retained and exported results must preserve supplied settings, including partial results after timeout, while leaving omitted settings absent. These are directly reproduced correctness defects; there is no separate tracking issue. ## Change and scope Keep directory intent at the shared path-resolution owner and use a loader-specific ancestor iterator. Apply the directory guard to ordinary file and TypeScript source candidates. Preserve existing package-main, container, confinement, and unresolved-result behavior. Project both optional resource limits at the shared Evidence owner and forward the validated request directly through the service and partial-result path, removing redundant option reconstruction. The existing Apple fixture correction gives Swift compilation a bounded timeout while retaining all assertions. ## Contract and boundary impact - Semantic owner and earliest changed stage: JavaScript artifact candidate selection and application Evidence parameter projection. - Contribution scope and maintainer decision: ordinary fixes within existing offline analysis; a new scope decision is not applicable. - CLI and MCP/tool catalog: input schemas unchanged; both graph projections receive corrected module paths and Evidence retains supplied limits. - Provider/platform compatibility: CommonJS and ESM checked independently against Node. Resource defaults and enforcement are unchanged. - Evidence/provenance: raw requests, omission semantics, retained parent Evidence, partial results, and authenticated bundle round trips remain preserved. Recorded limits do not independently prove enforcement. - Target authority and cleanup: the existing owned static-analysis worker is used; analyzed application source is not executed. No new network, mutation, or cleanup behavior. - Generated metadata, package, installation impact: none. Expected effects: - [x] Read existing files or captured data. - [x] Launch or attach to a process. Process launch is limited to the existing analysis worker. Native Node resolution controls and Swift fixture generation are tests against owned fixtures. ## Evidence and regression coverage Four compiled CLI/stdio MCP cases compare both graph projections with native Node resolution. They cover trailing slash/dot/double-dot requests, scoped and nested dependencies, missing directories, nearest dependency precedence, package-main behavior, a TypeScript decoy, and the differing ESM target. Six public provenance cases cover both limits, either limit, omitted limits despite ambient `NODE_OPTIONS`, retained parent Evidence, export/import, and an actual one-millisecond timeout followed by a successful MCP ping. Existing memory-pressure and cancellation regressions also pass. Representative output is `resolved_path: "dep/index.js"` for `./dep/.`. An analysis requested with both controls retains `{"format":"auto","integrity_policy":"fail","max_heap_mb":128,"analysis_timeout_ms":30000}` in Evidence. The timeout case retains `analysis_timeout_ms: 1` in partial Evidence. The original CommonJS public regressions and requested-limit projection regression failed before their respective fixes. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, provenance, and failed attempts remain preserved. - [x] Incomplete, unavailable, and uncertain outcomes remain visible. ## Validation performed Pinned Node 24.18.0/npm 11.16.0 on macOS arm64, against upstream `fb28ec1`: - `npm run test:focused -- src/application/javascript/JavaScriptArtifactPathResolution.test.ts tests/acceptance/applications/javascriptCommonJsDirectoryLookup.test.ts tests/acceptance/applications/javascriptAnalysisLimits.test.ts tests/boundary/mcp/javascriptAnalysisResources.test.ts tests/boundary/mcp/javascriptAnalysisCancellation.test.ts tests/boundary/filesystem/interfaceBuilderBundle.test.ts`: build passed; 190 tests passed across six files, with one explicit Xcode `ibtool` prerequisite skip. - `npm run check:ci`: all eight tasks and architecture guards passed; duplication 2.98%. - `npm run docs:check`: passed, including two README consistency tests. - `npm run verify:test-discovery`: all 1,058 files discovered exactly once. - `git diff --check`: passed. ## Compatibility, safety, and release No breaking changes, migration, dependencies, or installation changes. Actual compiled CLI, stdio MCP, isolated Node worker, bundle persistence, and Swift Foundation fixtures ran locally. The Xcode-compiled storyboard case was unavailable because `ibtool` was not configured. Windows and Bun resource behavior were not exercised locally. Local verification does not establish hosted CI success. ## Before merge - [x] Concrete problem, focused outcome, and issue-link rationale stated. - [x] Scope and authority resolved. - [x] Tests cover public behavior, omission, partial failure, and persistence. - [x] Contracts and generated metadata reviewed; no updates required. - [x] Representative output and remaining platform limits described. - [x] Final diff reviewed for secrets, unrelated changes, 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:04:23 +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#5152
No description provided.