Skip to content

fix(serve): resolve node ids containing punctuation in _find_node (#2467) - #2499

Open
ousamabenyounes wants to merge 1 commit into
Graphify-Labs:v8from
ousamabenyounes:fix/issue-2467
Open

fix(serve): resolve node ids containing punctuation in _find_node (#2467)#2499
ousamabenyounes wants to merge 1 commit into
Graphify-Labs:v8from
ousamabenyounes:fix/issue-2467

Conversation

@ousamabenyounes

Copy link
Copy Markdown

Why

Issues Fix #2467
Branch v8
Bug fix? yes

graphify explain (and _find_node) cannot resolve any node ID that contains punctuation — e.g. concept:domain:gosu, entity:*, commit:*. The reporter measured 2,327 of 2,327 concept/overlay nodes silently unresolvable by id, while punctuation-free ids resolved. The docstring promises id lookup ("Return node IDs whose label or ID matches …"), and find_node_ambiguity's own hint tells users to "retry with the full node id" — so this is an oversight, not a design choice.

Root cause

In graphify/serve.py _find_node_tiers, the query is normalized two ways:

term       = " ".join(_search_tokens(label))          # \w+ tokens -> punctuation becomes a space
norm_query = _strip_diacritics(str(label)).lower().strip()   # punctuation preserved

The exact-match tier compared the node id (nid_lower, which keeps punctuation) only against term (which replaces punctuation with spaces). For concept:domain:gosu, term is "concept domain gosu" — it can never equal nid_lower "concept:domain:gosu". The punctuation-preserving norm_query was already compared against norm_label/bare_label, but not against nid_lower.

What changed

Add the symmetric comparison norm_query == nid_lower to the exact-match tier — the exact counterpart of the norm_query == norm_label clause that was added for punctuated labels in #1704. One production line, plus a clarifying comment and a CHANGELOG entry.

Why it's safe

  • Purely additive, zero false-positive risk. A node newly enters the exact tier only when the whole diacritic-stripped, lowercased query byte-equals that node's id — the definition of an exact id match. It can only promote a currently-unresolvable id into exact; it cannot pull in a different node or reorder resolution for any existing query.
  • Correct tier. The docstring states node-ID exact matches are grouped with label exact matches, and the existing term == nid_lower clause already lives in exact.
  • Siblings untouched. shortest_path / _score_nodes use a separate scoring path and are not affected; explain and find_node_ambiguity route through _find_node_tiers and only benefit.
  • Scope is the exact-id case reported in explain/_find_node cannot resolve node IDs containing punctuation (e.g. concept:domain:x) #2467; partial-prefix punctuated-id lookup (the prefix tier) is intentionally left as a separate concern to avoid speculative false-prefix matches.

Test verification (RED → GREEN)

New test tests/test_serve.py::test_find_node_resolves_id_with_punctuation.

RED — on unmodified v8, with only the new test applied:

tests/test_serve.py:239: in test_find_node_resolves_id_with_punctuation
    assert _find_node(G, "concept:domain:gosu") == ["concept:domain:gosu"]
E   AssertionError: assert [] == ['concept:domain:gosu']
FAILED tests/test_serve.py::test_find_node_resolves_id_with_punctuation
1 failed

GREEN — with the fix:

tests/test_serve.py::test_find_node_resolves_id_with_punctuation PASSED
1 passed

The test also guards the regressions (punctuation-free ids and label lookups still resolve).

Full suite: 4044 passed on clean v84045 passed, 3 skipped on this branch (the one added test). ruff check graphify tests clean; python -m tools.skillgen --check OK; graphify --help install smoke OK.

…aphify-Labs#2467)

term tokenizes punctuation into spaces, so it never equaled the
punctuation-preserving node id (concept:domain:x resolved to nothing).
Compare norm_query against nid_lower too, mirroring the norm_query ==
norm_label label path from Graphify-Labs#1704. Exact-tier only, purely additive:
punctuation-free ids and label lookups are unchanged.

@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 node-matching logic in _find_node_tiers within graphify/serve.py to add a comparison between the punctuation-preserving norm_query and nid_lower, so that exactly-typed node IDs containing punctuation (e.g. concept:domain:x) can resolve by id. It updates the accompanying code comment and adds a changelog entry describing the intended fix (#2467). A new test (test_find_node_resolves_id_with_punctuation) is added covering punctuated-id lookup along with regression checks for plain ids and label lookups.

No blocking issues surfaced.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 663 functions depend on the 470 functions this change touches.

Health — grade A; 10 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):

  • dispatch_command() — 2 callers, 118 callees (high)
  • _query_graph_text() — 14 callers, 8 callees (high)
  • _score_query() — 14 callers, 5 callees (high)
  • run_benchmark() — 16 callers, 3 callees (high)
  • _query_terms() — 16 callers, 3 callees (high)
  • _build_server() — 2 callers, 20 callees (high)
  • _load_graph() — 9 callers, 3 callees (medium)
  • _shortest_path_text() — 5 callers, 4 callees (medium)
  • …and 2 more

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

explain/_find_node cannot resolve node IDs containing punctuation (e.g. concept:domain:x)

1 participant