feat(understand-diff): optional graph query backend for retrieval - #651
Open
galshubeli wants to merge 10 commits into
Open
feat(understand-diff): optional graph query backend for retrieval#651galshubeli wants to merge 10 commits into
galshubeli wants to merge 10 commits into
Conversation
added 10 commits
August 17, 2026 10:26
Adds an opt-in, read-only query adapter over the existing knowledge-graph.json so understand-diff can resolve changed files and their blast radius with a query instead of a chain of greps. Nothing changes by default: the JSON stays the source of truth, no data is written back, and when no backend is configured the skill continues with the existing grep steps. Backends are picked automatically. FalkorDBLite runs embedded with no server or configuration; UA_FALKORDB_URL points at a FalkorDB instance instead. The graph is rebuilt only when the JSON's content hash changes. On this repo's own graph (793 nodes / 1390 edges) the blast-radius query returns the same 111 affected nodes as the grep walk.
…workspaces Extends the query adapter to cover the rest of what was proposed in Egonex-AI#649, all opt-in and all still read-only with respect to knowledge-graph.json. Incremental sync: a digest is stored per source file, so re-syncing after a /understand refresh replaces only the files whose nodes changed. Editing one file resyncs 1 file / 10 nodes instead of 379 / 793, and node and edge counts are preserved exactly because edges touching a rebuilt node are re-created. Semantic search: when UA_EMBED_URL points at an embedding endpoint, node vectors are built during sync and indexed. semantic-traverse seeds from vector similarity and traverses in a single statement. Vectors live only in the graph, never in the JSON, which would grow by ~1.2MB. Without the env var nothing changes. Multi-repo: --workspace loads several repos into one graph with ids namespaced per repo, and links repos that declare each other in package.json, so impact analysis crosses repo boundaries. File-level cross-repo edges need the scan phase's import map and are left as a follow-up. Single-repo behaviour is unchanged: blast-radius, calls-from, calls-to and nodes-for-file return identical results to before, with un-namespaced ids.
…xposed Adds tests/skill/diff/test_graph_query.py following the existing unittest convention. Every test skips unless a FalkorDB backend is importable, so a checkout without the optional dependency runs 23 skips and exits 0. The graphs are synthetic with a hand-checkable import chain rather than a snapshot of a real repo, so the expected traversal results are derivable from the fixture. Writing the tests turned up a real bug in nodes-for-file. Paths were only matched in one direction, checking whether the stored path ends with the caller's. That fails whenever the caller's path is the longer of the two, which is what happens when git reports repository-relative paths but the graph was built from a scoped subdirectory. Now matched from the right in both directions. Also fixes a Cypher precedence bug found the same way: ENDS WITH binds tighter than string concatenation, so the appended path needed parentheses. Single-repo results are unchanged: blast-radius 111, calls-from 12, calls-to 3, nodes-for-file 10 on this repo's own graph.
Workspaces previously merged every repo into a single graph and namespaced node ids to keep them apart. That made cross-repo traversal easy but it gave up isolation, made every query pay for the whole workspace, and meant a repo's ids read differently inside a workspace than outside one. Now each repo keeps its own graph, with its own ids, and a small index graph holds only the repos and the dependencies between them. One backend instance holds all of them, so isolation costs no extra processes. A cross-repo question is answered in two cheap stages: traverse the index to find which repos are affected, then query only those repos' graphs. The index is tiny, so the traversal stays transitive and engine-side rather than becoming a repo-by-repo walk in Python. affected-repos exposes the index directly; blast-radius returns same-repo detail alongside a downstreamRepos list. Downstream results are repo-level because there are no file-level cross-repo edges to follow — that needs the scan phase's import map. Tests extended to a web -> api -> shared chain so transitivity and the hop limit are both covered, plus a check that a repo cannot see another's files. 26 tests, still skipped entirely when no backend is installed. Single-repo results unchanged: blast-radius 111, calls-from 12, calls-to 3, nodes-for-file 10.
Four fixes found by reviewing the sync path. Edge-only changes were never synced. Digests covered node fields only, so adding an import between two otherwise unchanged files left every digest intact, sync reported 'cached', and the edge never reached the graph. That is the most common thing a commit does, and dependency edges are the whole point of this feature. Digests now cover each file's incident edges, so both endpoints resync. Stamps were written per file before edges were created, so a crash between the two left every file stamped with its edges missing, and the next run reported 'cached' and never repaired it. Stamps are now written last, after edges, so an interrupted sync is retried instead of silently kept. The vector index was only created on a first sync. Configuring UA_EMBED_URL after a plain sync produced a raw driver error, because no index existed and no node had a vector. Indexes are now ensured on every sync and nodes missing vectors are backfilled. Node and edge types are interpolated into Cypher as labels and relationship types, which cannot be parameterised. Since knowledge-graph.json is committed and shared between teammates, types are now validated as plain identifiers and rejected otherwise. Writes are also batched with UNWIND, one query per label and per relationship type rather than one per node and per edge: a full sync of this repo's graph drops from 2183 queries to roughly 50. Tests: 34, up from 26, covering edge add/remove/property changes, both injection attempts, and the backfill path. Semantic tests skip unless UA_EMBED_URL is reachable; everything still skips without a backend.
A second review turned up three more defects in the same sync layer, all of the same shape: persistent state was assumed rather than checked. Swapping embedding models left a stale vector index. The previous fix created an index when none existed but ignored one with the wrong dimension, so 384-dimension vectors were written into a 4-dimension index, sync reported success, and only the query failed -- with an error naming neither the cause nor the fix. The existing dimension is now read from db.indexes(); a mismatch drops the index, clears the unusable vectors and re-embeds, and the rebuild is reported in the sync summary. Stamps whose nodes no longer exist are pruned before diffing. A sync interrupted after stamping would otherwise leave the file marked present with its nodes missing, and every later run would call it cached. search silently ignored an explicit limit in batch mode, returning 25 rows to a caller that asked for 2 -- the opposite of what a skill watching its context wants. The limit is threaded through and exposed as --limit. The workspace index is no longer deleted and rebuilt on every invocation. It is stamped with a digest of the manifests it derives from, so unchanged manifests reuse it and concurrent readers never observe it half-built. Tests: 39, up from 34. The stale-index test has to plant the bad index from processes with no embedder configured, because a process that has one now repairs it on the way in.
Running each feature by hand turned up a defect the unit tests could not see, because the fixture had no two files sharing a name. blast-radius seeded from a node's name, and basenames are not unique: this repository has eleven files called index.ts and five called types.ts. So asking for the impact of index.ts unioned the impact of all eleven and said nothing about having done so. Worse, the skill instructions told the agent to pass exactly that -- the changed file's basename -- which is the ambiguous form. On this repo's own graph the difference is stark: --name types.ts reports 110 affected nodes, while the two real files report 47 and 65 separately. blast-radius now takes --path and seeds from that file alone, matching from the right in both directions like nodes-for-file already did. --name still works for the cases where a symbol is what you have, but the docstring and the skill instructions both now say to pass a path, which is what git reports anyway. Neither given is an error rather than an empty result. The test fixture gains a second file named types.ts so the ambiguity is representable, and a test asserts that a path does not pick up its namesake while a name does. Tests: 41, up from 39.
Walking the skill's own instructions on a real repository turned up two
problems that no amount of testing the adapter directly would have found.
The instructions said to run `python graph-query.py`, but the embedded
backend needs Python 3.12 or newer and a project's default python is often
older. On such a machine the availability check failed, the skill fell back
to grep, and the feature was dead weight that never announced itself. The
commands now use "${UA_PYTHON:-python}" so a user with a suitable
interpreter elsewhere can point at it once, the requirement is stated where
the commands are, and the error names the version it is actually running so
the cause is obvious.
The fast path also claimed to cover steps 4 to 6, but only covered 4 and 5:
step 6 is architectural layers, and layers were not exposed at all. Added a
layers-for op. It reads the layers array straight from the JSON rather than
the graph, because a layer lookup is a flat intersection rather than a
traversal, and keeping it out of the graph keeps it out of the incremental
sync's invariants -- which is where every bug in this file has been.
Verified end to end on a fixture repository: types.ts changed, and the
batch call reports store.ts at one hop, view.ts at two, and the Core layer.
Tests: 42, up from 41.
…recedence The existing tests were all integration tests that shell out to the CLI, and CI runs Python tests from an explicit module list that did not include them. So this branch contributed no executed test coverage on CI at all: the module was never named, and even named, every test would skip because the optional backend is not installed on the runner. Adds 27 unit tests over the pure helpers -- type validation, label and relationship naming, digest stability, layer intersection, data directory resolution, embedding text assembly and node row shape. They need no database, so they actually run: the full Python suite is now 164 tests in 0.55s, of which 42 skip for want of a backend. Both new modules are added to the CI step. layers_for is now a thin wrapper over a module-level layers_containing so the intersection can be tested without constructing a graph. The unit tests immediately found a real inconsistency. find_graph_json checked .ua before .understand-anything, while resolveUaDirName in packages/core/src/persistence/index.ts prefers the legacy directory when it exists. On a project with both, this adapter and the rest of UA would have disagreed about which graph is authoritative. Precedence now matches core, with a comment pointing at the function it has to agree with. Tests: 69 in this directory -- 42 integration, 27 unit.
The embedded backend needs Python 3.12 or newer, and a project's default python is frequently older -- in which case the availability check failed, the skill fell back to grep, and the feature was inert. Documenting an environment variable put the burden on the user for a problem the script can usually solve: a newer interpreter is normally installed alongside the old one. The script now looks for one that can actually import the driver and hands over to it, announcing the switch on stderr. Verified from a 3.10 interpreter: it finds the capable one on PATH and the fast path works with no configuration at all. UA_PYTHON still wins when the right interpreter is somewhere unusual, a configured server short-circuits the whole check since no local driver is needed, and a marker in the environment means the handover happens at most once. Candidates are probed by importing the driver rather than by trusting a version number, because a 3.12 interpreter without the package installed is no more use than a 3.10 one. Unit tests cover the guards. The probing branch itself is not unit tested because it ends in os.execve, which would replace the test runner.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
An opt-in, read-only query adapter over the existing
knowledge-graph.json, plus an optional fast path inunderstand-diffthat uses it. Follows up on discussion #649.Nothing changes by default. The JSON stays the source of truth, nothing is written back, and when no backend is configured the skill continues with the existing grep steps exactly as before. The diff is additive: no existing line is modified or removed.
Why
understand-diffresolves a changed file's blast radius by greppingknowledge-graph.jsonfor node ids, then grepping again for each id in the edges array. That's a multi-hop traversal done as repeated text search, so its cost grows with the size of the graph rather than the size of the answer.Measured on this repo's own graph (793 nodes / 1390 edges, produced by
/understand), for one changed file (packages/core/src/staleness.ts):batchcallSame 11 nodes, 13x less context. The gap widens with how connected the changed file is: for
packages/core/src/types.ts, which 41 other nodes import, the grep walk pulls ~107k tokens versus ~2.3k.Grep is charged only the characters it emits, so the real difference is larger — the model's reasoning tokens between hops aren't counted. The JSON being pretty-printed is the best case for grep.
How it works
skills/understand-diff/graph-query.pymirrors the JSON into a graph and answers queries against it:nodes-for-filereturns the file node plus every function and class defined in it (step 4).blast-radiusreturns what transitively depends on it (step 5).Backends are detected automatically:
UA_FALKORDB_URL.The graph is rebuilt only when the JSON's content hash changes, so repeated queries pay the load cost once. Loading is a script, not an agent, so it costs no LLM tokens.
Scope
understand-diffis wired up here.chat,explainandonboarduse the same grep pattern and could follow if you like the shape of this.Notes for review
batchcall for that reason.persistence/index.tsas asaveGraph/loadGraphstyle interface than as a skill-level script, say so and I'll rework it.