fix(skill): structurally extract docs, fix whole-file node ID drift - #2548
fix(skill): structurally extract docs, fix whole-file node ID drift#2548dolakzdenek wants to merge 1 commit into
Conversation
…x whole-file node ID drift
Part A only ever ran deterministic AST extraction over code files, even
though extract() already has a structural Markdown extractor
(extract_markdown, .md/.mdx/.qmd/.skill) that mints a stable whole-file
node + one node per heading. Every markdown file went through the LLM
semantic path only, so nothing upstream had already decided that file's
canonical node id - each Part B subagent had to invent one.
Running /graphify on a 48-file real-world docs corpus reproduced the
failure mode this causes: two subagents extracting the same doc suite
in parallel independently produced `docs_architecture` and
`docs_architecture_document` for one file (ARCHITECTURE.md), splitting
it into two disconnected nodes and inflating its apparent "god node"
degree with edges that actually belonged to the other shard.
Fixes at the root, in two parts:
- graphify/extract.py: add structural_extensions(), a public accessor
for which extensions have a registered structural extractor (derived
from the existing _DISPATCH table, not a hand-maintained duplicate
list - the exact drift hazard graphify.ids documents for node-ID
recipes). Exposed via graphify.__init__'s lazy-import map.
- skill Part A: also structurally extract document/paper files whose
extension is in structural_extensions(), not just code. Part C's
existing "AST nodes first, semantic deduped by id" merge then
naturally unifies any LLM-created whole-file node into the
AST-authoritative one, once both agree on the id.
- extraction-spec.md (verbose + compact): (1) define the whole-file
node ID convention explicitly - bare {stem}, matching
extract_markdown's own file_nid, never a `_document`/`_file` suffix;
(2) add the citer->citee direction rule for references/cites edges
between whole-file document nodes, mirroring the existing `calls`
rule for code, plus a self-check subagents can apply
(edge.source_file should match the source node's file, not the
target's).
A stub node's source_file deliberately stays on the CITING file (the
normal source_file RULE, unchanged) even when it stands in for a file
the subagent is only referencing, not extracting - not the referenced
file's own path. An earlier draft of this fix set it to the referenced
file's path, on the theory that a node should describe the file it
represents; running that draft as a live incremental --update against
a real 460-node docs graph destroyed 293 of those nodes (64%) in one
run. build_merge's replace-on-re-extract logic treats ANY node's
source_file appearing in a new extraction as proof that file was
re-extracted this run, with no way to distinguish "I read this file"
from "I merely cited it" - so a stub node honestly describing which
file it represents reads, to the merge, as "ARCHITECTURE.md was just
rescanned," and everything ARCHITECTURE.md previously contributed gets
pruned. tests/test_build_merge_hyperedges_and_prune.py adds a
regression test reproducing this exact mechanism in miniature, so a
future change to replace-on-re-extract semantics can't silently
reopen it.
tools/skillgen/gen.py: rename SHARED_INTRO_ALLOWLIST to
SHARED_CORE_ALLOWLIST (now holds intro consolidation AND shared
heading renames) and allowlist the Part A heading rename for the
per-host coverage audit; add _is_structural_docs_fix_line to the
monolith-roundtrip guard for aider/devin.
tests/test_extraction_spec_ids.py: lock the whole-file-node ID
convention to the real extract._make_id/_file_stem functions, plus a
direct regression test for the exact docs_architecture /
docs_architecture_document split.
All 5 skillgen structural validators (--check, --audit-coverage,
--schema-singleton, --monolith-roundtrip, --always-on-roundtrip) and
the full pytest suite pass.
There was a problem hiding this comment.
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 extends the skill pipeline's "Part A" structural extraction so it runs over documentation/paper files whose extensions have a registered structural extractor (e.g. .md, .mdx, .qmd, .skill), not just code files. To support this, it adds a new structural_extensions() helper in graphify/extract.py (exposed via graphify/__init__.py) that returns the set of extensions backed by a structural extractor, and rewrites the Part A snippets across the many per-agent skill markdown files and their generated/expected test fixtures to use it. The changes span the skill generator tooling, expected-output fixtures, and associated tests.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2562 functions depend on the 1464 functions this change touches.
Health — grade A; 10 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
extract()— 375 callers, 39 callees (high)_rebuild_code()— 93 callers, 52 callees (high)extract_xaml()— 19 callers, 17 callees (high)dispatch_command()— 2 callers, 118 callees (high)_get_extractor()— 24 callers, 6 callees (high)run_pipeline()— 8 callers, 13 callees (high)collect_files()— 17 callers, 6 callees (high)render()— 13 callers, 5 callees (high)- …and 2 more
Verification — 2562 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: 2432 function(s) in the blast radius were not formally verified this run
Summary
Part A of the skill only ever ran deterministic AST extraction over code files, even though
extract()already ships a structural Markdown extractor (extract_markdown,.md/.mdx/.qmd/.skill) that mints a stable whole-file node + one node per heading. Every markdown file went through the LLM semantic path only, so nothing upstream had already decided that file's canonical node id — each Part B subagent had to invent one independently.Running
/graphifyon a 48-file real-world docs corpus reproduced the failure mode this causes: two subagents extracting the same doc suite in parallel independently produceddocs_architectureanddocs_architecture_documentfor one file (ARCHITECTURE.md), splitting it into two disconnected nodes and inflating its apparent "god node" degree with edges that actually belonged to the other shard. The same pattern showed up for 8 files in that corpus.What changed
graphify/extract.py: addsstructural_extensions(), a public accessor for which extensions have a registered structural extractor (derived from the existing_DISPATCHtable, not a hand-maintained duplicate list — the exact drift hazardgraphify.idsdocuments for node-ID recipes). Exposed viagraphify.__init__'s lazy-import map.structural_extensions(), not just code. Part C's existing "AST nodes first, semantic deduped by id" merge then naturally unifies any LLM-created whole-file node into the AST-authoritative one, once both agree on the id — no new dedup logic needed.extraction-spec.md(verbose + compact):{stem}, matchingextract_markdown's ownfile_nid, never a_document/_filesuffix.references/citesedges between whole-file document nodes, mirroring the existingcallsrule for code, plus a self-check subagents can apply.source_file— the normal rule, unchanged. An earlier draft of this fix pointed a stub node'ssource_fileat the referenced file instead (on the theory that a node should describe the file it represents); running that draft as a live incremental--updateagainst a real 460-node docs graph destroyed 293 of those nodes (64%) in one run, becausebuild_merge's replace-on-re-extract logic treats any node'ssource_fileappearing in a new extraction as proof that file was just re-scanned.tests/test_build_merge_hyperedges_and_prune.pyadds a regression test reproducing this exact mechanism in miniature.tools/skillgen/gen.py: renamesSHARED_INTRO_ALLOWLIST→SHARED_CORE_ALLOWLIST(now holds intro consolidation and shared heading renames) and allowlists the Part A heading rename for the per-host coverage audit; adds_is_structural_docs_fix_lineto the monolith-roundtrip guard for aider/devin.tests/test_extraction_spec_ids.py: locks the whole-file-node ID convention to the realextract._make_id/_file_stemfunctions, plus a direct regression test for the exactdocs_architecture/docs_architecture_documentsplit.Test plan
--check,--audit-coverage,--schema-singleton,--monolith-roundtrip,--always-on-roundtrippytestsuite green (4126 passed; one pre-existing flaky test unrelated to this change, confirmed flaky on a cleanv8checkout too)build_mergedata-loss hazard found while developing this fix🤖 Generated with Claude Code