fix(python): resolve src-layout imports when the scan root is a symlink (closes #3446) - #3447
fix(python): resolve src-layout imports when the scan root is a symlink (closes #3446)#3447burgetjp wants to merge 1 commit into
Conversation
`extract()` resolves `root` (extract.py), but the caller's paths keep whatever
spelling they were collected with — and `collect_files(symlinked_root)` returns
symlinked paths. `_resolve_python_module_path` then compares a resolved root
against an unresolved `current_path`: `anc.relative_to(root)` raises on the very
first ancestor, the upward walk breaks immediately, and the absolute import
resolves to nothing.
The result is a silently smaller graph for the same corpus. Identical files, one
reached by its real path and one through a symlink:
/private/tmp/ctor -> 11 nodes, 20 edges
/tmp/ctor -> 11 nodes, 15 edges (/tmp is a symlink on macOS)
The node count is unchanged, and node ids stay clean and repo-relative in both
runs, so the usual "absolute paths leaked into the ids" symptom never appears.
What goes missing are the `calls` edges for constructor calls on classes imported
package-root-relative (`from app.config import Settings`, package at `svc/app/`);
only a generic `uses` edge survives, and the callee reads downstream as dead code.
An import spelled from the scan root keeps its `calls` edge either way, so the
symlink and the src-layout spelling have to combine to trigger it.
macOS `/tmp` -> `/private/tmp` is the easiest way to hit this, but a symlinked
workspace or home and bind/overlay mounts in CI qualify too.
Re-derive the scan root as the CALLER spelled it by walking up from the importing
file to the ancestor whose resolved form matches `root`, then probe with that, so
every path this function returns keeps one spelling consistent with the ids built
elsewhere. Containment now accepts either spelling.
Deliberately NOT fixed by normalizing `extract()`'s inputs: resolving the incoming
paths does fix this, but it breaks
`test_rebuild_code_incremental_rename_preserves_symlink_source_path` along with
the Kotlin and JS resolution suites — preserving the caller's path spelling in
`source_file` is existing contract. The fix is kept inside the comparison.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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. No changes could be formally verified in this run.
Graphify review — findings
Fixes src-layout Python import resolution when the scan root is reached through a symlink (macOS /tmp, symlinked workspaces, CI bind mounts): _resolve_python_module_path now re-derives the root in the caller's own path spelling and compares ancestors with symlinks resolved via the new _is_within and _resolved_or_self, so the upward walk no longer breaks on the first ancestor and drops calls edges. _resolved_or_self leaves a path untouched when it can't be resolved, so a broken symlink or unreadable parent doesn't abort resolution. Adds a test that extracts one corpus through both a real and a symlinked path and asserts identical nodes and edges, preserved constructor-call edges, and repo-relative (not absolute) node IDs.
No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1826 functions depend on the 149 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 565 callers, 43 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 — 1826 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: 787 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
30 of 263 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_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_symlinked_scan_root.py— impact, changed-testtests/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 \_resolve\_python\_module\_path.
The verifier did not have enough to check \_resolve\_python\_module\_path, 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 `current_path` is annotated `Path` — outside the synthesizable primitive/collection set
· 1 grounded finding(s) anchored inline below; 43 more finding(s) on lines outside this diff (see the check run).
| return False | ||
|
|
||
|
|
||
| def _resolve_python_module_path(module_name: str, current_path: Path, root: Path, level: int) -> Path | None: |
There was a problem hiding this comment.
_resolve_python_module_path()
7 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
1 participant
Fixes #3446.
The bug
extract()resolvesroot, but the caller's paths keep whatever spelling they were collected with — andcollect_files(symlinked_root)returns symlinked paths._resolve_python_module_paththen compares a resolved root against an unresolvedcurrent_path:The upward walk breaks immediately and the absolute import resolves to nothing.
The symptom
Identical files, one corpus, two spellings of the same directory:
/private/tmp/ctor(real path)/tmp/ctor(symlink, macOS)The node count is unchanged and node ids stay clean and repo-relative in both runs, so the familiar "absolute paths leaked into the ids" symptom never shows up. What disappears are the
callsedges for constructor calls on classes imported package-root-relative (from app.config import Settings, package atsvc/app/) — only a genericusesedge survives, and the callee reads downstream as genuine dead code.An import spelled from the scan root (
from svc.app.config import Settings) keeps itscallsedge either way, so the symlink and the src-layout spelling have to combine to trigger it.macOS
/tmp→/private/tmpis the easiest way to hit this, but a symlinked workspace or home directory and bind/overlay mounts in CI qualify too — and CI is exactly where a silently-smaller graph goes unnoticed.The fix
Re-derive the scan root as the caller spelled it: walk up from the importing file to the ancestor whose resolved form matches
root, then probe with that. Every path the function returns then keeps one spelling, consistent with the ids built elsewhere. Containment accepts either spelling.What I deliberately did not do
Normalizing
extract()'s inputs (paths = [p.resolve() for p in paths]) also fixes it, and was my first attempt. It breakstest_rebuild_code_incremental_rename_preserves_symlink_source_pathplus the Kotlin object-literal and JS import-resolution suites — preserving the caller's path spelling insource_fileis existing contract. So the fix stays inside the comparison rather than rewriting the inputs.Verification
tests/test_symlinked_scan_root.py— 3 tests, red before / green after. The symlink is created inside pytest'stmp_path, so this is symlinks generally, not a macOS/tmpquirk, and it skips cleanly where symlink creation is not permitted. One test guards the fix's shape by asserting ids stay repo-relative.test_ollama_retry_cap(4) andtest_ts_normalizer_scales_linearly_on_large_files(a wall-clock assertion that passes in isolation), all of which fail on pristinev8too. No new failures.test_watch.py,test_kotlin_object_literal.py,test_js_import_resolution.py,test_python_import_resolution.py,test_src_layout_import_resolution.py,test_symbol_resolution.py,test_skillgen.py: 333 passed.ruff checkclean;pyright43 errors before and after.Note on the test itself
Both spellings name one physical directory, so they also share one
graphify-out/cache. The test clears it between runs — otherwise the second extraction replays entries the first keyed under the other spelling, and you end up measuring cache reuse instead of resolution. That shared-cache interaction may be worth a look separately; it is not touched here.Independent of #3445 (both branch from
v8); the two do not overlap.