fix(imports): pick workspace exports by importer platform, runtime before types (#3487) - #3489
Conversation
…fore types The workspace `exports` resolver reduced a condition object to a single target, so a package that splits its runtime source from its declarations never produced a cross-package edge. Reported in Graphify-Labs#3487; the exports map itself is present and works, the defect is in how one is chosen. `_EXPORT_CONDITION_PRIORITY` listed `types` ahead of `require` and `default`. `types` is declaration-only and normally points into `dist/`, which is build output and not part of the corpus, so the resolver returned a path that is not on disk and stopped there — `_package_entry_candidates` returned `[candidate]` as soon as any condition matched, so the `default` source beside it was never tried and the import became a dangling `ref_*` external. That is not the order the map was added with. Graphify-Labs#1308 specifies `source -> import -> module -> default -> require -> types`, and the commit that implemented it says `default` is consulted LAST so a matching `import`/`module`/`svelte` condition wins; the tuple shipped with `types` in the middle instead. Three changes, all on the workspace `exports` path: - `types` moves to the end of `_EXPORT_CONDITION_PRIORITY`, restoring the specified order. `require`/`default` keep their relative order, so the behaviour pinned by `test_workspace_subpath_export_default_consulted_last` is unchanged. - `_resolve_export_targets` keeps every condition target in preference order instead of returning the first, so a target that is not on disk falls through to the next condition rather than taking the import down with it. - `react-native` is honoured only for an importer that is itself native, and each target is tried alongside its platform-suffixed siblings (`Icon.tsx` -> `Icon.web.tsx` / `Icon.native.tsx`), which is how a bundler reaches a platform split whose `exports` target names the unsuffixed path. Importer platform comes from whole path segments, so `apps/mobile/src` is native while `packages/web-utils` is not web. `_resolve_export_target` keeps its signature and its semantics for the `package.json` `imports` path, and platform expansion applies only to the workspace `exports` path, so relative imports elsewhere are untouched. Behaviour note for review: when every `exports` target escapes the package directory, resolution still falls through to the bare-path fallback exactly as before. The first revision of this change swallowed that fallback, which the containment guard made reachable, and a test now pins it. The `"."` branch additionally gains the containment guard its subpath sibling already had; that is the only change here beyond the reported defect, and such a package previously resolved outside its own directory. Verified: same 29 pre-existing failures before and after, byte-identical failure sets (`diff` clean; unrelated to this path — non-regular-file, watch and uninstall-scope tests on Windows). Passing goes 5500 -> 5507, the seven tests added here. The fixture from Graphify-Labs#3487 resolves its four previously-dropped imports and leaves the three control cases unchanged; focused run 109 passed; `ruff check` clean; all five `tools.skillgen` CI-parity checks OK.
![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.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Reorders JS exports-map condition priority so types (which points into unbuilt dist/) is tried last, and has _resolve_export_targets keep every matching condition in preference order so an import resolves to whichever target is real source on disk instead of dropping the edge on the first missing one. Adds per-importer platform awareness: _importer_platform infers a platform from the importing directory's path segments, _platform_variants expands exports targets into platform-suffixed siblings (.web.tsx/.native.tsx) with the importer's own platform first, and react-native conditions are honoured only for native importers. Ambiguous importers fall back to a fixed platform order so resolution stays deterministic rather than producing no edge.
No blocking issues surfaced. 6 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1875 functions depend on the 234 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 585 callers, 44 callees - new:
_rebuild_code()— 115 callers, 51 callees - new:
_extract_generic()— 18 callers, 26 callees - new:
extract_js()— 85 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
_resolve_js_module_path()— 34 callers, 9 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
extract_objc()— 27 callers, 9 callees - …and 36 more — each is listed as a finding
Verification — 1875 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: 876 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
30 of 268 test file(s) selected (11%) via static blast radius.
tests/test_astro_extraction.py— impacttests/test_build.py— impacttests/test_cjs_module_extension.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_dotnet.py— impacttests/test_extract.py— impacttests/test_forwarding_review_findings.py— impacttests/test_import_extension_resolution.py— impacttests/test_indirect_dispatch.py— impacttests/test_indirect_dispatch_assign_return.py— impacttests/test_indirect_dispatch_getattr.py— impacttests/test_js_exported_scalar_bindings.py— impacttests/test_js_import_resolution.py— impact, changed-testtests/test_languages.py— impacttests/test_multilang.py— impacttests/test_package_json_subpath_imports.py— impacttests/test_pascal.py— impacttests/test_pascal_resolution.py— impacttests/test_phantom_external_import.py— impacttests/test_python_import_resolution.py— impacttests/test_python_underscore_resolution.py— impacttests/test_rationale.py— impacttests/test_ruby_resolution.py— impacttests/test_scala_self_type.py— impacttests/test_src_layout_import_resolution.py— impacttests/test_swift_computed_properties.py— impacttests/test_ts_new_expression_calls.py— impacttests/test_typescript_module_extensions.py— impacttests/test_unmapped_at_alias_resolution.py— impacttests/test_vue_extraction.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify \_package\_entry\_candidates.
The verifier did not have enough to check \_package\_entry\_candidates, 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 `package_dir` is annotated `Path` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_resolve\_export\_target (not a proof).
The verifier ran both versions of \_resolve\_export\_target on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_resolve\_workspace\_import.
The verifier did not have enough to check \_resolve\_workspace\_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 `start_dir` is annotated `Path` — outside the synthesizable primitive/collection set
· 44 more finding(s) on lines outside this diff (see the check run).
9a1978e to
c704a5b
Compare
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Reorders JavaScript exports-map condition priority so types (which points into unbuilt dist/) loses to real runtime targets like require/default, and makes _resolve_export_targets return every matching target in preference order so an import falls through to the next condition when a target isn't on disk instead of dropping the edge entirely. Adds per-importer platform selection: _importer_platform infers a platform from whole path segments of the importer's directory, _platform_variants expands an exports target to platform-suffixed siblings (importer's platform first, deterministic order otherwise), and native-only conditions like react-native are honoured only for native importers. Preserves the existing _contained_in_package guard against targets escaping the package directory when generating candidates.
No blocking issues surfaced. 7 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1875 functions depend on the 234 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 585 callers, 44 callees - new:
_rebuild_code()— 115 callers, 51 callees - new:
_extract_generic()— 18 callers, 26 callees - new:
extract_js()— 85 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
_resolve_js_module_path()— 34 callers, 9 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
extract_objc()— 27 callers, 9 callees - …and 36 more — each is listed as a finding
Verification — 1875 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: 876 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
30 of 268 test file(s) selected (11%) via static blast radius.
tests/test_astro_extraction.py— impacttests/test_build.py— impacttests/test_cjs_module_extension.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_dotnet.py— impacttests/test_extract.py— impacttests/test_forwarding_review_findings.py— impacttests/test_import_extension_resolution.py— impacttests/test_indirect_dispatch.py— impacttests/test_indirect_dispatch_assign_return.py— impacttests/test_indirect_dispatch_getattr.py— impacttests/test_js_exported_scalar_bindings.py— impacttests/test_js_import_resolution.py— impact, changed-testtests/test_languages.py— impacttests/test_multilang.py— impacttests/test_package_json_subpath_imports.py— impacttests/test_pascal.py— impacttests/test_pascal_resolution.py— impacttests/test_phantom_external_import.py— impacttests/test_python_import_resolution.py— impacttests/test_python_underscore_resolution.py— impacttests/test_rationale.py— impacttests/test_ruby_resolution.py— impacttests/test_scala_self_type.py— impacttests/test_src_layout_import_resolution.py— impacttests/test_swift_computed_properties.py— impacttests/test_ts_new_expression_calls.py— impacttests/test_typescript_module_extensions.py— impacttests/test_unmapped_at_alias_resolution.py— impacttests/test_vue_extraction.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify \_package\_entry\_candidates.
The verifier did not have enough to check \_package\_entry\_candidates, 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 `package_dir` is annotated `Path` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_resolve\_export\_target (not a proof).
The verifier ran both versions of \_resolve\_export\_target on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_resolve\_workspace\_import.
The verifier did not have enough to check \_resolve\_workspace\_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 `start_dir` is annotated `Path` — outside the synthesizable primitive/collection set
· 44 more finding(s) on lines outside this diff (see the check run).
1 participant
Fixes #3487.
What actually goes wrong
The
exportsmap is consulted — #1308 added that, ine8dabad. The defect is in how a condition object is reduced to one target._EXPORT_CONDITION_PRIORITYshipped as:typesis a declaration-only condition and normally points intodist/, which is build output and not in the corpus. So forthe resolver returned the
distpath, found nothing on disk, and stopped. There was no fall-through, because_package_entry_candidatesreturned[candidate]as soon as any condition matched — so thedefaultsource sitting right beside it was never tried, and the import became a danglingref_*external.That is not the order the map was added with. The commit that introduced it says "
defaultis consulted LAST … so a matchingimport/module/sveltecondition wins", and #1308 specifiessource -> import -> module -> default -> require -> types. The tuple puttypesahead ofrequireanddefaultinstead.Two more gaps in the same path:
react-nativewas never consulted, and no platform-suffixed sibling was ever tried, so"./controls/*": {"default": "./src/controls/*.tsx"}could not reach the realBottomNav.web.tsx/BottomNav.native.tsx.The fix
typesmoves to the end of_EXPORT_CONDITION_PRIORITY, restoring the order TypeScript workspace imports do not resolve package subpath exports #1308 specified. A runtime target always beats a declaration.require/defaultkeep their relative order, so the behaviour pinned bytest_workspace_subpath_export_default_consulted_lastis unchanged._resolve_export_targetskeeps every condition target in preference order rather than returning the first, so a target that is not on disk falls through to the next condition instead of taking the import down with it.react-nativeis honoured only for a native importer, so a web importer resolves the same specifier throughdefaultrather than being attributed a native file it never imports. This is a custom condition, so it is per-importer by nature.Icon.tsx→Icon.web.tsx/Icon.native.tsx), which is how a bundler reaches a platform split whoseexportstarget names the unsuffixed path._resolve_export_targetkeeps its signature and semantics for thepackage.jsonimportspath; platform expansion applies only to the workspaceexportspath, so relative imports elsewhere are untouched.Behaviour notes for review
exportstarget escapes the package directory, resolution still falls through to the bare-path fallback exactly as before. A new test pins this — my first revision accidentally swallowed that fallback, and the containment guard made it reachable."."branch now applies the same containment guard the subpath branch already applied. This is the only change beyond the reported defect, and it is a consistency fix: such a package previously resolved outside its own directory.apps/mobile/src→ native;packages/web-utils→ none, since matching is segment-wide, not substring). It is best-effort by design: when nothing distinguishes the importer every platform keeps the same priority, so resolution stays deterministic rather than dropping the edge.Verified
Measured through
_resolve_js_module_pathon the fixture from the issue, before and after:@acme/ui/components/Icon({types, react-native, default})Icon.web.tsxIcon.native.tsx@acme/ui2/controls/BottomNav(wildcard, target absent)BottomNav.web.tsxBottomNav.native.tsx@acme/lib/util(exact subpath, target exists)@acme/root(bare root,"."exists)@acme/nodot(exports has no".")The controls were already correct before the change, which localises the defect to condition selection and fall-through rather than to a missing resolution path.
Seven tests added to
tests/test_js_import_resolution.py, one per behaviour above plus locks on the ordering and the containment fall-through. Focused run:tests/test_js_import_resolution.py+tests/test_import_extension_resolution.py→ 109 passed (PR #3488's tests still pass unchanged).ruff checkclean.tools.skillgenCI-parity checks all OK.Full suite, same commands CI runs:
Byte-identical failure sets —
diffof the sortedFAILEDlines is empty. The 29 are pre-existing and unrelated to this path (non-regular-file, watch and uninstall-scope tests on Windows). The +7 passing are the tests added here.graphify --helpandgraphify installboth exit 0.Note on #3487's reporter
@cbartens reports in the issue that they have this working and offered to contribute it. I could not tell from the thread whether maintainers want that, so I am putting this up as a concrete patch — happy to close it in favour of theirs, or to have it serve as a reference implementation. No maintainer has replied on the issue, and nothing here is claimed or assigned.