Repository navigation
fix(javascript): preserve CommonJS lookup and analysis provenance - #2306
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
TL;DR: With this PR, REA picks the same file as Node 22, 24 and 26 for ResultsI checked 135 requests against real Node 22, 24 and 26, which all agreed. REA ran on the commit this PR sits on (aeee507) and on the PR head (56a0aee).
Still different, before and afterIf Not run
Thanks, rohitg00, for the fix, and for tests that check the answers against real Node. Counts by group
All 31 changed results
The 3 results that differ from Node on both trees
Test setup
CommandsHow I got the Node answers. CommonJS requests use node <tree>/scripts/rea.mjs analyze-javascript-application <fixture> --format json
env -C <head tree> npm run test:focused -- tests/acceptance/applications/javascriptCommonJsDirectoryLookup.test.tsBoth trees came from 🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊 |
|
@greptileai review |
|
This PR is set for a Plus review. TREX isn't supported with Plus or Apex reviews yet, so Greptile reviews this PR at Base while TREX is on. |
|
|
@morluto Would like you to consider this for new release as well. |
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.jsanddep/index.jspresent,require("./dep/.")previously selected the file after path normalization erased directory intent. Node selectsdep/index.js. CommonJS analysis could also select a decoy undernode_modules/node_modules, an ancestor position Node skips for this loader. ESM has a different search order and must keep it.Separately,
max_heap_mbandanalysis_timeout_msreached 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
Expected effects:
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 retainsanalysis_timeout_ms: 1in partial Evidence.The original CommonJS public regressions and requested-limit projection regression failed before their respective fixes.
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 Xcodeibtoolprerequisite 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
ibtoolwas not configured. Windows and Bun resource behavior were not exercised locally. Local verification does not establish hosted CI success.Before merge