fix: resolve bare in-repo Python module imports - #2301
Conversation
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR extends the Python import resolution logic in _repoint_python_package_imports (in graphify/extract.py) to handle bare module imports (e.g. import solar_allocator) by building a new index mapping module stems to canonical file nodes, and repointing dangling import edges to a matching in-repo file when exactly one candidate exists. The change guards against ambiguity by only rewiring when a single file matches the stem, and preserves the existing alias-based resolution path. The PR also adds four new tests to tests/test_src_layout_import_resolution.py covering in-repo bare-import resolution, external imports remaining unresolved, ambiguous same-stem filenames staying unresolved, and a mixed-syntax scenario combining bare, relative, package-qualified, and external imports.
Worth a look
- Bare import resolution ignores importer's own file scope, may cross-repoint —
graphify/extract.py:289· 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 — 1188 functions depend on the 205 node(s) this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 348 callers, 27 callees
Verification — 1188 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: 1068 function(s) in the blast radius were not formally verified this run
· 1 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR modifies the Python package import repointing logic in graphify/extract.py. It adds a new mechanism that indexes canonical Python file nodes by their module stem and uses this to repoint bare-module import edges (e.g. import solar_allocator) to matching in-repo file nodes, with a guard to skip resolution when multiple files share the same stem or when the candidate is the source itself. The change also adds four tests to tests/test_src_layout_import_resolution.py covering bare imports resolving to in-repo files, external imports remaining unresolved, ambiguous same-stem imports staying unresolved, and a mixed-syntax scenario combining bare, relative, package-qualified, and external imports. The surface area is confined to the _repoint_python_package_imports function and the associated test file.
Worth a look
- Bare import stem match ignores package context, repoints across unrelated packages —
graphify/extract.py:289· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Bare-import repointing can rewire legitimate external/module-shadowing imports to unrelated in-repo files —
graphify/extract.py:289· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Stem-based repoint can shadow legitimate external imports when exactly one same-stem file exists —
graphify/extract.py:291· 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 — 1188 functions depend on the 205 node(s) this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 348 callers, 27 callees
Verification — 1188 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: 1068 function(s) in the blast radius were not formally verified this run
· 1 more finding(s) on lines outside this diff (see the check run).
safishamsi
left a comment
There was a problem hiding this comment.
Thanks @hopstreax — the gap is real (bare in-repo import pkg.mod emits an unresolved bare-module target). But as written the fix fabricates wrong edges, which is the one thing we can't ship. The stem index is corpus-wide with no stdlib or importability gate, so I reproduced this end-to-end on the branch:
mypkg/logging.py # repo's own helper
mypkg/app.py # `import logging` (the stdlib)
-> mypkg_app --imports--> mypkg_logging. Under Python 3 absolute imports, import logging inside a package is the stdlib, not mypkg/logging.py; any repo with a uniquely-named logging.py/types.py/queue.py gets every stdlib import of that name rewired to it (same for from logging import ...). The external-import test (requests/numpy) doesn't catch it because there's no in-repo stem collision.
Second issue: uniqueness is judged against the current extraction batch (all_nodes), so an incremental update where only one of two same-stem files is in the batch resolves what a full scan leaves ambiguous — the #2211/#2213 incremental-parity class.
Requested changes: (a) exclude sys.stdlib_module_names; (b) gate on importability — resolve only when the candidate is a sibling of the importer's directory or top-level under the scan root (sys.path semantics), which still fixes the #2280 sibling case; and ideally stamp target_file (existence-gated, like the relative-import branch) so the #2169 remap keeps incremental parity rather than resolving off the batch. The structure and self-loop guard are otherwise good — happy to re-review once stdlib/importability are gated.
|
Thanks for the review! I've addressed the requested changes:
I reran the relevant import-resolution test suite after the changes, and all tests are passing. |
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR modifies Python bare-import handling in graphify/extract.py. In _import_python, it adds logic to resolve single-segment (non-dotted, non-stdlib) import X statements to an in-repo file path via _resolve_python_module_path, falling back to the module-name-based id when unresolved, and attaches a target_file when the resolved path is a real file. It also adds guard clauses in _repoint_python_package_imports for empty alias maps and missing edge targets. On the test side, it adds several new cases in tests/test_src_layout_import_resolution.py covering bare imports resolving to in-repo files, external imports staying unresolved, ambiguous same-stem filenames, and mixed import syntaxes in a single file.
No blocking issues surfaced. 6 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1188 functions depend on the 205 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 348 callers, 27 callees
Verification — 1188 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: 1068 function(s) in the blast radius were not formally verified this run
· 1 more finding(s) on lines outside this diff (see the check run).
Summary
Fixes #2280 by resolving bare in-repo Python module imports to their canonical file nodes during graph normalization.
Python AST extraction can emit bare module names (for example,
import solar_allocator) as import edge targets. Sincebuild_from_json()only resolves canonical node IDs, these bare targets remained unresolved, causing valid dependency edges to be silently dropped even when the corresponding Python file existed in the repository.This change extends
_repoint_python_package_imports()to perform a conservative second-pass rewrite for unresolved bare Python imports. It builds an index of canonical Python file nodes keyed by module stem and rewrites a bare import only when it uniquely identifies a single in-repo Python file. Existing package-qualified alias resolution continues to take precedence, while ambiguous matches and external imports are intentionally left unresolved to avoid introducing incorrect dependency edges. The rewrite is applied only to Python import edges, preventing cross-language rewrites.Tests
Added regression tests covering:
This approach preserves existing behavior for package-qualified imports while restoring missing dependency edges for legitimate bare in-repo Python imports without introducing ambiguity or cross-language regressions.