Skip to content

fix(build): keep definition_file repo-relative like source_file - #3023

Open
winesoft-namjin-yun wants to merge 1 commit into
Graphify-Labs:v8from
winesoft-namjin-yun:fix/decldef-definition-file-relative
Open

fix(build): keep definition_file repo-relative like source_file#3023
winesoft-namjin-yun wants to merge 1 commit into
Graphify-Labs:v8from
winesoft-namjin-yun:fix/decldef-definition-file-relative

Conversation

@winesoft-namjin-yun

Copy link
Copy Markdown
Contributor

Follow-up to #2990 / #2991, shipped in 0.9.49. My own regression — sorry.

What

_merge_decl_def_classes records the dropped impl node's provenance on the survivor by copying its source_file verbatim:

keeper["definition_file"] = definition["source_file"]

At that point the value is still absolute. Every pass that makes a stored path portable keys on the literal string "source_file", so the new attribute never gets relativized while its sibling does. The result, on a real C++ corpus built at /data/graphify/repos/ston-edge-server:

Source:     Application/Cache/Process/CacheHttp2Config.h L96
Defined in: /data/graphify/repos/ston-edge-server/Application/Cache/Process/CacheHttp2Config.cpp L47

Two consequences, both from the same line:

  • The graph is no longer portable. source_file is repo-relative precisely so a graph built on CI can be read from any checkout; Defined in: now names a path that exists on no other machine. A reader who follows it gets "no such file" and reasonably concludes the graph is stale.
  • It leaks the build machine's layout. For anyone who commits graphify-out/ or publishes a graph as a release asset, home-directory and directory-structure detail lands in graph.json. Nothing in get_node's output hints that this one field is machine-local.

Scope is .c/.cc/.cpp/.cxx/.m/.mm paired with a header — C/C++/Objective-C corpora only; nothing else grows the attribute.

Fix

Relativize definition_file wherever source_file is relativized:

  • build_from_json's per-node normalization (_norm_source_file, which also handles the Windows backslash case)
  • watch._relativize_source_files and watch._rebase_relative_source_files

In watch.py the two keys now share one _PORTABLE_PATH_KEYS tuple, so a future portable path cannot be added to one pass and missed by the other — which is exactly how this slipped through. The scope guard is unchanged: a definition_file outside the watched tree is still left alone rather than forced under the root.

Only the recorded string changes. Node ids, labels, source_file, node/edge counts and communities are all untouched.

Tests

Four, all red before the fix except the scope-guard regression test:

  • test_build_from_json_relativizes_definition_file — an absolute definition_file under root comes out as src/Foo.cpp; definition_location passes through untouched.
  • test_relativize_source_files_relativizes_definition_file
  • test_rebase_relative_source_files_rebases_definition_file
  • test_relativize_source_files_leaves_an_outside_definition_file_alone — pins the scope guard for the new key (passes either way; there to stop a future widening).

Note that the tests I shipped with #2991 asserted str(node["definition_file"]).endswith("Foo.cpp"), which passes for an absolute path too. That is why this got through, so the new assertions compare full relative strings.

Full suite, same machine, clean v8 vs this branch:

v8:           13 failed, 4756 passed, 232 skipped
this branch:  13 failed, 4760 passed, 232 skipped

Identical failure set by name (terraform, ollama-retry-cap, install-references, markdown-nested-frontmatter — all missing-optional-dependency failures in my environment, none on this path); +4 is the new tests.

Real-world check

Rebuilt three C++ corpora (1.7k, 1.2k and 550 files) on 0.9.49 plus this patch: 13,682 nodes carry a definition_file, and every one is now repo-relative — 0 absolute, 0 pointing at a non-impl extension, and all 13,682 survivors are headers, as expected. Node counts before and after the patch are identical (39,041 / 46,551 / 15,613), confirming only the string changed. Verified over the served path too: get_node now answers Defined in: Application/Granite/Delivery/CacheService/CacheFile.cpp L1684.

Environment: Python 3.12.3, tree-sitter 0.25.2, Linux, AST-only build with no LLM backend.


Unrelated nit, mentioning it only because it is one line in the same feature: the 0.9.49 release note for #2990 reads "the node now points at the definition site (the implementation body) instead of the header prototype". The shipped behaviour keeps source_file on the declaration and adds the definition site as separate attributes — that additivity (no re-keying of existing graphs) is the part worth stating. Happy to send a note-only PR if that is useful.

…hify-Labs#2990)

0.9.49 records definition_file/definition_location on the survivor when a
C/C++/ObjC declaration and its definition merge into one node, copying the
dropped impl node's source_file verbatim. Every pass that makes a stored path
portable keys on the literal "source_file", so the new attribute kept the
build machine's absolute path while its sibling was relativized: graph.json
carried the builder's filesystem layout, and get_node's "Defined in:" line
named a path that exists on no other checkout.

Relativize definition_file wherever source_file is relativized —
build_from_json's per-node normalization, and watch's _relativize_source_files
/ _rebase_relative_source_files, where the two keys now share one tuple so a
future portable path cannot be added to one pass and missed by the other.
Only the recorded string changes: node ids, labels, counts and edges are
untouched, and the scope guard still leaves a path outside the watched tree
alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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

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

Relativizes definition_file on merged C/C++/ObjC decl/def nodes alongside source_file in both build_from_json (graphify/build.py) and the watch path-normalization passes _relativize_source_files/_rebase_relative_source_files (via a new _PORTABLE_PATH_KEYS tuple in graphify/watch.py), so graphs no longer ship the build machine's absolute path. Adds build and watch tests covering relativization, the outside-scope guard, and cache-root rebasing for the new key, plus a changelog entry.

No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 1608 functions depend on the 777 functions this change touches.

Health — this change adds coupling hotspots:

  • new: _rebuild_code() — 98 callers, 50 callees
  • new: build_from_json() — 188 callers, 18 callees
  • new: build_merge() — 46 callers, 14 callees
  • new: to_obsidian() — 36 callers, 13 callees
  • new: extract_files_direct() — 17 callers, 20 callees
  • new: to_wiki() — 41 callers, 7 callees
  • new: extract_corpus_parallel() — 26 callers, 11 callees
  • new: _call_claude_cli() — 31 callers, 9 callees
  • …and 39 more — each is listed as a finding

Verification — 1608 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: 1229 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify build\_from\_json.

The verifier did not have enough to check build\_from\_json, 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: not verifiable: all 6 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly NameError — names the real obstacle, not a sampling gap)

Could not verify: Could not verify \_rebase\_relative\_source\_files.

The verifier did not have enough to check \_rebase\_relative\_source\_files, 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 `source_root` is annotated `Path` — outside the synthesizable primitive/collection set

Could not verify: Could not verify \_relativize\_source\_files.

The verifier did not have enough to check \_relativize\_source\_files, 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 `root` is annotated `Path` — outside the synthesizable primitive/collection set

· 47 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.

1 participant