fix(export): a control character in a label must not abort the export (#2897) - #2899
Conversation
…Graphify-Labs#2897) Labels reach the exporters unfiltered from the corpus. A markdown heading pasted from a terminal capture carries ANSI escapes (\x1b), and the form feed some Python/Emacs sources use as a section separator is \x0c. Detection, extraction and the graph build all accept them; two exporters then died on the whole graph. to_graphml -> ValueError: All strings must be XML compatible: Unicode or ASCII, no NULL bytes or control characters to_obsidian -> OSError: [Errno 22] Invalid argument Reproduced end to end from a single markdown file whose heading contains an ANSI colour sequence: 5 nodes, one label carrying \x1b, and graphml plus obsidian both crash while cypher, json and html complete. Losing the entire vault or the entire .graphml over one heading is the wrong failure mode -- the character is not even information anyone wanted. The codebase already knew the hazard, which is what makes this a gap rather than a design decision: _cypher_escape drops C0 controls explicitly, and to_html runs labels through security.sanitize_label. Only these two paths did not. - GraphML: strings are sanitised in _graphml_safe, the coercion hook that already existed for the None/non-scalar cases (Graphify-Labs#1831). XML 1.0 permits tab, LF and CR, so only the other C0 controls are dropped. Node IDs are relabelled through the same filter, since they become the `id` attribute of every <node> and edge endpoint and a crash there loses the export just as completely. - Obsidian: the stem builder already folded CR/LF to spaces; every other C0 control (and DEL) now folds the same way instead of being carried into a path Windows rejects outright. to_svg does NOT crash but renders a tofu box where the escape was (matplotlib warns "Glyph 27 missing from font"). Same root cause, much smaller symptom, left alone here rather than folded into a crash fix. Deliberately narrow otherwise: ordinary labels round-trip byte-identical, and the whitespace XML actually allows is preserved rather than swept up with the rest.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Formal verification. 2 change(s) tested, no difference found (not proven).
Graphify review — findings
Strips XML-illegal C0 control characters from GraphML output (_strip_xml_illegal applied to string values, JSON-serialized attributes, and node IDs) and folds all control characters to spaces when building Obsidian filename stems (_obsidian_safe_stem via _CONTROL_TO_SPACE_RE), so a single control character in a label no longer aborts the whole export. Tab/LF/CR are preserved since XML permits them. Adds tests/test_export_control_characters.py covering GraphML, Obsidian, and the already-safe Cypher/JSON paths.
Worth a look
- XML sanitizer misses non-C0 XML-illegal codepoints —
graphify/export.py:505· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Control-character ID stripping can merge distinct nodes —
graphify/export.py:1176· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- GraphML node-id remap can collide distinct nodes into one —
graphify/export.py:1176· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- GraphML ID sanitization can merge distinct nodes —
graphify/export.py:1178· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Node id relabel can collide two distinct ids into one, silently dropping a node/edges —
graphify/export.py:1178· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 488 functions depend on the 61 functions this change touches.
Health — this change adds coupling hotspots:
- new:
_rebuild_code()— 98 callers, 51 callees - new:
build_merge()— 46 callers, 14 callees - new:
to_obsidian()— 34 callers, 12 callees - new:
to_json()— 50 callers, 7 callees - new:
dispatch_command()— 2 callers, 117 callees - new:
_make_graph()— 32 callers, 6 callees - new:
run_pipeline()— 8 callers, 13 callees - new:
to_canvas()— 17 callers, 4 callees - …and 5 more — each is listed as a finding
Verification — 488 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: 340 function(s) in the blast radius were not formally verified this run
Formal verification
No difference found (not proven): No behavior difference found in \_obsidian\_safe\_stem (not a proof).
The verifier ran both versions of \_obsidian\_safe\_stem on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
No difference found (not proven): No behavior difference found in to\_graphml (not a proof).
The verifier ran both versions of to\_graphml on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 13 more finding(s) on lines outside this diff (see the check run).
|
Shipped in v0.9.48 via authorship-preserving cherry-pick. Thanks @abhay-codes07! Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.48 |
Fixes #2897.
The bug
Labels reach the exporters unfiltered from the corpus. Two of them die on a C0 control character, and they take the whole export with them:
End to end from one markdown file whose heading carries an ANSI colour sequence — what you get by pasting a terminal capture into docs:
Detection, extraction and the graph build all accept the file happily; the failure lands at the very end, after the expensive work, and costs the entire artifact rather than one note.
Narrowed to exact classes:
Neither trigger is exotic: ANSI escapes come from committed terminal captures, and form feed is the long-standing Emacs/Python page separator inside source files.
Why this reads as a gap, not a decision
The codebase already handles this everywhere else:
_cypher_escapedrops C0 controls, and says so — "First normalise: drop NUL and other C0 control chars except tab."to_htmlroutes labels throughsecurity.sanitize_label, whose docstring is "Strip control characters and cap length."Only these two paths hand the raw label to a writer that cannot take it.
The change
GraphML — strings are filtered in
_graphml_safe, the coercion hook that already existed for theNone/non-scalar cases (#1831), so there is one place where "can this value be written" is decided. Only the C0 controls XML forbids are dropped; tab, LF and CR are valid XML and are preserved, with a test pinning that so the fix cannot creep into ordinary whitespace.Node IDs go through the same filter via
relabel_nodes. They become theidattribute of every<node>and edge endpoint, so a control character there is exactly as fatal as one in a value — and a hand-built graph can carry one even though normalised ids never do.Obsidian —
_obsidian_safe_stemalready folded CR/LF to spaces; the remaining C0 controls and DEL now fold the same way instead of being carried into a path Windows rejects with EINVAL.Losing an escape character is the right cost. Losing the export is not.
Deliberately not included
to_svgdoes not crash, but matplotlib warnsGlyph 27 () missing from font(s) DejaVu Sansand renders a tofu box where the escape was. Same root cause, much smaller symptom, and fixing it means touching the label path of a renderer I would want to test properly rather than fold into a crash fix. Happy to follow up if you want the exporters fully uniform.Tests
tests/test_export_control_characters.py(33 tests): every crashing character class against both exporters, the XML-legal whitespace surviving untouched, a control character in a node id, the readable part of the label still reaching the file, an all-control label still yielding a usable name, and the two exporters that already coped continuing to cope. Plus a clean label round-tripping byte-identical, so the fix is invisible for ordinary input.Reverting
export.pyand keeping the tests fails 21 of 33.Validation
Windows 11, Python 3.12, branched off
b14b52e(0.9.47).15 failed, 4738 passed->15 failed, 4771 passed. Identical failure set — no regressions; the +33 are the new tests. The 15 are pre-existing Windows failures (symlink privileges, FIFO/socket fixtures, and the install-scope ones in my open test(install): stop three Windows failures that assume the POSIX layout #2802).