Rename ontology doc, fix TODO staleness, add suggest_questions + stream-triage-by-graph-relevance - #106
Merged
Conversation
English-only in a public repo (the file's content was always English -- just the filename was Spanish). Updated every reference across concept docs, TODO.md, ADR-017, .claude/CLAUDE.md, and vault_models.py's CitedClaimNode docstring; regenerated the 3 committed JSON schemas that embed that docstring. Full suite 1290 passed (this branch's baseline, off main).
# Conflicts: # .claude/CLAUDE.md
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.
…h-relevance Phase B, increments 2 and 3 of 4 (Toulmin shipped in #105; cross-chat entity linking deferred to its own session -- see the plan discussion). suggest_questions: same 4-layer pattern as god_nodes/surprising_connections (kg_queries.suggest_questions -> KnowledgeGraphService cache -> kg_app.py route + client -> chat_tools ToolSpec/handler). Phrases a grounded question per RelatesTo edge, deduped by entity pair, capped at one question per source_file before filling remaining slots. Stream triage: a lightweight text-overlap score (KnowledgeGraphService. graph_relevance, backed by a cached distinct_entity_labels() scan) against already-extracted KG entity labels -- no stream/Zotero content is ever indexed into Kùzu, keeping docs/concepts/stream.md's stated boundary intact. New GET /zotero/items/relevance endpoint scores and sorts the existing item listing; a new UI toggle in the Zotero browser panel calls it and renders a small match-count badge. Both the new /graph_relevance route (directly reachable on the kg worker) and its zotero_routes.py caller share one bound (kg_queries. GRAPH_RELEVANCE_MAX_TEXTS/_MAX_TEXT_LENGTH) rather than two literals that would drift. TODO.md's now-fully-shipped tool list corrected; docs/concepts/ zotero-item.md documents the new endpoint and its relationship to stream.md's KG-pollution boundary.
CServinL
changed the title
…view
graph_relevance() used a bare substring check with no word-boundary --
any short entity label ("AI", "US", "ROC") matched inside an
unrelated longer word ("explain", "custom", "process"). Switched to
compiled word-boundary regex, precomputed once per call instead of once
per text (also fixes an O(texts x labels) re-lowering cost). Also
dedupes case-variant labels ("Neural Networks"/"neural networks" from
two different documents) so one concept isn't double-counted.
suggest_questions() deduped by entity id, not label -- despite
surprising_connections() two functions above it existing specifically
because same-document-scoped ids mean the same real-world concept pair
gets a different id per document. The same question text came out once
per paper discussing it, not once total. Fixed to dedupe by normalised
label pair, same as surprising_connections already does.
None of this was caught by the original test suite or self-audit,
because every original test was shaped like the code's own assumptions
(small single-label fixtures, whole-word test strings, single-document
fixtures) -- confirming internal consistency, not correctness. Pattern
persisted to the review checklist.
…abels The previous fix for the substring false-positive bug switched to plain \b word-boundary regex -- but \b requires a \w/\W transition, so it silently fails to match any label that itself starts or ends with punctuation: ".NET", "Ph.D.", "C++", "e.g.", "U.S." never matched even when the text said them verbatim. Switched to (?<!\w)...(?!\w), which asserts "not adjacent to a word character" regardless of what the label's own edge characters are. Caught by deliberately probing the fix's own edges in a second adversarial pass, not by the tests written alongside the first fix -- persisted to the checklist as its own lesson: fixing one instance of "tests shaped like my own assumptions" doesn't fix the habit.
Confidence + degree (moderate weight, DEGREE_WEIGHT=0.15 via log1p so a mega-hub can't drown out a confident edge between two obscure entities), per user's decision after weighing options. Both signals were already extracted per-edge/entity data, unused by this tool despite surprising_connections ranking by the same confidence_score field two functions above it. The one-per-document diversity guarantee is preserved deliberately: the guaranteed-slot selection (each document's best-ranked edge) and the leftover fill are ranked independently and never re-sorted together -- doing so would let a document with many high-ranked edges push a document with exactly one modestly-ranked edge out of the result entirely. Two of the three new tests initially passed on both old and new code for unrelated reasons (insertion-order coincidence; a guarantee that was never actually at risk from that test's shape) -- caught by checking each test against a plausible wrong alternative, not just against pre-fix history. Persisted as its own checklist lesson.
…indings 1. suggest_questions() deduped cross-document concept pairs during the raw scan, before ranking existed -- whichever document Kùzu visited first won the slot regardless of confidence_score, contradicting the function's own docstring. Fixed: score every row first, then dedupe by keeping each pair's best-ranked row. 2. _upsert() wrote confidence_score/weight via `e.get(k, default) or default` -- 0.0 is falsy in Python, so a genuinely-zero score was laundered into 0.5 (or 1.0 for weight) before it was ever persisted. This defeated the ranking fix above at the write side; proven by a test that failed even after the read-side default was fixed correctly. 3. suggest_questions()' degree count didn't exclude self-loops -- a self-referential edge is emitted twice by the undirected scan with the same id on both ends, inflating that entity's degree by +2 for zero real connectivity. 4. zotero_items_relevance() interpolated item.title with no None-guard, unlike item.abstract_note on the same line -- a titleless item scored the literal string "None" against KG entity labels. 5. graph_relevance() recompiled every cached label's regex from scratch on every call; a caller can invoke it several times per page load (one call per item batch). Patterns are now precompiled once per _refresh_entity_labels() cycle instead. Rejected as unverified: a claimed whitespace-run dedup gap in label matching (doesn't reproduce -- two label variants with different internal whitespace can't both match the same single occurrence of text); a claim that docs/diagrams weren't regenerated (verified false, zero diff, no diagram source references any of the new tool names). Persisted as new checklist items: ranking added after a filter existed means every pre-existing filter needs re-auditing for order-dependence; a falsy-default fix on one side of a field (read) doesn't cover the other side (write) of the same bug.
Previously each method only degraded on requests.RequestException (network-level failure) -- a Pydantic ValidationError on model_validate(), or an AttributeError/TypeError from .get() on a response that parsed as JSON but wasn't the expected shape, propagated as an unhandled exception through this client into whatever called it. Fixed consistently across every method (not just graph_relevance, which is where this was first flagged and deliberately deferred to be fixed everywhere at once) via a shared _safe() wrapper, each call site keeping its own already-defined degrade value. _get()/_post() also now catch a non-JSON 200 body (json.JSONDecodeError, a ValueError subclass) -- the HTTP call succeeds but resp.json() itself fails, the same 'reachable but unusable' class _safe() covers one layer up. 6 new regression tests, one per distinct degrade shape (list, single object, bool via .get(), the unreachable-KGStatus default, and graph_relevance's length-preserving zero-score degrade), plus the non-JSON-body case at the _get() level. All confirmed red on pre-fix code.
…coverage gap
_safe()'s except tuple didn't include ValueError, so clear_dead_letters()'s
int(data.get("removed", 0)) still raised uncaught on a non-numeric field
-- the exact bug class that commit existed to close, just missed for one
conversion. graph_relevance() also had no length-mismatch check: every
item in a wrong-length response can validate individually with nothing
raised, silently breaking the positional-correspondence guarantee the
surrounding comment promises.
Separately: suggest_questions was missing from two pre-existing
parametrized tests that exist specifically to catch a broken marker/regex
for every tool (test_new_tools_advertised_in_system_prompt,
test_tool_call_re_matches_new_markers) -- found by grepping the test file
for the marker, not by writing suggest_questions' own tests.
…est race graph_routes.py's own docstring states every Phase-A Kùzu-backed capability gets a public route here regardless of whether it also has a chat ToolSpec -- suggest_questions had a client method, a kg_app.py internal route, and a ToolSpec, but no public route, making it unreachable outside the chat loop. Added, matching the existing pattern exactly (same DEFAULT_TOP_ENTITIES/SUGGEST_QUESTIONS_MAX bound as /graph/god_nodes and /graph/surprising_connections). Note: read_source has the same pre-existing gap from an earlier PR, not touched here. loadZoteroItems() had no request sequencing between overlapping calls (collection click, debounced search, and now the relevance toggle) -- pre-existing, but the relevance toggle's kg-round-trip path has a materially different latency than plain /items, making the race meaningfully more likely to surface a stale/mismatched item list. Fixed with a last-request-wins sequence counter. Two findings from this review round rejected: batching graph_relevance calls sequentially in zotero_routes.py is real but not fixed here -- concurrent HTTP calls would require a broader move off the synchronous requests library, out of scope for a review-fix pass. Compiling regex patterns under the lock in _refresh_entity_labels() is correct as-is, not a bug -- the suggested fix would reintroduce the exact compute-and-publish-must-be-atomic race the checklist's own item 4 exists to prevent for a background-refreshed cache.
… rename Same TODO.md line as this branch's docs/ontologia.md -> docs/ontology.md rename also carried an adjacent dead link -- footnote.md was renamed to claim.md in an earlier, unrelated PR (the Footnote/FootnoteRelation -> CitedClaimNode/InferenceNode split), and nothing caught the leftover reference since it wasn't part of that rename's own scope.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues remain in relevance request handling, question grounding and provenance, and Zotero UI state and display.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds suggest_questions, Zotero graph-relevance triage, and related documentation, schema, API, UI, and test updates.
Changes:
- Adds cached KG question suggestions across REST and chat.
- Adds relevance scoring, sorting, and badges to Zotero items.
- Renames ontology documentation and updates stale references and TODO status.
- Expands coverage for services, routes, clients, schemas, and UI behavior.
File summaries
| File | Summary |
|---|---|
ui/src/routes/+page.svelte |
Adds Zotero relevance sorting and badges. |
TODO.md |
Marks shipped KG capabilities complete. |
tests/unit/services/test_knowledge_graph_service.py |
Tests service caching and relevance scoring. |
tests/unit/services/test_knowledge_graph_client.py |
Tests KG client methods and resilience. |
tests/unit/services/test_kg_queries.py |
Tests question generation and label queries. |
tests/unit/services/test_chat_tools.py |
Tests chat tool behavior. |
tests/unit/server/test_zotero_routes.py |
Tests the Zotero relevance route. |
tests/unit/server/test_kg_app_bounds.py |
Tests KG request bounds. |
tests/unit/server/test_graph_routes.py |
Tests the public suggestion route. |
tests/unit/agents/test_chat_agent.py |
Updates context-window regression coverage. |
schemas/turn-node.schema.json |
Updates ontology references. |
schemas/cited-claim-node.schema.json |
Updates ontology references. |
schemas/chat.schema.json |
Updates ontology references. |
prisma/storage/models/vault_models.py |
Updates the ontology docstring reference. |
prisma/storage/models/kg_models.py |
Adds suggestion and relevance models. |
prisma/services/knowledge_graph_service.py |
Adds caching and relevance scoring. |
prisma/services/knowledge_graph_client.py |
Adds KG client methods and degradation handling. |
prisma/services/kg_queries.py |
Adds suggestion and entity-label queries. |
prisma/services/chat_tools.py |
Registers and renders suggest_questions. |
prisma/server/zotero_routes.py |
Adds relevance-scored Zotero items. |
prisma/server/kg_app.py |
Adds bounded KG worker endpoints. |
prisma/server/graph_routes.py |
Adds the public suggestion endpoint. |
docs/wiki/adr/ADR-017-claim-attribution-and-footnote-model.md |
Updates ontology references. |
docs/ontology.md |
Renames the ontology document. |
docs/concepts/zotero-item.md |
Documents relevance triage. |
docs/concepts/zotero-collection.md |
Updates ontology references. |
docs/concepts/wiki-link.md |
Updates ontology references. |
docs/concepts/transclusion.md |
Updates ontology references. |
docs/concepts/stream.md |
Updates ontology references. |
docs/concepts/source.md |
Updates ontology references. |
docs/concepts/note.md |
Updates ontology references. |
docs/concepts/literature-review-report.md |
Updates ontology references. |
docs/concepts/graph-node.md |
Updates ontology references. |
docs/concepts/claim.md |
Updates ontology references. |
docs/concepts/citation.md |
Updates ontology references. |
docs/concepts/chat.md |
Updates ontology references. |
Review details
Suppressed comments (3)
prisma/server/zotero_routes.py:211
get_all_items()/get_collection_items()return the full library/collection, whilegraph_relevance()has a 2-second timeout. This synchronous loop therefore adds one KG round-trip per 200 items; a large library or an unavailable KG can make one/items/relevancerequest takeceil(item_count / 200) * 2sbefore returning. Add a bounded bulk path or fail-fast/cap behavior so a relevance outage does not block the whole Zotero listing.
for i in range(0, len(texts), kg_queries.GRAPH_RELEVANCE_MAX_TEXTS):
scores.extend(get_indexer().graph_relevance(texts[i:i + kg_queries.GRAPH_RELEVANCE_MAX_TEXTS]))
prisma/services/chat_tools.py:629
- The header aggregates all source slugs, but this line does not render each question's
grounding_source_filenext to its question. With multiple questions from different documents, the model cannot tell which source grounds which follow-up and may attach a later claim or footnote to the wrong document; include the per-question slug in each line (the structuredrawmapping is not what the model sees).
lines = [f"- {q.question}" for q in citable]
prisma/services/chat_tools.py:13
- This module docstring still says that only
search_vaultandgraph_contextare implemented, which contradicts the new sentence immediately below and the retrieval handlers added in this PR. Leaving that stale statement in the module header will mislead maintainers about which tools are available.
Only search_vault and graph_context are implemented for this first
increment — TODO.md's design also sketches expand_node, get_full_text,
god_nodes, surprising_connections, suggest_questions; only get_full_text's
consultation-sub-agent redesign remains deferred, the rest are built.
- Files reviewed: 35/36 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…red issue
- openStream() bypassed loadZoteroItems() entirely (direct fetch,
no zoteroRequestSeq participation, ignored zoteroSortByRelevance) --
opening a stream while relevance-sort was active silently showed
unsorted, unbadged results, and could race with an in-flight
loadZoteroItems() call with no coordination. Routed through the
shared helper instead of duplicating the fetch.
- The relevance match-count badge never rendered for a genuine
zero-score item (0 is falsy in Svelte's {#if}) -- same falsy-zero
defect class already fixed twice this session in Python, now caught
in the UI too. Checked !== undefined instead.
- The relevance toggle button had no aria-pressed, so its active state
was only conveyed visually.
The 4th finding (a chat-tier-drifted edge's source_file being citable
even after its endpoints' trust_tier relabels non-chat via an id
collision) is real, but it's the same already-known, already-deferred
issue TODO.md tracks as 'KG entity ids aren't directory-unique' --
god_nodes/surprising_connections already share the identical exposure,
suggest_questions just inherits it. Documented the citability angle
there rather than a one-off fix in this one function.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Suggested questions lose per-question source provenance, and the chat-tools documentation is contradictory.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
prisma/services/chat_tools.py:632
- The model-facing text loses the one-to-one provenance carried by
SuggestedQuestion: all slugs are merged into one header while each question line omits its own source. With questions from multiple documents (and especially leftover questions that reuse a document), the model cannot tell which slug grounds a selected question and may attach the wrong footnote. Include the corresponding compound slug on every question line while retaining the aggregateSources:header.
prisma/services/chat_tools.py:13 - The module docstring still says only
search_vaultandgraph_contextare implemented, then says the other tools are built. Update the opening sentence so the documentation no longer contradicts the registry immediately below.
- Files reviewed: 35/36 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Chats never reach the knowledge graph at all today (.sess, not .md/.txt -- KnowledgeGraphService._full_index() only walks index_extensions), so trust_tier=='chat' never actually occurs on a real entity/edge. And the KG's trust_tier filter was never what enforced 'chats aren't sources' in the first place -- that's independently guaranteed by chats' storage format, ChromaIndexer's explicit chats/ exclusion, and RECALL being the one deliberately separate, non-citable path for chat content. This is a gap in a redundant backup layer for a rule two other mechanisms already hold, not a threat to the rule itself. My first write-up of this (previous commit) overstated it as a live citability leak.
1610 -> 109 lines. TODO.md had become a chronological engineering journal spanning months -- resolved/historical narrative mixed in with genuinely open items, most checkboxes already stale by the time this session started (several already fixed by PRs #104/#105/#106, confirmed against the actual codebase rather than trusted at face value). Kept only what's still genuinely pending, verified against current code for each ambiguous item (chat-tier label, deletion cascade, graphify migration, streaming, container build, chart wiring, etc.) rather than assuming the checkbox state was accurate. Also dropped personal/private details incidental to the historical narrative (nothing left that belongs in a public repo's backlog). Cross-references to now-removed TODO.md sections fixed where they made an active, findable claim (config_reload.py, its test, ADR-004, ADR-008, ADR-015's header) -- either redirected to the code/ADR that's the real durable home, or the essential point inlined directly since the pointer was the only thing carrying it. Git history has the full removed content if any of it is ever needed again; docs/logs/ remains the home for investigation-log-shaped writeups, which is what the user asked TODO.md to stop doing.
ADR-004 (2025-09-14) originally had a code block demonstrating an early, since-superseded CLI design where stream updates would run as tracked SQLite jobs (status/results commands checking job state) -- never actually implemented; streams still run synchronously today, no job system exists. The 2026-07-27 'CLI minimized' follow-up section got inserted in the middle of that original code block, swallowing its opening fence and heading but leaving the tail two function stubs + the orphaned closing fence dangling right after the new prose. Removed -- nothing here described anything that was ever built.
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
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.
Bundles several small/medium pending items together on this branch (user's call, to avoid micro-PRs) rather than as separate PRs:
Rename
docs/ontologia.md→docs/ontology.md— English-only in a public repo; the content was always English, just the filename was Spanish. Every reference updated across concept docs,TODO.md, ADR-017,.claude/CLAUDE.md, andvault_models.py'sCitedClaimNodedocstring; regenerated the 3 committed JSON schemas that embed that docstring.TODO.mdstaleness fix —god_nodes/surprising_connections/expand_node/read_source/per-section extraction were all still marked[ ]/"not built yet" despite shipping in PR Add knowledge-graph retrieval tools: expand_node, god_nodes, surprising_connections, authors, vault_health, timeline, read_source #104.Phase B, increment 2 —
suggest_questionschat tool. Same 4-layer patterngod_nodes/surprising_connectionsalready use (kg_queries.suggest_questions→KnowledgeGraphServicecache →kg_app.pyroute + client →chat_toolsToolSpec/handler). Phrases a grounded question perRelatesToedge, deduped by entity pair, capped at one question persource_filebefore filling remaining slots — so no single document crowds out the rest.Phase B, increment 3 — stream triage by graph relevance (lightweight design). A pure text-overlap score (
KnowledgeGraphService.graph_relevance, backed by a cacheddistinct_entity_labels()scan) against entity labels already extracted into the knowledge graph — no stream/Zotero content is ever indexed into Kùzu, keepingdocs/concepts/stream.md's stated "streams shouldn't pollute the knowledge graph" boundary intact. NewGET /zotero/items/relevanceendpoint scores and sorts the existing item listing; a new "Sort: Relevance" toggle in the Zotero browser panel calls it and renders a small match-count badge per item.Phase B's 4th increment, cross-chat entity linking, is explicitly deferred to its own session — it has no prior design anywhere in the repo and would require either extending KG entity extraction to chat content (reversing the deliberate "chats are not sources" decision) or inventing a new chat-scoped entity concept, on top of the cross-chat
RECALLmechanism that already does topical-similarity search across chats.Full suite: 1351 passed. UI build clean. Every new test confirmed red on pre-change code before the fix.