Skip to content

fix(dedup): make id prefix reconstruction Unicode aware - #3559

Closed
ayushcodes10 wants to merge 7 commits into
Graphify-Labs:v8from
ayushcodes10:fix-3352-unicode-id-prefixes
Closed

ayushcodes10 wants to merge 7 commits into
Graphify-Labs:v8from
ayushcodes10:fix-3352-unicode-id-prefixes

Conversation

@ayushcodes10

Copy link
Copy Markdown
Contributor

Fixes #3352.

Summary

The reported symptom: when two markdown pages mention the same entity, the page that actually defines the concept can lose its own node to a page that merely mentions it in passing. The reporter's concrete case is a page named exactly after the concept (Korean filename) losing its node to an unrelated page that just references it.

dedup.py already has the mechanism the issue's own "Suggestion" section asks for: _collision_rank ranks a node whose source_file defines its id (_defines_id) ahead of one that merely references it, specifically so the defining page wins. That mechanism is correct and already tested for ASCII paths. The bug is one level down: _id_prefixes — which _defines_id uses to reconstruct what id a node extracted from a given source_file would legitimately mint — reimplemented id slugification with its own ASCII-only regex (re.compile(r"[^a-z0-9]+")) instead of the canonical normalize_id() every real extractor actually mints an id with. For a path segment made of non-Latin characters (Korean, CJK, Cyrillic, ...), the ASCII regex strips it entirely instead of preserving it, so the reconstructed prefix never matches the id that was actually minted for that file. _defines_id then never recognizes the defining page as defining anything, and _collision_rank falls through to plain arrival order — exactly the symptom reported.

Verified directly against current code before writing the fix: _id_prefixes('concepts/작업 단위 폴더 + README 진입점 컨벤션.md') returned {'concepts_readme', 'readme'} (every Korean character dropped), while the id an extractor actually mints for that path (via _file_stem + make_id) is concepts_작업_단위_폴더_readme_진입점_컨벤션 — completely different from any prefix the old code could ever reconstruct.

Changes

  • graphify/dedup.py: _id_prefixes now slugifies each path segment with normalize_id (from graphify.ids, the same Unicode-aware casefold→NFKC→[^\w]+ recipe every extractor uses) instead of the local ASCII-only regex, which is now unused and removed.
  • tests/test_dedup.py: five new tests — _id_prefixes preserving non-Latin segments, _defines_id recognizing a non-Latin defining path, an end-to-end deduplicate_entities test reproducing the issue's own Korean defining/referencing pair in both arrival orders, and a negative control confirming an ordinary ASCII path's reconstructed prefixes are byte-for-byte unchanged.
  • CHANGELOG.md: entry under 0.9.61 (unreleased).

Testing

  • python -m pytest tests/test_dedup.py -k "korean or non_latin or ascii_path_unchanged" -q — 5 passed
  • Full suite: python -m pytest -q — 5490 passed, 68 skipped (pre-existing unrelated failures excluded: tests/test_ollama_retry_cap.py missing the optional openai module, and test_ts_normalizer_scales_linearly_on_large_files / test_hyperedge_convex_hull_js_is_geometrically_sound, both known environment-specific flakes unrelated to this change)
  • python -m tools.skillgen --check — OK

🤖 Generated with Claude Code

https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh

ayushcodes10 and others added 7 commits September 14, 2026 22:37
Toward issue 3352: _id_prefixes reimplements id slugification with
its own ASCII only regex instead of the canonical normalize_id every
real extractor uses to mint an id, so it silently drops non Latin
characters. This adds the import with no behavior change yet; the
swap lands in the next commit.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Fixes issue 3352.

_id_prefixes reimplemented id segment slugification with its own
ASCII only regex, so any path segment made of non Latin characters
(Korean, CJK, Cyrillic and so on) collapsed to nothing instead of
being preserved. _defines_id, the only real caller, uses these
prefixes to recognize when a node's own source_file is the file its
id encodes, and _collision_rank already implements the definer wins
rule this issue asks for. With the wrong prefixes, a node minted from
a non ASCII path never matched, so a genuine defining page lost the
definer wins tiebreak and the survivor fell through to plain arrival
order, exactly the symptom this issue reports.

Swapping to normalize_id, the same recipe every real extractor mints
an id with, makes the reconstructed prefixes match the ids that are
actually minted for such a file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Confirms the exact repro shape from the issue: the reconstructed
prefix set for a Korean file path preserves every segment instead of
collapsing them to nothing, matching what the file's own id is
actually minted as.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Confirms the layer above _id_prefixes actually benefits from the
fix: a node whose id and source_file both encode a Korean path is
recognized as defining that id, and a node sharing that id from a
different file is correctly recognized as merely referencing it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Runs deduplicate_entities on the defining and referencing Korean
node pair in both arrival orders and asserts the defining file own
node survives either way, matching the concrete case the issue
reports: a page whose filename equals the concept name losing its own
node to a page that merely mentions it in passing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Confirms the reconstructed prefix set for an ASCII path is byte for
byte identical to what the old ASCII only regex produced, so the fix
changes behavior only for paths the old code was already getting
wrong.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh

@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. 1 change(s) alter behavior, breaking input(s) attached.

Behavior changes: \_id\_prefixes changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_id\_prefixes behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.

Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.

Evidence: On input \{"source\_file":"'h\\u00e9llo w\\u00f6rld'"\}, the old code produced \{'h\_llo\_w\_rld'\} but the new code produces \{'héllo\_wörld'\}. Paste that input straight into a regression test.


Graphify review — findings

Fixes the definer-wins dedup tiebreak for non-Latin filenames: _id_prefixes now slugifies each path segment with the Unicode-aware normalize_id used to mint real ids, instead of an ASCII-only regex that dropped Korean/CJK/Cyrillic characters entirely and left a defining page unable to claim the id it encodes. ASCII paths produce the same prefixes as before, so the only behavioral change is that non-Latin defining pages now win the tiebreak against pages that merely reference the same entity.

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

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 725 functions depend on the 429 functions this change touches.

Health — this change adds coupling hotspots:

  • new: deduplicate_entities() — 77 callers, 24 callees
  • new: build_merge() — 76 callers, 14 callees
  • new: build() — 52 callers, 6 callees
  • new: dispatch_command() — 2 callers, 124 callees
  • new: _prune() — 9 callers, 3 callees
  • new: _llm_tiebreak() — 1 callers, 10 callees
  • new: test_poisoned_manifest_is_healed() — 0 callers, 6 callees

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

Test selection

Test selection

275 of 275 test file(s) selected (100%) via static blast radius.

Escalated to a full run for safety — the selection is not trustworthy on its own (see below). CI should run the whole suite.

  • tests/test_affected_cli.py — full-run-safety
  • tests/test_affected_member_seed.py — full-run-safety
  • tests/test_agents_platform.py — full-run-safety
  • tests/test_analyze.py — full-run-safety
  • tests/test_anthropic_custom_endpoint.py — full-run-safety
  • tests/test_antigravity_install.py — full-run-safety
  • tests/test_apm_fallback_version.py — full-run-safety
  • tests/test_architecture_doc.py — full-run-safety
  • tests/test_astro_extraction.py — full-run-safety
  • tests/test_astro_import_ids.py — full-run-safety
  • tests/test_atomic_canvas_export.py — full-run-safety
  • tests/test_atomic_version_stamp.py — full-run-safety
  • tests/test_atomic_writes.py — full-run-safety
  • tests/test_backend_env_isolation.py — full-run-safety
  • tests/test_backend_extras.py — full-run-safety
  • tests/test_benchmark.py — full-run-safety
  • tests/test_benchmark_raw_graph.py — full-run-safety
  • tests/test_build.py — impact, full-run-safety
  • tests/test_build_merge_dedup_scope.py — impact, full-run-safety
  • tests/test_build_merge_hyperedges_and_prune.py — impact, full-run-safety
  • tests/test_build_merge_shrink_guard.py — impact, full-run-safety
  • tests/test_builtin_global_type_refs.py — full-run-safety
  • tests/test_cache.py — full-run-safety
  • tests/test_callflow_html.py — full-run-safety
  • tests/test_cargo_introspect.py — full-run-safety
  • tests/test_carried_hyperedge_remap.py — impact, full-run-safety
  • tests/test_case_sensitive_resolution.py — full-run-safety
  • tests/test_charmap_encoding.py — full-run-safety
  • tests/test_chunking.py — full-run-safety
  • tests/test_cjs_module_extension.py — full-run-safety
  • tests/test_claude_cli_backend.py — full-run-safety
  • tests/test_claude_md.py — full-run-safety
  • tests/test_cli_broken_pipe.py — full-run-safety
  • tests/test_cli_export.py — full-run-safety
  • tests/test_cli_help.py — full-run-safety
  • tests/test_cluster.py — full-run-safety
  • tests/test_codebuddy.py — full-run-safety
  • tests/test_community_hub_labels.py — full-run-safety
  • tests/test_community_labels_skill.py — full-run-safety
  • tests/test_confidence.py — full-run-safety
  • tests/test_corrupt_graph_json.py — impact, full-run-safety
  • tests/test_cpp_nested_and_cli.py — full-run-safety
  • tests/test_cpp_objc_cross_file_calls.py — full-run-safety
  • tests/test_cpp_preprocess.py — full-run-safety
  • tests/test_cross_extension_reexport_self_cycle.py — impact, full-run-safety
  • tests/test_cross_language_call_resolution.py — full-run-safety
  • tests/test_cross_repo_member_calls.py — full-run-safety
  • tests/test_cross_repo_shared_types.py — full-run-safety
  • tests/test_csharp_call_site_generic_args.py — full-run-safety
  • tests/test_csharp_enum_members.py — full-run-safety
  • … and 225 more

non-code file(s) changed (CHANGELOG.md) → running the full suite for safety (a code graph can't see config/fixture/data deps)

changed code file(s) with no mapped test (CHANGELOG.md) — a coverage gap or a missing link — running the full suite rather than only the selected tests

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

Behavior changes: \_id\_prefixes changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_id\_prefixes behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.

Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.

Evidence: On input \{"source\_file":"'h\\u00e9llo w\\u00f6rld'"\}, the old code produced \{'h\_llo\_w\_rld'\} but the new code produces \{'héllo\_wörld'\}. Paste that input straight into a regression test.

· 7 more finding(s) on lines outside this diff (see the check run).

safishamsi added a commit that referenced this pull request Sep 15, 2026
…cribed

#3558's fix landed but its diff omitted the 3 tests its description claimed;
add them (self-loop ignored, self-loops-only is 0, 0..1 bound holds). Also drop
a changelog line the #3559 cherry-pick injected into the released 0.9.61 section.
@safishamsi

Copy link
Copy Markdown
Collaborator

Shipped in v0.9.62 (now on PyPI: uv tool install graphifyy==0.9.62). Your commits were cherry-picked with authorship preserved, so this shows up under your GitHub contributions. Thanks @ayushcodes10!

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.

Node identity derived from source_file silently drops the defining page's own node

2 participants