Repository navigation
fix(javascript): retain unbound require semantic module links - #1803
rekcilyssup wants to merge 1 commit into
Conversation
Bare require calls (expression statements, arguments, returns, nested compositions) established no binding, so collectSemanticModuleLink never visited the call itself and the application graph omitted the semantic module-to-module edge plus its source-module import count. Visit CallExpression require origins not owned by a declarator or assignment right-hand side; emit one link with null imported/local/ exported names. Shadowing, dynamic specifiers, and owned member/export positions keep their existing behavior. Fixes morluto#1793
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 622728582f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
| else if (t.isVariableDeclarator(node)) collectRequireLink(node, state); | ||
| else if (t.isAssignmentExpression(node)) collectCommonJsExport(node, state); | ||
| else if (t.isCallExpression(node)) collectUnboundRequireLink(node, state); |
There was a problem hiding this comment.
When the source uses the supported form require?.("./dependency.cjs"), Babel emits an OptionalCallExpression; the static pass already recognizes that node in javascriptStaticAnalysis.ts:204-205, but this new dispatch only accepts CallExpression, and semanticRequireOrigin rejects optional calls too. Such a literal, unshadowed require therefore still produces only the static asset reference, with no semantic imports edge and import_relationships: 0; route optional calls through the same collector and origin checks.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
|
Superseded by #1799 — upstream landed the same fix (commit 3a17882) while this PR was in CI, which is why the static-checks lane reports a duplicate declaration. The upstream approach (call-identity set via |
1 participant
Summary
Fixes #1793.
Bare
requirecalls established no binding, socollectSemanticModuleLinknever visited the call itself. The application graph kept the static asset reference but omitted the semantic module-to-module edge and reportedimport_relationships: 0for the importing module.Problem and expected behavior
Repro from #1793 (
main.cjscontaining onlyrequire("./dependency.cjs");):module_link_kind: "require"edges frommain.cjs; source-module observation reportsimport_relationships: 0.requireedge with null imported/local/exported names;import_relationships: 1.Change and scope
src/domain/javascript/javascriptSemanticProjection.ts:collectSemanticModuleLinknow also visitsCallExpressionnodes viacollectUnboundRequireLink. It walks up through member-expression bases and transparent TS/paren wrappers to find the owning root; declarator initializers and assignment right-hand sides stay owned bycollectRequireLink/collectCommonJsExport(even when their patterns yield no binding, so no duplicate links).semanticRequireOriginstill enforces lexical shadowing and literal specifiers, and member-chain requires (non-empty imported path) are skipped.Contract and boundary impact
collectSemanticModuleLinkin the JavaScript semantic projection; bare require calls now emitrequiremodule links with null imported/local/exported names.analyze-javascript-applicationnow emits a semanticimportsedge plus an incrementedimport_relationshipscount for unbound requires;module_formatcan flip fromunknowntocommonjsfor files whose only require was unbound. No evidence shape changes; the new link uses the existingmodule-linktransfer schema (all-nullable name fields already allowed).Evidence and regression coverage
Tests added or updated:
src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts: bare require emits one null-named link; call/argument/return positions each emit one; bound/destructured/export-owned positions gain no duplicate; shadowed and dynamic calls stay link-free.tests/boundary/filesystem/javascriptRequireLiteralPaths.test.ts: end-to-end CLI-shape boundary test reproducing Unbound CommonJS require calls lack semantic module edges and report zero import relationships #1793 (three-file fixture from the issue), asserting the resolved semantic edge, null names,import_relationships: 1, and complete coverage.Base reproduction or other evidence: reproduced on
12c41600with the issue's exact fixture (static asset edges present, zero semantic require edges); after the fix the same command yields one resolved semantic edge andimport_relationships: 1.User-visible CLI/MCP output (if applicable): one additional
importsedge withmodule_link_kind: "require"for previously unbound requires.Remaining proof gaps: MCP transport parity not run separately; the boundary test exercises the shared reconstruction workflow both transports use.
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
npx vitest run --project domain src/domain/javascript/— 43 files, 825 tests passednpm run check:fast— typecheck + lint passednpx vitest run --project boundary tests/boundary/filesystem/javascriptRequireLiteralPaths.test.ts tests/boundary/filesystem/javascriptPackagePrecedence.test.ts tests/boundary/filesystem/javascriptHtmlResourcePaths.test.ts tests/boundary/filesystem/referenceSourceShadowedRequire.test.ts tests/boundary/cli/javascriptSelfImportRelationships.test.ts tests/boundary/javascript/javascriptApplicationCancellation.test.ts— 5 files, 54 tests passednpx vitest run --project composition tests/composition/javascript-applications/javascriptModuleRelationships.test.ts— 3 tests passednpx vitest run --project acceptance tests/acceptance/applications/packageJsonImports.test.ts tests/acceptance/applications/packageSubpathsCli.test.ts— 4 tests passednpx vitest run --project services src/application/javascript/— 18 files, 285 tests passednpm run test:fast— 377 files, 4346 tests passed, 7 skippednode scripts/rea.mjs analyze-javascript-application /tmp/repro1793 --artifact-format directory --json— before: 0 semantic require edges,import_relationships: 0; after: 1 resolved edge,import_relationships: 1Compatibility, safety, and release
No breaking changes. Files whose only require was previously unbound gain one semantic edge and count; downstream consumers handle null-name require links (same shape as the existing side-effect ESM import link). No migration, package, platform, or security implications.