Repository navigation
fix(skill): complete induced subgraph in query fallback - #4316
nothariharan wants to merge 1 commit into
Conversation
…s#4314) The inline NetworkX fallback for `/graphify query` recorded an edge only when BFS/DFS discovered an *unvisited* neighbour. An edge between two already-visited nodes (two seeds, a triangle chord, a cycle arc, a cross-link) was therefore dropped, so the fallback returned a traversal tree rather than the induced subgraph over the node set it reports. `graphify query` had the identical defect and fixed it in Graphify-Labs#2323 via `graphify.serve._complete_induced_edges`; the fallback now mirrors that completion. The visited set controls expansion only, and the edge list is completed over the visited set: ordered-pair keys on a directed graph, unordered frozenset keys otherwise, self-loops skipped, duplicates collapsed, and edges incident to unvisited nodes excluded. Updated the authoritative sources (references/query/default.md, core/aider.md, core/devin.md), regenerated every affected artifact and expected/ snapshot, and added a sanctioned monolith-diff predicate in tools/skillgen/gen.py. Regression tests extract and execute the shipped fallback and cover seeded nodes, BFS/DFS cross-links, cycles, edge direction, and parallel-edge de-duplication. Closes Graphify-Labs#4314
|
Thanks for the pull request, @nothariharan. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
There was a problem hiding this comment.
Graphify reviewed this change.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; no formal proof of the changed code.
Graphify review — findings
Fixes the query fallback's subgraph output so it includes every edge between visited nodes. Previously it emitted only the BFS/DFS traversal-tree edges, which dropped seed-to-seed links, triangle chords and cross-links (#4314). After traversal, a completion pass scans edges incident to the subgraph and appends any missing ones, skipping self-loops, deduping (direction-aware on directed graphs) and keeping cost proportional to the subgraph size. The fix ships in every generated skill variant and their skillgen fragments/expected outputs, with new tests covering directed-edge direction and DFS cross-links.
Review partial — this diff was larger than one review pass covers, so later files were not reviewed; some findings may be missing.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 576 functions depend on the 505 functions this change touches.
Health — grade A; 5 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
render()— 13 callers, 5 callees (high)audit_coverage()— 8 callers, 6 callees (high)main()— 3 callers, 11 callees (medium)monolith_roundtrip()— 3 callers, 5 callees (medium)test_audit_catches_a_dropped_non_allowlisted_heading()— 0 callers, 6 callees (medium)
Verification — 576 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: 576 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
368 of 368 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-safetytests/test_affected_defines.py— full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— full-run-safetytests/test_analyze.py— full-run-safetytests/test_anthropic_custom_endpoint.py— full-run-safetytests/test_antigravity_install.py— full-run-safetytests/test_apm_fallback_version.py— full-run-safetytests/test_architecture_doc.py— full-run-safetytests/test_astro_extraction.py— full-run-safetytests/test_astro_import_ids.py— full-run-safetytests/test_atomic_canvas_export.py— full-run-safetytests/test_atomic_version_stamp.py— full-run-safetytests/test_atomic_writes.py— full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_blade_extractor.py— full-run-safetytests/test_build.py— full-run-safetytests/test_build_located_semantic_identity.py— full-run-safetytests/test_build_merge_dedup_scope.py— full-run-safetytests/test_build_merge_hyperedges_and_prune.py— full-run-safetytests/test_build_merge_shrink_guard.py— full-run-safetytests/test_builtin_global_type_refs.py— full-run-safetytests/test_cache.py— full-run-safetytests/test_cache_stale_import_target.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— full-run-safetytests/test_cargo_missing_manifest.py— full-run-safetytests/test_carried_hyperedge_remap.py— full-run-safetytests/test_case_sensitive_resolution.py— full-run-safetytests/test_charmap_encoding.py— full-run-safetytests/test_chunking.py— full-run-safetytests/test_cjs_module_extension.py— full-run-safetytests/test_claude_cli_backend.py— full-run-safetytests/test_claude_md.py— full-run-safetytests/test_cli_broken_pipe.py— full-run-safetytests/test_cli_export.py— full-run-safetytests/test_cli_help.py— full-run-safetytests/test_cloud_cta.py— full-run-safetytests/test_cluster.py— full-run-safetytests/test_cluster_ambiguous_scale.py— full-run-safetytests/test_cluster_exclude_hubs.py— full-run-safetytests/test_cobol_extractor.py— full-run-safetytests/test_codebuddy.py— full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safety- … and 318 more
non-code file(s) changed (
graphify/skill-aider.md,graphify/skill-devin.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/query.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 (
graphify/skill-aider.md,graphify/skill-devin.md,graphify/skills/agents/references/query.md,graphify/skills/amp/references/query.md,graphify/skills/claude/references/query.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.
Summary
Fixes #4314.
The inline NetworkX fallback for
/graphify queryrecorded an edge only when BFS/DFS discovered an unvisited neighbour, so an edge between two already-visited nodes (two seeds, a triangle chord, a cycle arc, a cross-link) was dropped. The fallback returned a traversal tree rather than the induced subgraph over the node set it reports.graphify queryhad the identical defect and already fixed it in #2323 viagraphify.serve._complete_induced_edges; the inline fallback now mirrors that completion.Reproduced on
v8with the issue's example (seeds A/B/C; edges A→B, B→C, A→C):Changes
tools/skillgen/fragments/references/query/default.md,tools/skillgen/fragments/core/aider.md,tools/skillgen/fragments/core/devin.md: add an induced-edge completion pass after the BFS/DFS walk. The visited set controls expansion only; edges are completed over the visited set (ordered-pair keys when directed, unorderedfrozensetotherwise; self-loops skipped; duplicates collapsed; edges incident to unvisited nodes excluded).tools/skillgen/gen.py:_is_query_induced_edges_fix_lineplus registration in_SANCTIONED_MONOLITH_DIFFS(the monolith round-trip guard).graphify/skills/*/references/query.md,graphify/skill-aider.md,graphify/skill-devin.md, and every matchingtools/skillgen/expected/*snapshot.tests/test_skill_query_fallback_induced_edges.py: extracts the shipped fallback and executes it against a realgraphify-out/graph.json; covers seeded nodes, BFS/DFS cross-links, cycles, edge direction, and parallel-edge de-duplication (24 tests across the reference fragment and the shipped aider/devin/codex artifacts).Verification
python -m tools.skillgen --check→ OK (134 artifacts)python -m tools.skillgen --monolith-roundtrip→ OKpython -m tools.skillgen --audit-coverage→ OKpytest tests/test_skill_query_fallback_induced_edges.py→ 24 passedHEAD: the seeded case emits 0 edges before, 3 after; the new fixture also asserts the completion line is present, so a revert fails rather than silently passing.pathusesnx.shortest_pathandexplainis a one-hop listing, so neither needed changes.Note:
tests/test_skillgen.py::test_no_version_or_timestamp_in_outputandtests/test_skill_persisted_scan_options.pyfail identically on pristinev8in an uninstalled source checkout (__version__ == "unknown"/import graphifyfrom a temp cwd). They are unrelated to this change.Out of scope, same defect class
graphify/benchmark.py::_query_subgraph_tokensrecords edges the same discovery-coupled way. It only feeds an internal token estimate forgraphify benchmark, so it is not part of the/graphify queryanswer path; flagged for awareness rather than changed here.Notes for maintainers
v8(repo default).