fix(js): keep import edges to declared npm packages - #3092
fix(js): keep import edges to declared npm packages#3092rohit-jsfreaky wants to merge 2 commits into
Conversation
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Marks regex-rescued bare/scoped JS import stubs as type=module module anchors so they collapse onto a manifest-declared package node of the same id instead of being salted apart by id-disambiguation, which previously stranded the importing edge on a dead id and dropped it at build. _resolve_rescued_specifier now returns an is_external flag to distinguish these package specifiers from relative/aliased paths, which stay subject to disambiguation. Adds tests covering declared, undeclared, and unresolved-relative cases.
Worth a look
- External package imports are anchored to the path leaf, not the package name —
graphify/extract.py:1538· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1669 functions depend on the 244 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 501 callers, 42 callees - new:
_rebuild_code()— 98 callers, 50 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
extract_js()— 83 callers, 3 callees - new:
dispatch_command()— 2 callers, 122 callees - new:
_get_extractor()— 26 callers, 6 callees - new:
run_pipeline()— 8 callers, 13 callees - new:
collect_files()— 17 callers, 6 callees - …and 23 more — each is listed as a finding
Verification — 1669 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1518 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_emit\_rescued\_import.
The verifier did not have enough to check \_emit\_rescued\_import, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_rescue\_js\_dynamic\_imports.
The verifier did not have enough to check \_rescue\_js\_dynamic\_imports, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_resolve\_rescued\_specifier.
The verifier did not have enough to check \_resolve\_rescued\_specifier, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
· 31 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Anchors regex-rescued bare/scoped JS import specifiers to the package name rather than the path leaf, so lodash/fp/map and lodash/debounce collapse to one lodash node and @a/utils vs @b/utils stay distinct. Marks the resulting external stubs as type=module (like Swift module anchors) so they merge with a matching package.json declaration instead of being salted apart by id-disambiguation, which previously stranded the import edge on a dead id and dropped a declared dependency's inbound edges at build. Relative and unresolved-path specifiers are left untouched, so they remain subject to disambiguation.
Worth a look
- URL dynamic imports collapse to the scheme —
graphify/extract.py:1541· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1673 functions depend on the 248 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 503 callers, 42 callees - new:
_rebuild_code()— 98 callers, 50 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
extract_js()— 83 callers, 3 callees - new:
dispatch_command()— 2 callers, 122 callees - new:
_get_extractor()— 26 callers, 6 callees - new:
run_pipeline()— 8 callers, 13 callees - new:
collect_files()— 17 callers, 6 callees - …and 23 more — each is listed as a finding
Verification — 1673 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1522 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_emit\_rescued\_import.
The verifier did not have enough to check \_emit\_rescued\_import, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_rescue\_js\_dynamic\_imports.
The verifier did not have enough to check \_rescue\_js\_dynamic\_imports, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_resolve\_rescued\_specifier.
The verifier did not have enough to check \_resolve\_rescued\_specifier, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
· 31 more finding(s) on lines outside this diff (see the check run).
1 participant
Fixes #3084 (the dynamic-import half — see the closing note on the static half).
Summary
A bare specifier mints an external stub whose id is the package name. When a manifest in the same corpus declares that package, it produces a node under that same name. Two nodes, one id, different
source_file— so_disambiguate_colliding_node_idssalts both apart (postgresbecomespostgres_postgresandpackage_json_postgres).The importing edge carries neither salt's source key: its own
source_fileis the importer (app.mts), andunambiguous_remapsdeliberately excludes ids that were ambiguous. So the edge is left on the now-deadpostgresid and dropped at build. The pass already documents this exact shape for C headers — "a cross-file import edge from a THIRD file carries neither salt's source_key, so the edge dangles on the now deadfooid" (#1475) — this is the same failure for npm packages.The result is backwards, as the report says: an undeclared package keeps its edge (nothing to collide with), while a properly declared dependency becomes invisible, and "which files use this package?" answers nothing with no warning anywhere.
Reproduction
The issue's repro, on
v8(0.9.50):package.jsonFix
Mark the external stub the way Swift module anchors are marked (#1327). That comment already describes this case:
An npm package is a module rather than a path; the same package imported from N files is one node; and it is the same package the manifest declares. So the stub now collapses onto that declaration instead of being salted away from it.
Scoped to bare/scoped specifiers only, via an explicit
is_externalflag threaded out of_resolve_rescued_specifier. An unresolved relative specifier names a path, not a module, and must stay subject to disambiguation — two./helperstubs in different directories are different files and must not collapse. There is a test pinning that.No id construction is touched, so #1638's ref-namespacing and #2457's portable target ids both stand unchanged.
Tests
Three added to
tests/test_js_dynamic_imports.py:test_declared_npm_dependency_keeps_its_import_edge— the repro; confirmed to fail without this changetest_undeclared_npm_dependency_still_keeps_its_edge— guard: the no-manifest path must keep workingtest_unresolved_relative_import_is_not_marked_a_module— guard: a relative specifier must NOT become a module anchor, or two same-named stubs in different directories would collapse onto one nodeuv run --no-sync pytest tests/test_js_import_resolution.py tests/test_ts_import_require.py tests/test_phantom_external_import.py -q— 72 passed.uv run --no-sync pytest tests/test_js_dynamic_imports.py tests/test_js_dynamic_import_affected.py tests/test_astro_extraction.py tests/test_astro_import_ids.py tests/test_vue_extraction.py -q— 44 passed.uv run --no-sync ruff check graphify/extract.py— passed.Full suite: 12 failed, 5035 passed, 43 skipped — the same 12 as a clean
v8checkout on this Windows machine (fifo / unix-socket / hermes / gemini install paths / watch), none related to this change.The static-import half, deliberately not changed
The report's second case (
import x from 'pkg'targetingref_pkg) is left alone. That target is ref-namespaced on purpose, and_resolve_js_import_targetstates the intent:The namespacing is also what guards #1638 (a bare
tailwindcss/colors→colorsid colliding with an unrelated localcolors.tsand producing a confident phantom edge). Materializing a node there, or dropping the prefix, would contradict that decision and add a node for every external static import in every JS repo — a design call rather than a bug fix, so it is yours to make. Worth noting the two paths do disagree today: the rescue path materializes an external stub while the AST path does not. Happy to follow up in whichever direction you prefer.