fix(extract): canonicalize out-of-root import/include edge targets (#2243) - #2250
Open
Yyunozor wants to merge 1 commit into
Open
fix(extract): canonicalize out-of-root import/include edge targets (#2243)#2250Yyunozor wants to merge 1 commit into
Yyunozor wants to merge 1 commit into
Conversation
…raphify-Labs#2243) Follow-up to Graphify-Labs#1899. That fix taught the relativization pass to catch a NODE whose id was minted from an absolute out-of-root path and give it a portable "ext_"-namespaced id, by matching the node's own id against _make_id(str(its source_file)). But several cross-file resolvers (Python relative imports, C/C++/ObjC quoted #include) only ever emit an EDGE for an import target, no node -- so when that target lives outside the scan root, the belt-and-braces pass has nothing to learn the old->new id from, and the edge keeps the raw _make_id(str(absolute_path)) slug forever. The scan path, including the OS username, ends up in links[].source/target, and differs between machines/checkouts even though the node id sets are identical. _import_c also never stamped the transient `target_file` hint (Graphify-Labs#1814/Graphify-Labs#2169) its Python/JS siblings already use for exactly this kind of cross-file target canonicalization, so it could not benefit from that machinery either. Fix, in the two places this root cause actually lives: - _import_c now stamps target_file on a resolved #include, mirroring _import_python/_import_js. - The id_remap pass that already walks target_file-stamped edges to canonicalize in-root-but-unscanned targets now also handles the out-of-root branch it previously skipped ("leave its ids alone"): an existing out-of-root target gets the same portable ext_-namespaced id an out-of-root NODE already gets, so an edge with no node of its own is covered too. A target that does not exist on disk still stays dangling, unchanged from before. _portable_out_of_root_sf moved next to id_remap so both the new edge-target branch and the existing node-level pass share one implementation. Four tests in tests/test_extract.py: the out-of-root #include gets a portable id instead of the raw slug, and its transient target_file hint never leaks into the returned edge; the same corpus built from two differently-nested checkout paths produces a byte-identical target (the reported non-determinism, made explicit); an in-root, same-batch include still resolves to the real node's id (negative/regression guard); and the equivalent out-of-root Python relative import is fixed too, since the gap was in the shared remap path, not language-specific. Known limitation: this covers every current target_file-stamping resolver (Python relative imports, C/C++/ObjC #include, JS/TS/Svelte/Astro/Vue rescued imports). A resolver that mints a path-derived edge target WITHOUT stamping target_file at all -- none do today -- would still leak; the fix closes the gap in the shared mechanism, not a per-language allowlist.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #1899, which gave a NODE minted from an absolute out-of-root path a portable
ext_-namespaced id, matching the node's own id against_make_id(str(its source_file)). Some cross-file resolvers (Python relative imports, C/C++/ObjC quoted#include) only ever emit an EDGE for an import target, no node — so when that target is outside the scan root, the belt-and-braces pass has nothing to learn the old→new id from, and the edge keeps the raw_make_id(str(absolute_path))slug forever. The scan path, including the OS username, ends up inlinks[].source/targetand differs between machines/checkouts even though the node id sets are identical._import_calso never stamped the transienttarget_filehint (#1814/#2169) its Python/JS siblings already use for this kind of canonicalization.Root cause
extract.py'sid_remappass already walkstarget_file-stamped edges to canonicalize in-root-but-unscanned targets, but explicitly skipped the out-of-root branch ("leave its ids alone"), deferring to a later pass that only fires for nodes.Fix
_import_cnow stampstarget_fileon a resolved#include, mirroring_import_python/_import_js.id_remap's out-of-root branch now mints the same portableext_id an out-of-root NODE already gets (reusing_portable_out_of_root_sf, moved next toid_remapso both share it), so an edge with no node of its own is covered too. A target missing on disk still stays dangling, unchanged.Reproduction
Before:
links[].targetis a raw absolute-path slug, different on every checkout. After:ext_lib_foo_h, identical from two independently-built, differently-nested checkouts of the same tree.Validation
Four tests in
tests/test_extract.py: out-of-root#includegets the portable id (transient hint never leaks); two differently-nested checkouts of the same corpus give a byte-identical target; an in-root, same-batch include still resolves to the real node's id (regression guard); the equivalent out-of-root Python relative import is fixed too, confirming the gap is in the shared path, not_import_c-specific.py3.12: branch 3765/4 skipped, parent 3761/4 → delta +4, exact. py3.10: branch 3764/1 failed/4 skipped, parent 3760/1/4 → delta +4, exact; the 1 failure (
test_manifest_ingest.py::test_apm_parses_name_and_deps) is identical both sides, pre-existing.ruff check .clean both sides; all fivetools.skillgenguards and agraphify updatesmoke run pass.Known limitation
Covers every current
target_file-stamping resolver (Python, C/C++/ObjC, JS/TS/Svelte/Astro/Vue). A resolver minting a path-derived target without stampingtarget_file— none do today — would still leak; this closes the shared-mechanism gap, not a per-language allowlist.