Add knowledge-graph retrieval tools: expand_node, god_nodes, surprising_connections, authors, vault_health, timeline, read_source - #104
Conversation
…urprising_connections, authors, vault_health, timeline, read_source Extends chat's GRAPH_CONTEXT with dedicated tools for graph exploration (EXPAND_NODE, GOD_NODES, SURPRISING_CONNECTIONS, READ_SOURCE), plus report-shaped REST-only capabilities (authors, vault_health, timeline) -- every capability is dual-exposed via both the chat tool loop and a public /graph/* REST surface (usable directly today, or by a future MCP server), per the existing service -> kg_app.py -> KnowledgeGraphClient -> caller pattern already used for search/entities_for_file/top_entities. Retrieval logic split out of KnowledgeGraphService (which already did extraction + storage + background-loop + progress-tracking in one class) into a new kg_queries.py module, and ChatToolbox's tool dispatch moved from a hardcoded if/elif chain to a generic handler/grounding-flag lookup on ToolSpec so adding a tool no longer means editing multiple hardcoded lists by hand. surprising_connections is cached and computed only on the background index thread (2-hop enumeration is heavier than the other flat-scan queries), and deliberately avoids a `NOT (a)-[:RelatesTo]-(c)` Cypher pattern predicate in favor of a Python set-difference check, matching vault_health's existing precedent for the same Kùzu portability reason. Manual UI controls let a person trigger any of the 7 capabilities directly from the chat, independent of the LLM ever deciding to call one.
There was a problem hiding this comment.
🟡 Changes recommended
New raw-read surfaces should add path-traversal guards for slugs, and read_source() should avoid loading entire files for summary mode to preserve the bounded-read intent and performance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR expands Prisma’s knowledge-graph retrieval surface by adding multiple new KG query capabilities (tool-loop + public REST), extracting query logic into a dedicated module, and wiring UI + agent/tooling changes to support the new endpoints and bounded source reads.
Changes:
- Added new KG retrieval capabilities (expand_node, god_nodes, surprising_connections, authors, vault_health, timeline) and exposed them via
/graph/*plus mirrored client/service APIs. - Introduced bounded raw-document reading (
read_source) with REST exposure atGET /notes/{slug}/read, plus unit tests. - Refactored chat tool dispatch to a ToolSpec-driven handler/grounding mapping and updated agent claim parsing/grounding detection accordingly.
File summaries
| File | Description |
|---|---|
| ui/src/routes/+page.svelte | Adds manual UI controls to call /graph/* and /notes/{slug}/read directly and render results. |
| tests/unit/services/test_source_reader.py | Adds unit tests for bounded source reading modes (summary/section/ripgrep). |
| tests/unit/services/test_knowledge_graph_service.py | Adds service-level cache/refresh tests for surprising_connections. |
| tests/unit/services/test_knowledge_graph_client.py | Adds client mirror-method tests for new /graph/* capabilities and updates TopEntity shape expectations. |
| tests/unit/services/test_kg_queries.py | Adds integration-style tests for the new pure KG query functions over embedded Kùzu. |
| tests/unit/services/test_chat_tools.py | Adds tool-loop coverage for new ToolSpec markers and ChatToolbox handlers. |
| tests/unit/server/test_notes_routes.py | Adds route tests for GET /notes/{slug}/read modes and error handling. |
| tests/unit/server/test_graph_routes.py | Adds isolated FastAPI router tests for the public /graph/* surface. |
| tests/unit/agents/test_chat_agent.py | Updates context-window sizing assumptions after tool section expansion. |
| prisma/storage/models/vault_models.py | Factors cited-claim relation literals into a shared CitedRelation type. |
| prisma/storage/models/kg_models.py | Extends KG API models (new response types; richer TopEntity; ReadSourceResponse). |
| prisma/services/source_reader.py | Implements bounded source reading modes used by chat + REST. |
| prisma/services/knowledge_graph_service.py | Delegates retrieval to kg_queries and adds background-cached surprising_connections. |
| prisma/services/knowledge_graph_client.py | Adds REST client mirror methods for the new KG capabilities. |
| prisma/services/kg_queries.py | Introduces pure retrieval/query functions (search/entities_for_file/god_nodes/etc.). |
| prisma/services/chat_tools.py | Refactors tool dispatch to handler lookup; adds new KG + READ_SOURCE tools; derives grounding tool set. |
| prisma/server/notes_routes.py | Adds GET /notes/{slug}/read REST surface for bounded source reads. |
| prisma/server/kg_app.py | Adds new KG capability endpoints on the KG service app. |
| prisma/server/graph_routes.py | Adds public /graph/* router factory that forwards to KnowledgeGraphClient. |
| prisma/server/app.py | Wires the new public graph router into the main FastAPI app. |
| prisma/agents/chat_agent.py | Validates FOOTNOTES_JSON via a typed model and derives grounding tools from ToolSpec. |
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ecode, avoid loading whole file for summary reads find_file()/_find_md()'s dir--name compound-slug decode (ADR-021) had no containment check against the vault root -- a slug like "..--..--secret" or "--etc--passwd" would resolve (or, for the latter, entirely replace self.root, since Path's / operator discards the left side when the right is absolute) outside the vault. This predates PR #104, but that PR is what first exposes it to untrusted input (READ_SOURCE / GET /notes/{slug}/read pass a slug straight through with no wiki-link-resolution context in between). Fixed at the source in vault.py's new _resolve_compound_slug() helper, shared by both find_file() and _find_md(), rather than patching each new call site separately. Also: read_source()'s summary mode read the whole file into memory before slicing to the first _EXCERPT_CHARS -- defeats the "bounded slice" intent for large vault documents. Now reads only the needed prefix directly from the file handle.
There was a problem hiding this comment.
🟡 Changes recommended
Graph provenance, relationship direction, grounding, bounded reads, and result-limit issues can produce incorrect output or unsafe resource usage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (12)
Previously missed (12) — in code that hasn't changed since the last review.
prisma/services/chat_tools.py:98
EXPAND_NODEis classified as grounding, but its result contains no real vault slug: the handler wraps it under the syntheticpath="knowledge-graph", whichslug_resolves()cannot resolve. The model is instructed to cite that path, after which_sources_resolve()drops the claim, leaving cited-looking answer text with no claim record. Include resolvable source slugs in the response or do not classify this tool as citable grounding.
prisma/services/chat_tools.py:124SURPRISING_CONNECTIONSis marked as grounding even though its output provides only entity ids/relations and is wrapped with the unresolvable synthetic pathknowledge-graph. Any footnote copied from that path is later discarded byslug_resolves(), so this tool cannot currently produce the citable content promised by the flag. Expose the contributing document slugs (or a resolvable graph source) before treating it as grounding.
prisma/services/chat_tools.py:508- Reducing
source_fileto its stem makes the emitted citation ambiguous for nested files with duplicate names, contrary to the compound-slug convention invault_models.py:555-584. Emit the suffix-less relative path with directory separators encoded as--so the source resolves to the exact document.
prisma/services/kg_queries.py:191 - This undirected match loses the stored edge direction, but the response reconstructs every edge as
node_id -> neighbor. For an incoming edge such asneighbor -[extends]-> node_id, consumers receive the opposite factual relation. Preserve the actual endpoints, or explicitly model/render these as undirected connections rather than directedEdgeInfovalues.
prisma/services/kg_queries.py:250 - The two undirected hops do not preserve each relationship’s stored direction, yet
SurprisingConnectionis rendered everywhere with directed arrows. This can invert directional relations such ascitesorextendsand present a false connection. Return direction metadata/preserved endpoints, or render the relations as explicitly undirected.
prisma/services/kg_queries.py:252 Entity.source_filecannot establish that the endpoints never shared a document:_upsert()MERGEs entities by id and overwrites this single field on every later document (knowledge_graph_service.py:1202-1213). After both endpoints are mentioned again in different files, this comparison passes even if an earlier file contained both, violating the defining invariant of this report. Track/query entity-to-document occurrences rather than comparing the last-written source fields.
prisma/services/kg_queries.py:288file_countis computed fromEntity.source_file, but that field is overwritten whenever the same entity id is extracted from a later document (knowledge_graph_service.py:1202-1213). An author represented by the same canonical entity in two files is therefore counted once, not twice. Count from durable entity/document occurrences (or another per-file provenance structure) instead.
prisma/services/kg_queries.py:353- This collects every matching entity and then resolves/parses a vault file for each result, with no result limit. A common query can therefore scan the full graph, perform many file reads, and return an unbounded response on a request thread. Add a validated limit/pagination parameter and stop collecting once that bound is reached.
prisma/services/kg_queries.py:360 - Using only
Path(source_file).stemdiscards the directory encoded in graph provenance. If two nested files share a stem,get_any()may resolve the wrong document and attach the wrong year. Convert the full suffix-less relative path to the compound-slug form instead.
prisma/services/knowledge_graph_service.py:1545 - The cache is populated by calling
kg_queries.surprising_connections()with its defaultlimit=15, but the new public route accepts limits up to 100 and this reader slices only what was cached. Requests above 15 therefore silently return at most 15 results. Either populate the cache to the public maximum or constrain the route/client contract to the cache size.
prisma/services/source_reader.py:118 - The match-count cap does not bound the returned text: a matching/context line can itself be arbitrarily large, so a one-line multi-megabyte document is returned almost in full. Apply a character cap to the joined blocks to preserve the documented bounded-slice guarantee.
ui/src/routes/+page.svelte:1056 - The manual Expand result drops
data.edges, so users see neighboring entities but not the relationships that define the expansion. Include each target’s relation(s) in the rendered line; otherwise this control cannot display the endpoint’s core edge information.
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Balanced
…(ReDoS risk)
query in read_source(mode="ripgrep") is REST-caller-controlled (GET
/notes/{slug}/read) and was compiled as a regex and run against every
line. Python's stdlib re has no execution timeout, so a
catastrophic-backtracking pattern (e.g. "(a+)+$") could hang a server
worker. Switched to a plain case-insensitive substring match -- this mode
never needed real regex power, and there's no safe way to keep it with
stdlib re alone.
There was a problem hiding this comment.
🟡 Changes recommended
Core graph queries can lose provenance or reverse edge semantics, producing incorrect and uncitable results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (9)
Previously missed (7) — in code that hasn't changed since the last review.
prisma/services/chat_tools.py:497
- This handler exposes no resolvable document source: the only
<untrusted_source path>is the literalknowledge-graph, while the footnote prompt tells the model to copy that path (or aSources:slug), andChatAgent._sources_resolve()rejects it because it is not a vault slug. Consequently claims grounded by EXPAND_NODE lose their citations, despite this tool being markedgrounding=True. Include the supporting relationship source-file slugs in the response/header.
This issue also appears on line 526 of the same file.
prisma/services/kg_queries.py:152
- The degree and sample relations aggregate all incident edges, but the only provenance returned is
e.source_file, a last-writer entity field rather than the edges' supporting documents._god_nodes()then presents that one file in itsSources:header, so generated citations can point to a document that does not support the reported connections. Aggregate distinctr.source_filevalues and expose those as the grounding sources instead.
prisma/services/kg_queries.py:205 - The MATCH is undirected, but every returned edge is rewritten as
node_id -> neighbour. This reverses incoming semantic relations—for example an extractedn2 -> center (extends)becomescenter -> n2 (extends)in the public API. Preserve the relationship's actual endpoints, or explicitly model/render these results as undirected.
This issue also appears on line 270 of the same file.
prisma/services/kg_queries.py:252
- This co-document check uses only each entity's current
source_file, butEntityis keyed globally by id and every_upsertoverwrites that scalar (knowledge_graph_service.py:1202-1213). If A and C co-occur in one document and are later re-seen separately in X and Y, this condition becomes true and reports them as “surprising” despite the earlier shared document. The definition requires durable entity-to-document membership rather than last-writer provenance.
prisma/services/kg_queries.py:256 - The candidate cap is applied before hub, direct-edge, and mirror filtering. A high-degree excluded hub can fill all 500 top-scoring rows, causing this function to cache an empty/short result even when valid lower-scoring connections exist. Filter excluded candidates before limiting, or page candidates until
limitaccepted results are collected.
prisma/services/kg_queries.py:288 - This report cannot reliably count “source files naming an author” from
Entity.authorandEntity.source_file: both fields are overwritten whenever the same global entity id is upserted from another document (knowledge_graph_service.py:1202-1213). A later mention can erase the author entirely, and repeated mentions collapse to one last-writer file. Build this report from document metadata or durable per-document entity provenance instead.
prisma/services/kg_queries.py:360 Path.stemdiscards the directory, so nested sources with the same filename share this cache key andget_any()may resolve the wrong bare-slug document. Vault paths moved into directories are addressed with compound slugs (vault.py:1148-1150); preserve the relative path here before resolving the year.
prisma/services/kg_queries.py:272
- These arrows are not guaranteed to match the stored relationship directions because both hops were matched undirected. Directional relations such as
citesorextendscan therefore be presented backwards inSurprisingConnectionand in both UI renderers. Return actual edge endpoints/directions, or render the hops without directional arrows.
out.append(SurprisingConnection(
entity_a=a_id, entity_b=c_id, bridge=b_id,
relation_a=rel_a, relation_b=rel_b, score=score,
prisma/services/chat_tools.py:531
- SURPRISING_CONNECTIONS is marked as grounding, but this result contains only entity ids/relations and is wrapped under the non-resolvable path
knowledge-graph; unlike GRAPH_CONTEXT/GOD_NODES it provides noSources:slugs. Any document citation the model derives from this tool is therefore rejected by_sources_resolve(). Return the two supporting edge source files and expose their compound slugs here.
lines = [
f"- {c.entity_a} --[{c.relation_a}]--> {c.bridge} --[{c.relation_b}]--> {c.entity_b}"
for c in links
]
wrapped = wrap_untrusted("knowledge-graph", "\n".join(lines))
return ToolResult(text=wrapped, raw=[c.model_dump() for c in links])
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Balanced
… not just match count _RIPGREP_MAX_MATCHES bounded how many match blocks were considered, not their size -- a single arbitrarily long line (a minified blob, a data URI) could still make the joined text return megabytes, contrary to the "bounded slice" contract every read_source() mode is supposed to honor. Adds a per-line cap (_RIPGREP_MAX_LINE_CHARS) and a total-output budget (_RIPGREP_MAX_TOTAL_CHARS), and a new ReadSourceResponse.truncated field so a caller can tell the difference between "this is everything" and "more existed than what's shown" -- structured signal instead of the caller having to guess from match_count vs. the shown block count.
There was a problem hiding this comment.
🟡 Changes recommended
The cross-document query conflicts with document-scoped entity IDs, and several direction, provenance, caching, and truncation issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (10)
Previously missed (6) — in code that hasn't changed since the last review.
prisma/agents/chat_agent.py:178
- The newly grounded tools do not preserve this predicate’s existing “no data => falsey result” invariant:
_expand_node,_god_nodes,_surprising_connections, and missing_read_sourceall return nonempty parenthetical messages withraw=[]. Consequently_turn_had_no_grounding()treats an all-empty tool turn as grounded and can retain claims that had no citable input. Carry an explicithad_data/grounding-success signal intoToolCallNode(or make all empty handlers persist a falsey result) instead of inferring success from message text.
prisma/services/chat_tools.py:98 - This tool is marked as grounding, but its response contains no vault source slug:
EntityInfoomitssource_file,EdgeInfoomits relationship provenance, and the wrapper path is the syntheticknowledge-graph. Since cited claims are later required to passslug_resolves(), an answer grounded only by EXPAND_NODE cannot produce the promised valid citation. Include resolvable source provenance in the response/prompt output, or do not classify this tool as grounding.
This issue also appears on line 124 of the same file.
prisma/services/chat_tools.py:508
Path.stemis not a resolvable identifier for nested files and is ambiguous when directories contain the same filename. TheSources:header is what the model copies into citations, so GOD_NODES can emit a slug that resolves to the wrong document. Preserve the relative directory using the established compound-slug encoding.
prisma/services/kg_queries.py:205- The undirected match loses each stored edge’s direction, but the response rewrites every edge as
node_id -> neighbor. For an existingn2 -> centerrelation such asextends, this returnscenter -> n2with the same label and reverses the asserted fact. Query incoming and outgoing edges separately (or return explicit original endpoints) and preserve their source/target orientation.
This issue also appears on line 270 of the same file.
prisma/services/knowledge_graph_service.py:1400
- A deletion-only watcher batch leaves this cache stale:
_delete_file()’s successful return value is ignored, whilechangedonly reflects extraction of existing files. Deleting a document can therefore leave its surprising connections visible until a later edit or full index refresh.
prisma/services/knowledge_graph_service.py:1543 - The cache is always computed with
kg_queries.surprising_connections’s default limit of 15, but the new public endpoint accepts limits up to 100. Requests for 16–100 can never receive the requested number even when more candidates exist. Populate the cache to the public maximum (preferably via a shared constant) and slice only at read time.
prisma/services/source_reader.py:123
- The response slices overlong matching/context lines but never sets
truncatedfor that loss. This contradictsReadSourceResponse.truncated’s contract and means callers are told the result is complete even when part of a line was discarded; the new single-long-line test currently codifies that incorrectFalsevalue.
f"{j + 1}{':' if j == i else '-'}{lines[j][:_RIPGREP_MAX_LINE_CHARS]}" for j in range(lo, hi)
prisma/services/kg_queries.py:360
- Using only
Path.stemdiscards the directory from an exact graphsource_file. If two nested sources share a filename,get_any()performs a bare-stem vault scan and can read the other file’s year, producing an incorrect timeline. Convert the relative source path to the repository’s compound slug so the exact document is resolved.
slug = Path(source_file).stem
prisma/services/kg_queries.py:272
a/b/ccome from an undirected path, sorelation_aandrelation_bdo not establish the directions later rendered asa --> b --> c. An originally storedb -> acitation is therefore reported with the arrow reversed. Preserve relationship endpoints/directions in the query and response, or render these links without directional arrows.
out.append(SurprisingConnection(
entity_a=a_id, entity_b=c_id, bridge=b_id,
relation_a=rel_a, relation_b=rel_b, score=score,
prisma/services/chat_tools.py:124
- SURPRISING_CONNECTIONS is classified as grounding, but
SurprisingConnectiondrops both relationships’source_filevalues and the handler only exposes entity IDs under a syntheticknowledge-graphpath. No source shown to the model can pass the later vault-slug validation, so this tool cannot support a cited claim. Return both contributing document slugs and expose them in the tool text, or mark the tool non-grounding.
grounding=True,
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Balanced
…l, not entity id
Production extraction mints entity ids as {stem}_{entity}
(_extraction_system_prompt) -- every document has its own id namespace, so
the same concept extracted from two different documents always ends up as
two different ids and can never literally be "the same node" for a 2-hop
Cypher pattern to walk through. The original implementation required
exactly that, so it could never find a real cross-document bridge in
practice; the unit tests only passed because they manually reused one
literal id across two documents, bypassing the very invariant that breaks
this in production.
Rewritten as a flat edge scan (same shape as god_nodes/authors/
vault_health) that groups candidate bridges by normalised label instead of
id, since label is the human-readable concept name and is comparable
across documents. Also fixes a second bug found while making this change:
a same-degree->=2 entity showed up twice in its own label group (once per
edge touching it), which without a same-node check looked like two
documents' separate instances sharing a label when it was really one node
connecting two neighbours directly -- not "surprising" at all, since
expand_node already surfaces that directly.
SurprisingConnection.bridge is now a label string, not an entity id --
updated the model docstring and both service/query-level tests to build
realistic fixtures (distinct ids, shared label) instead of the
unrealistic single-shared-id shortcut.
"ripgrep" implied a dependency on the external rg binary that never existed -- the mode is (and after the earlier ReDoS fix, has only ever been) pure-Python literal substring matching, no subprocess, no external tool. Renamed the mode value, constants, function, and tests to "literal" to say what it actually is.
There was a problem hiding this comment.
🟡 Changes recommended
Several retrieval paths can return incorrect relationships, stale cached data, ambiguous source attribution, or responses that violate documented bounds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (6) — in code that hasn't changed since the last review.
prisma/services/chat_tools.py:508
- These values are presented as source slugs for the model to copy into footnotes, but bare stems are ambiguous for nested files. The established API convention is a compound
dir--nameslug (vault_models.py:550-584); otherwise citations can resolve to a different same-named document.
prisma/services/kg_queries.py:387 source_fileis vault-relative, butPath.stemdiscards its directory. Nested sources such aspapers/a.mdandsources/a.mdboth cache undera, soget_any("a")can resolve the wrong file and attach the wrong year. Preserve the path using the API-facing compound slug format.
prisma/services/knowledge_graph_service.py:1400- A deletion-only watcher cycle leaves
changed == 0, so this refresh is skipped even though_delete_file()removed graph rows. The new cache can consequently keep returning connections involving deleted entities until another content update or restart; track successful deletions and refresh both caches when either extraction or deletion changes the graph.
prisma/services/knowledge_graph_service.py:1543 - The public route accepts
limitvalues up to 100, but this refresh useskg_queries.surprising_connections' default limit of 15. The cache therefore never contains more than 15 entries, so requests for 16–100 cannot honor their parameter.
prisma/services/source_reader.py:96 available_sectionsincludes every heading at full length, so section mode is not actually response-bounded: a file made entirely of headings can return essentially the whole document despite_SECTION_MAX_CHARS. Cap both the number and length of returned headings (and expose truncation) to preserve the endpoint's bounded-slice contract.
tests/unit/services/test_source_reader.py:149- This assertion codifies the opposite of the response contract: the matching line was capped, so content was omitted and
truncatedshould be true. Update this expectation together with the implementation so callers can detect incomplete output.
prisma/services/source_reader.py:124
- A line longer than
_RIPGREP_MAX_LINE_CHARSis silently sliced here without settingtruncated. This contradictsReadSourceResponse.truncated's documented per-line-budget semantics and prevents clients from knowing that returned text is incomplete.
block = "\n".join(
f"{j + 1}{':' if j == i else '-'}{lines[j][:_LITERAL_MAX_LINE_CHARS]}" for j in range(lo, hi)
)
prisma/services/kg_queries.py:300
- These arrows are synthesized from rows produced by an undirected edge scan, so neither
rel_anorrel_bis guaranteed to point in the displayed direction. Incomingcites/extendsedges can therefore be rendered as the opposite factual relationship. Retain each edge's original endpoints or render a direction-neutral connection.
out.append(SurprisingConnection(
entity_a=a_id, entity_b=c_id, bridge=bridge_label,
relation_a=rel_a, relation_b=rel_b, score=score,
))
- Files reviewed: 23/23 changed files
- Comments generated: 2
- Review effort level: Balanced
…e, fix label-based direct-edge exclusion expand_node() ran one undirected MATCH and unconditionally reported source=node_id regardless of the real stored direction, so an incoming edge (neighbour -> node_id) was reported as its false inverse. Split into two directed queries (outgoing, incoming) so the reported direction always matches what RelatesTo actually stores. surprising_connections()'s direct-edge exclusion was still keyed by entity id even though bridge identity had already moved to label -- a third document asserting a direct edge between the same concepts (using its own document-scoped ids) would never match an id-keyed exclusion set. Rebuilt the exclusion set from normalised endpoint labels instead.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues include incorrect graph relationship reporting, ambiguous source resolution, grounding-state errors, and malformed-slug failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (4) — in code that hasn't changed since the last review.
prisma/server/graph_routes.py:47
- The route accepts limits up to 100, but
_refresh_surprising_connections()populates the cache with the query's default limit of 15, so requests for 16–100 can never receive the requested number even when more candidates exist. Either fill the cache to the advertised maximum or validate this endpoint against the actual cache capacity.
prisma/services/chat_tools.py:508 - These source identifiers drop all directory components. For nested files with the same stem, the model sees an ambiguous bare source name and can cite a different document than the hub came from; the repository's API-facing representation is the compound
dir--nameslug (vault_models.py:555-584). Preserve the complete relative path when producing the source header.
prisma/services/kg_queries.py:408 - Reducing
source_filetoPath.stemloses its directory, so two files such assources/2024/paper.mdandsources/2025/paper.mdshare this cache key andvault.get_any("paper")may read the wrong year. Nested vault paths are API-addressed with compound slugs (dir--name; seevault_models.py:555-584), so derive that slug from the full relative path before resolving the node.
ui/src/routes/+page.svelte:591 - This result state is global rather than associated with the active chat. After running a tool in one chat, opening another chat leaves the old result visible there; an in-flight request can also complete after navigation and populate the new chat. Key results/requests by chat slug (or cancel and clear them on every active-chat transition) before rendering.
prisma/services/chat_tools.py:505
- This no-hit result is nonempty, so
ToolCallNode.resultbecomes truthy and_turn_had_no_grounding()treats the turn as grounded even thoughrawcontains no citable content. An unsupported answer after an empty hub lookup can therefore avoid the forced inference annotation. Return an empty result or add an explicit grounding-success signal.
if not entities:
return ToolResult(text="(the knowledge graph has no connected entities yet)", raw=[])
prisma/services/chat_tools.py:525
- This no-hit result is nonempty, so
ToolCallNode.resultbecomes truthy and_turn_had_no_grounding()treats the turn as grounded even thoughrawcontains no citable content. An unsupported answer after an empty connection lookup can therefore avoid the forced inference annotation. Return an empty result or add an explicit grounding-success signal.
if not links:
return ToolResult(text="(no surprising connections found yet)", raw=[])
prisma/services/chat_tools.py:542
- A missing document returns nonempty text, so this grounding tool is recorded with a truthy result and
_turn_had_no_grounding()concludes that real source content was found. The model can then answer after the miss without the forced inference annotation. Return an empty result for this miss (the agent supplies “no results found”) or track grounding success separately from display text.
try:
resp = read_source(self._vault, slug, mode="summary")
except FileNotFoundError:
return ToolResult(text=f"(no vault document with slug {slug!r})", raw=[])
- Files reviewed: 23/23 changed files
- Comments generated: 8
- Review effort level: Balanced
…ction, truncation, edge-case crash, broken test fixtures
- expand_node/god_nodes/surprising_connections/read_source now return an
empty text on a genuine "found nothing" result instead of a human-
readable placeholder message. A nonempty ToolResult.text was making
ToolCallNode.result truthy, so _turn_had_no_grounding never flagged the
turn as ungrounded even though there was nothing citable -- an
unsupported answer after an empty tool call could dodge the forced
inference annotation.
- ChatToolbox._expand_node's per-neighbour relation lookup was keyed by
edge.target unconditionally, silently dropping the relation for any
incoming edge (where target is the queried node, not the neighbour).
Rewritten to key by whichever endpoint isn't the queried node, and to
render each edge's real stored direction rather than a bare
"connects to" list.
- surprising_connections' relations were rendered as forward arrows in
both chat_tools.py and the UI even though the underlying flat scan
doesn't preserve which side of either hop actually stored the relation.
Switched to a direction-neutral "--[relation]--" rendering in both
places.
- source_reader.py's per-line clipping silently truncated an oversized
line without setting `truncated`, contradicting the field's contract.
- vault.py's _resolve_compound_slug didn't guard candidate construction
itself: a slug of exactly "--" decodes to "/", and
Path("/").with_suffix(...) raises ValueError before containment can be
checked, turning the public read endpoint's 404 into a 500.
- Two of the path-traversal regression tests (test_notes_routes.py,
test_vault_public_surface.py) planted their "leaked" file at the wrong
directory depth relative to what their own slug actually decodes to, so
they'd have passed against the old vulnerable code too. Fixed to use a
single-hop slug matched to where it actually resolves.
- UI's manual Expand control discarded `data.edges` entirely, showing
neighbours with no indication of how they're related. Now renders each
edge with its real direction, mirroring the chat tool's own text.
There was a problem hiding this comment.
🟡 Changes recommended
Graph cache invalidation, hub filtering, provenance handling, and nested source resolution have unresolved correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
prisma/services/kg_queries.py:302
- This only detects the same candidate entity instances having equal
source_filevalues; it does not detect that the endpoint concepts co-occur in a third document. Production extraction creates document-scoped IDs (knowledge_graph_service.py:164-165), so a third document mentioning A and C has new IDs and this comparison still passes. The current test reusesa/cacross documents, bypassing that invariant. Build a normalized label-to-document occurrence map and exclude candidates whose endpoint labels share any document.
This issue also appears on line 408 of the same file.
prisma/services/kg_queries.py:408
source_fileis a vault-relative path, but reducing it toPath.stemloses its directory. Forarchive/paper.md, this looks up barepaper; duplicate stems are supported and can resolve a different document, assigning the wrong year. Preserve the path as the API-facing compound slug (archive--paper), as established byvault_models.py:550-584.
slug = Path(source_file).stem
- Files reviewed: 23/23 changed files
- Comments generated: 4
- Review effort level: Balanced
…eletion-only cache refresh, hub-degree floor, cache limit - _god_nodes' Sources: header and timeline()'s year lookup used Path(source_file).stem, collapsing nested paths with the same filename to one slug. Factor the dir--name encode into VaultService. slug_for_relpath() (beside its _resolve_compound_slug decode) and point chat_tools, kg_queries, and vault.py's two inline copies at it. - _process_pending tracked only extracted files, so a deletion-only watcher cycle never refreshed top_entities/surprising_connections. Track successful _delete_file() calls; gate the refreshes on extracted or deleted. The false-positive log line stays on extracted. - _refresh_surprising_connections treated every top-15 cache slot as a hub, suppressing all bridges in vaults with <=15 connected entities. Add _HUB_MIN_DEGREE and require genuine connectivity, not just rank. - The cache inherited kg_queries.surprising_connections' default limit of 15 while GET /graph/surprising_connections allows up to 100. Share one SURPRISING_CONNECTIONS_MAX constant between the route bound and the cache populate size.
There was a problem hiding this comment.
🟡 Changes recommended
Chat trust-boundary leaks, incomplete hub exclusion, unbounded timeline results, and cross-chat UI state remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
prisma/services/kg_queries.py:207
- The queried entity itself is not filtered by trust tier; only its neighbour is. Supplying a legacy/chat entity ID therefore returns chat-derived edges, whose
source_fileis then emitted as a grounding source even though chats are explicitly non-citable. Require botheandoto be non-chat in both directions.
prisma/services/knowledge_graph_service.py:1569 _top_entities_cachecontains only the top 15 rows, so this does not actually exclude every entity meeting the degree threshold. In a larger graph, a degree-5+ bridge ranked 16th or lower remains eligible and can dominate “surprising” results. Compute the hub-ID set independently across all entities (or query by the threshold) rather than deriving it from the display cache.
ui/src/routes/+page.svelte:591- This result state is global to the page rather than scoped to the active chat. A completed result remains visible after switching chats, and an in-flight request started in chat A can populate the controls in chat B; nearby chat operations avoid this by capturing and rechecking
activeChat.slug. Associate the result with the originating slug (and render it only for that slug), or clear/cancel it on chat changes.
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Balanced
…ter chat tier on both edge endpoints - timeline() ran unbounded: a broad query matched the whole entity table, expanded every hit to all relationship documents, and read each one's frontmatter while holding the Kùzu lock. Added a `limit` (DEFAULT 50, route bound 1..TIMELINE_MAX) that caps both matched entities (top-scored, like search) and returned entries, plus a per-entity document cap for the hub fan-out. Propagated through graph_routes -> client -> kg_app -> service -> kg_queries. - The timeline edge scan filtered only the matched endpoint, so a note entity linked to a chat-tier entity picked up the chat edge's source_file as a timeline document. Bind the other endpoint and exclude chat tier on both sides, matching search/compute_top_entities/god_nodes/ surprising_connections.
There was a problem hiding this comment.
🟡 Changes recommended
Trust-boundary, unbounded-response, performance, and asynchronous UI-state issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
prisma/server/graph_routes.py:43
- Unlike the other collection endpoints, this public one-hop traversal has no result bound. A high-degree hub can return every incident edge/entity in one response and hold the service's sole Kùzu lock while materializing them. Add a validated
limit(or pagination) and propagate it through the client, worker route, service, and query.
prisma/server/graph_routes.py:67 - This endpoint returns the complete orphan list with no limit or pagination, so a large/disconnected vault can create an arbitrarily large in-memory response. Keep
orphan_countas the total, but bound or paginate theorphanscollection and propagate that contract through the client and worker.
prisma/services/source_reader.py:96 available_sectionsis unbounded even though this API promises bounded reads: every heading is returned at full length. A document with many headings (or one very long heading) can therefore produce an arbitrarily large response in both section-hit and section-miss cases. Cap the heading count and cumulative heading length, and expose truncation to callers.
ui/src/routes/+page.svelte:1119- This async result is applied without tying it to the chat that launched the request. If the user switches chats while
apiFetchis pending, the old request's result (or error) appears in the newly active chat; completed results also persist across later chat switches. Capture and recheckactiveChat.slugas the adjacent send/regenerate flows do, or clear/scope graph-tool state when opening a chat.
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Balanced
…pand_node center; read timeline years from frontmatter, not get_any - expand_node filtered chat tier only on the neighbour, not the queried center. A chat-scoped center whose neighbour id was re-upserted by a note/source would return (and ground) a chat-asserted relationship. Apply the predicate to `e` in both directed queries. - timeline resolved each document's year via vault.get_any(slug), which walks the whole vault in _find_md() and builds a full node model just for `year` -- up to hundreds of times under the Kùzu lock. New VaultService.frontmatter_for_relpath() resolves the already-known relative path directly and reads only the file's head.
… unbounded-work, citation-slug Self-audit against the recurring Copilot-review defect classes surfaced sibling bugs I'd only ever patched at the flagged line: Robustness - Every kg_queries scan (search, compute_top_entities, god_nodes, expand_node, authors, surprising_connections, timeline) iterated its result stream *outside* the try that guards conn.execute(). A Kùzu error mid-scan propagated out of the daemon thread via _loop -> _drain_once -> _process_pending -> _refresh_* (which, unlike _full_index, had no try/except) and silently stopped all incremental indexing. Loops moved into the try; _loop now catches per-cycle. Chat-tier boundary - vault_health's connectivity scan had no trust_tier filter, so a note entity sharing an id with a chat-edge participant counted as connected and was dropped from the orphan list. Filter both endpoints, like every other scan. Unbounded work - timeline held the sole Kùzu lock through up to ~2000 frontmatter reads. Split into timeline_scan (locked, Cypher) + timeline_build (unlocked, vault I/O). - expand_node had no result bound -- a hub id returned thousands of entities/edges under the lock. Added `limit` (route bound 1..EXPAND_MAX), propagated through client/kg_app/graph_routes. - source_reader section/literal modes read the whole file; a large imported PDF->MD loaded tens of MB into a worker. Capped at _MAX_SCAN_CHARS. Citation slug - _search_vault (SEARCH_VAULT) and query() (GRAPH_CONTEXT) derived the citable slug as Path(source_file).stem, dropping the directory -- the bug round 7 fixed elsewhere and made slug_for_relpath() for. Now compound. Cleanup - expand_node dropped a self-loop edge that matched both directed queries. - Removed dead _compute_top_entities wrapper (tests repointed to the _refresh_top_entities/top_entities path). Ran docs/diagrams/gen.sh (no diff -- the hand-maintained diagram sources model at a coarser level than routers/helper modules).
Classified all 36 review threads into 10 defect classes, re-audited HEAD
against each (see the reply for the full write-up). Two more instances:
- _search_vault read the whole vault document into memory for a 2000-char
excerpt -- same class as round 2's read_source summary fix, in a sibling.
Bounded read now.
- /graph/expand_node?id= and /graph/timeline?q= had no max_length, so a
pathological value became a giant Cypher param / thousand-token substring
scan under the sole Kuzu lock. Bounded at 512 (also /notes/{slug}/read's
query).
Audit conclusions with no code change (stated so they aren't "fixed" wrong
later): surprising_connections' a1_src == a2_src exclusion is sound (a
shared last-writer means that document node-upserted both concepts, so it
does mention both); god_nodes/authors/vault_health full-scan-under-lock and
entities_for_file's missing tier filter are known and deliberately deferred.
The recurring defect classes from PR #104's review marathon, as an in-repo checklist. Referenced from .claude/CLAUDE.md's read-before-coding list so it's the first thing consulted before touching kg_queries.py, the /graph/* routes, source_reader, or the chat grounding tools.
…onstants Item 1 of the deferred list: these three /graph/* endpoints did full unbounded Cypher scans on the live request path under the sole Kuzu lock (vault_health worst: two scans, no limit param). Now cache-only reads, same pattern as top_entities/surprising_connections: - _god_nodes_cache / _authors_cache / _vault_health_cache, each with a _refresh_* that scans + publishes under one lock hold. - One _refresh_derived_caches() drives all five, called from every _process_pending / _full_index / rename cycle; drop_index() resets all. - vault_health gains a `limit` (slices `orphans`, keeps `orphan_count` accurate) propagated graph_routes -> client -> kg_app -> service. - GOD_NODES_MAX / AUTHORS_MAX / VAULT_HEALTH_MAX / DEFAULT_AUTHORS shared constants; kg_app.py's route defaults now reference kg_queries constants instead of bare literals (item 3). Item 2 (entities_for_file tier filter): left unfiltered on purpose -- addressed by exact source_file (no join, no id-collision path), only reached from /admin/kg, never a citation path, and a chat-tier filter would break inspecting a chat file's own extraction. Rationale now in the function docstring. The full-graph-scan-under-lock line in docs/kg-retrieval-review-checklist.md updated to "all five are cache-only".
There was a problem hiding this comment.
🟡 Changes recommended
Several new public retrieval paths remain unbounded, and chat-tier filtering and asynchronous UI state handling need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
prisma/services/source_reader.py:96
available_sectionsis not bounded. A document containing many headings—or very long heading lines—can make section mode return nearly the whole file through this list despite the API's bounded-response contract. Cap the heading count and per-heading/total characters, and report truncation just as literal mode does.
ui/src/routes/+page.svelte:1115- Capture the active chat slug before this request and re-check it before writing the result. As written, switching chats while the fetch is in flight lets the old request populate
graphToolResultin the newly opened chat (and completed results also persist acrossopenChat); nearby chat requests guard post-awaitupdates by slug. Reset or scope the result/loading state when opening another chat as well.
prisma/server/graph_routes.py:67
vault_healthreturns the complete orphan list without a limit or pagination contract. In a sparse or partially indexed large vault this can serialize most of the entity table into one public response; keeporphan_countas the total but return a bounded page/sample oforphansand propagate the paging parameters through the client and worker.
@router.get("/vault_health", response_model=VaultHealthResponse)
def vault_health():
"""First-cut vault health: entities with no relationship edges."""
return get_client().vault_health()
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Incremental work can be lost after failures, worker limits are bypassable, and several grounding and truncation paths can report incomplete data as reliable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
prisma/services/chat_tools.py:527
- The text includes every hub even though the global
slugscheck succeeds when only one hub has provenance. A source-less hub is then exposed as grounded by unrelated documents. Filter the displayed hubs to entries with nonemptysource_files(while retaining full data inrawif needed).
lines = [
f"- {e.label} ({e.degree} connections)"
+ (f" — e.g. {', '.join(e.sample_relations)}" if e.sample_relations else "")
for e in entities
prisma/services/chat_tools.py:550
- A connection is rendered even if one or both hop source files are missing; the global slug list can be made nonempty by the other hop or another connection, so the uncitable two-hop claim is still classified as grounded. Only render connections for which both hop provenances are present.
lines = [
f"- {c.entity_a} --[{c.relation_a}]-- {c.bridge} --[{c.relation_b}]-- {c.entity_b}"
for c in links
]
raw = [c.model_dump() for c in links]
- Files reviewed: 26/26 changed files
- Comments generated: 7
- Review effort level: Balanced
- Grounding tools (expand_node/god_nodes/surprising_connections): rendered every row but only cited the ones with a source_file -- an uncitable row under another's `Sources:` header misattributes it. Now render + cite the same citable-only subset. - kg_app.py worker routes: the worker binds to a host and is directly reachable, so every route now carries the /graph router's ge/le/ max_length bounds -- "the client passes a sane value" was not a defence. - _drain_once: the queue is .clear()ed before processing, so an exception caught by _loop() silently dropped the batch. Re-queue everything and mark stale before re-raising. Same fix applied to chroma_service._loop, which had the original loop-dies-on-exception bug. - _refresh_surprising_connections: hub exclusion read the 15-slot priming cache, so the 16th+ hub stayed eligible as a bridge. New kg_queries.hub_ids() returns every entity of degree >= _HUB_MIN_DEGREE. - source_reader section/literal: read _MAX_SCAN_CHARS + 1 to detect the file continued past the cap; propagate `truncated` into both modes so a miss past the boundary isn't mistaken for exhaustive. - UI runGraphTool: capture the chat slug, recheck after the await before writing graphToolResult; clear it on chat switch (single-flight graphToolLoading always released). docs/kg-retrieval-review-checklist.md updated: validate at every HTTP boundary; degree thresholds from the whole graph not the priming cache; catch-all around a drain loop must re-queue; read N+1 to detect a capped read; truncated flag across all modes.
…tities bound Two non-functional cleanups from a full re-review of the #104 diff: - kg_queries.py: the DEFAULT_TIMELINE/TIMELINE_MAX comment still described timeline() reading frontmatter under the Kùzu lock. KnowledgeGraphService. timeline() splits the scan (locked) from timeline_build() (lock released) precisely so it doesn't -- reword to why the hit-count bound still matters (it caps the follow-on per-document I/O). - kg_app.py /top_entities: bounded le= by kg_queries.DEFAULT_TOP_ENTITIES, which only coincidentally equals the cache size it actually needs to match. Bind it to knowledge_graph_service.TOP_ENTITIES_CACHE_SIZE (the real source of truth -- the cache holds exactly that many rows) so the two can't drift.
…ly needs Follow-up to the /top_entities fix in the previous commit: record the defect class (a bound referencing a coincidentally-equal constant instead of the one it actually tracks) so it's checked on the next route added.
The public /graph/* router rejects an empty ?id=/?q= (min_length=1); the kg worker's mirrored routes only capped max_length, so an empty required param passed validation there. The worker binds to a host and is directly reachable, so it's a real second boundary -- align it, and extend the same floor to the diagnostic routes (/search, /ranked_nodes, /query, /entities_for_file) whose required param is never legitimately empty. Behaviour was already harmless (kg_queries returns empty on a falsy id/terms), but "validate identically at every boundary" is the point.
…ram for param Records the class behind the previous commit: the worker route mirrored max_length but not min_length, and "the query fn returns empty anyway" was treated as covering the gap.
There was a problem hiding this comment.
🟢 Approval recommended
The retrieval surfaces are consistently bounded, provenance-aware, synchronized, and comprehensively covered by focused tests.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Per-section extraction, god_nodes, surprising_connections, expand_node, and the get_full_text/read_source bounded-excerpt case were all still marked '[ ]'/'not built yet' despite shipping in PR #104. Only suggest_questions and the consultation-sub-agent redesign remain genuinely unbuilt -- narrowed the framing to just those.
Knowledge-graph retrieval: dual-exposed capabilities (chat + REST)
Adds 7 new knowledge-graph retrieval capabilities, each usable both from the chat tool loop and
a public REST surface (
/graph/*,/notes/{slug}/read) — a foundation for a future MCP serveras well. See
docs/wiki/architecture.mdfor the existing 4-layer pattern(service →
kg_app.py→KnowledgeGraphClient→ caller) this extends.New capabilities
expand_node— one-hop traversal from a specific entity id, direction preserved per edge.god_nodes— most-connected hub entities across the vault, with source file + samplerelations.
surprising_connections— links between entities that emerge from the graph itself, withno single document ever asserting them directly (background-computed, cached).
authors— distinct entity authors grouped by how many source files name each (REST only).vault_health— entities with zero relationship edges (REST only).timeline— entities matching a query, joined to their source's publication year, sortedchronologically (REST only).
read_source— bounded, addressable reads of a vault document's own raw text(
summary/section/literalmodes) — never the whole file at once.Refactors this required
kg_queries.pyout ofknowledge_graph_service.py— pure retrieval functions, noextraction/indexing/HTTP awareness, so most future retrieval work only needs this bounded file
in view.
ChatToolbox/ToolSpecdispatch is now generic (handler/groundingfields resolved viagetattr) instead of a hardcoded if/elif chain and a hand-maintained grounding-tool set.graph_routes.pypublic router, distinct from the diagnostic/admin/kg/*surface.model ever decides to invoke the matching tool.
Review history
Addressed 7 rounds of automated review on this PR: path traversal in compound-slug decoding,
ReDoS risk in the original regex-based
read_sourcemode (replaced with literal matching),unbounded response size,
surprising_connectionsbridging by entity id instead of label (everydocument mints its own id namespace, so the same concept never shares an id across documents),
edge-direction bugs in
expand_node, and grounding-detection bypasses across several toolhandlers.