fix(build): coerce non-string node ids so a numeric id cannot abort the build - #2337
fix(build): coerce non-string node ids so a numeric id cannot abort the build#2337Rishet11 wants to merge 1 commit into
Conversation
…he build
A backend can emit {"id": 10} where the schema says {"id": "10"}. Every id
consumer downstream assumes str, so one numeric id aborted the whole run at
the final merge step, after the full extraction had already been paid for.
There are three crash sites, not the one in the report:
- dedup._pick_winner's _CHUNK_SUFFIX.search(n["id"]) raises
"expected string or bytes-like object, got 'int'" (the reported
traceback), but only when the node lands in a duplicate or fuzzy group
- build_from_json's sorted(node_set) raises "'<' not supported between
instances of 'str' and 'int'" for a lone node with nothing to dedup
- the same sort, reached via the fuzzy-group path
A str() cast at the reported line therefore does not fix the common case:
a single numeric-id node never reaches _pick_winner. Coerce at ingest
instead, in _coerce_non_string_ids, called from build() before dedup (which
keys on id) and from build_from_json (the direct entry used by cli.py,
watch.py and diagnostics.py, which never goes through build()). The nested
call on the build() path is an idempotent no-op.
Edge endpoints and hyperedge members are coerced alongside the nodes on
purpose: coercing node ids alone would renumber 10 to "10" and leave every
edge pointing at the vanished 10, trading a loud crash for a silently
disconnected graph. The legacy from/to aliases are included because dedup
reads them directly (Graphify-Labs#803).
Only bool-free numeric scalars are coerced. A None, list or dict id is left
for validate_extraction to report, since str(None) == "None" would fabricate
an id that no edge references.
Closes Graphify-Labs#2326
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 adds handling for numeric (int/float) node ids that a backend might emit where strings are expected. A new helper _coerce_non_string_ids (with its _coerce_id companion) walks an extraction's nodes, edge endpoints (including legacy from/to aliases), and hyperedge members, converting numeric scalar ids to strings while leaving booleans and non-scalar values untouched. This coercion is invoked in two spots: in build before dedup, and in build_from_json after the links-to-edges remap. The PR also adds a new test file, tests/test_non_string_node_ids.py, covering the coercion behavior across the dedup path, the direct build_from_json entry, edge/hyperedge linkage, float and legacy-alias endpoints, and negative cases (bools, non-scalars, string ids). The surface area is limited to graphify/build.py and the new test module.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 727 functions depend on the 91 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
build_from_json()— 142 callers, 14 callees - worse:
build()— 28 callers, 4 callees
Verification — 727 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: 527 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
Closes #2326
The bug
A backend can emit
{"id": 10}where the schema says{"id": "10"}. Every id consumer downstream assumesstr, so one numeric id aborted the whole run at the final merge step, after the full extraction had already been paid for.Three crash sites, not one
While reproducing I found the reported traceback is only one of three:
dedup.py:635_CHUNK_SUFFIX.search(n["id"])→expected string or bytes-like object, got 'int'(the reported traceback)build.pysorted(node_set)→'<' not supported between instances of 'str' and 'int'sorted(node_set)That third row is why I did not take the one-line fix the traceback points at. A
str()cast inside_scoreleaves the most common case still crashing, because a lone node never reaches_pick_winner.The fix
New
_coerce_non_string_ids(extraction), called at two sites:build(), immediately beforededuplicate_entities, which keys on idbuild_from_json(), the direct entry used bycli.py,watch.pyanddiagnostics.py, which never goes throughbuild()Both are required: removing either reintroduces one of the crash sites. The nested call on the
build()path is an idempotent no-op.Edge endpoints and hyperedge members are coerced alongside the nodes on purpose. Coercing node ids alone would renumber
10to"10"and leave every edge pointing at the vanished10— trading a loud crash for a silently disconnected graph. The legacyfrom/toaliases are included because dedup reads them directly (#803).Only bool-free numeric scalars are coerced. A
None, list or dict id is left forvalidate_extractionto report, sincestr(None) == "None"would fabricate an id that no edge references.Verification
tests/test_non_string_node_ids.py; 12 fail with the fix reverted, so they genuinely bite4fe1109: 4 failed, 3895 passed, 3 skippedruff check .clean;python -m tools.skillgen --check,--audit-coverage,--schema-singletonall OKbuild_mergewith{"id": 10}: nodes['10', 'b'], edge('10', 'b')— node kept, edge intactKnown limitation
If a backend emitted both
10and"10"as separate nodes, they now merge, last-writer-wins. That matches existing behavior for two nodes already sharing a string id, so it is not a new failure mode, but it is worth knowing. Numeric hyperedgeidfields are not coerced either — only hyperedge members, which are node references.I could not reproduce against a live ollama backend (not available locally); I reproduced by feeding the builder the same fragment shape ollama produces, which is where the crash lives.