Skip to content

fix: resolve bare in-repo Python module imports - #2301

Open
hopstreax wants to merge 3 commits into
Graphify-Labs:v8from
hopstreax:fix/2280-python-import-resolution
Open

fix: resolve bare in-repo Python module imports#2301
hopstreax wants to merge 3 commits into
Graphify-Labs:v8from
hopstreax:fix/2280-python-import-resolution

Conversation

@hopstreax

Copy link
Copy Markdown
Contributor

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. Since build_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:

  • Resolution of bare in-repo imports to canonical file nodes.
  • External imports remaining unresolved and therefore not creating dependency edges.
  • Ambiguous bare imports (multiple files with the same module stem) remaining unresolved to prevent false-positive rewrites.
  • Mixed import syntaxes (bare, package-qualified, relative, and external) continuing to behave correctly together.
  • Existing regression coverage for scan-root-independent Python import resolution and protection against cross-language alias rewrites remains intact.

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.

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-repointgraphify/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).

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 packagesgraphify/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 filesgraphify/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 existsgraphify/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 safishamsi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hopstreax

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I've addressed the requested changes:

  • Switched bare import resolution to use _resolve_python_module_path() instead of filename-stem matching, so it now follows Python's importability rules.
  • Excluded standard library modules from this resolution using sys.stdlib_module_names.
  • Added target_file to resolved bare-import edges so incremental extraction follows the same canonicalization path as full extraction (Incremental --no-cluster extraction overwrites the full graph with the changed-file subset #2169).
  • Removed the old stem-based bare import repointing logic from _repoint_python_package_imports(), which now remains responsible only for repository-wide package alias rewriting.
  • Updated the regression test to verify that import utils resolves to the importable sibling (src/utils.py) rather than a non-importable duplicate.
  • Added a clarifying comment documenting why only single-segment imports are resolved during extraction, while dotted imports continue to be handled by _repoint_python_package_imports() to preserve repository-wide ambiguity detection.

I reran the relevant import-resolution test suite after the changes, and all tests are passing.

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python AST import edges to in-repo modules emit bare module names, silently dropping real dependencies

2 participants