feat: KG standard tables — matches Convex kgSpec.ts#46
Conversation
Add standardized knowledge graph schema to BrainLayer's SQLite: - kg_entities: canonical_name, description, confidence, importance, valid_from/until, group_id - kg_relations: fact, importance, valid_from/until, expired_at, source_chunk_id - kg_entity_chunks: mention_type (explicit/inferred/embedding_match) - kg_current_facts VIEW (auto-filters expired relations) - New methods: soft_close_relation, get_current_facts, traverse (2-hop CTE), resolve_entity - Shared constants: ENTITY_TYPES, RELATION_TYPES, DECAY_CONSTANTS, effective_score() - 41 new tests in test_kg_standard.py, all backward compatible Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR extends the Knowledge Graph system with standardized constants, time-decay scoring, and enhanced entity/relation management. Introduces entity and relation type definitions, adds time-based decay scoring, expands the KG schema with validity windows and metadata fields, and provides new traversal and resolution utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| WHERE r.depth > 0 | ||
| ORDER BY r.depth, e.name | ||
| """ | ||
| params_list = [entity_id, entity_id] + relation_types + [max_depth] |
There was a problem hiding this comment.
Traverse parameter order wrong with relation_types filter
High Severity
When relation_types is provided, params_list puts the relation type strings before max_depth, but the SQL query's WHERE re.depth < ? placeholder expects max_depth as the third parameter. The actual ordering in the query is: entity_id, entity_id, max_depth, then the IN (?, ...) relation type placeholders. But params_list = [entity_id, entity_id] + relation_types + [max_depth] puts relation types third and max_depth last. This causes a string to be compared against depth, completely breaking traversal when relation_types is specified.
Additional Locations (1)
| # 1. Exact alias | ||
| result = self.get_entity_by_alias(name_or_alias) | ||
| if result: | ||
| return result |
There was a problem hiding this comment.
Alias resolution returns dict missing new standard fields
Medium Severity
resolve_entity calls get_entity_by_alias as its first resolution step and returns the result directly. But get_entity_by_alias wasn't updated — it only returns 6 fields (id, entity_type, name, metadata, created_at, updated_at), missing the 7 new standard fields (canonical_name, description, confidence, importance, valid_from, valid_until, group_id). Steps 2–4 of resolution all return the full 13-field dict, so callers get inconsistent dict shapes depending on how the entity was resolved.
Additional Locations (1)
| if relation_types: | ||
| placeholders = ", ".join("?" for _ in relation_types) | ||
| rel_filter = f"AND r.relation_type IN ({placeholders})" | ||
| params = [entity_id] + relation_types + [max_depth] |
There was a problem hiding this comment.
Dead params variable never used in traverse
Low Severity
The params variable is computed (lines 2919–2923) but never used — params_list is what actually gets passed to cursor.execute. This dead code is confusing, especially since params also has an incorrect parameter ordering, which could mislead future maintainers into thinking it's the intended parameter list.
| confidence = excluded.confidence | ||
| confidence = excluded.confidence, | ||
| fact = COALESCE(excluded.fact, kg_relations.fact), | ||
| importance = COALESCE(excluded.importance, kg_relations.importance), |
There was a problem hiding this comment.
COALESCE on importance is ineffective due to default
Medium Severity
In add_relation, importance has a Python default of 0.5 (never None), so COALESCE(excluded.importance, kg_relations.importance) in the ON CONFLICT clause will always use the excluded value. On upsert without an explicit importance, the existing stored importance is silently overwritten with 0.5 instead of being preserved. This contradicts the COALESCE pattern's intent — importance would need an Optional[float] = None default to allow preservation.
| WHERE (valid_from IS NULL OR valid_from <= strftime('%Y-%m-%dT%H:%M:%fZ','now')) | ||
| AND (valid_until IS NULL OR valid_until >= strftime('%Y-%m-%dT%H:%M:%fZ','now')) | ||
| AND expired_at IS NULL | ||
| """) |
There was a problem hiding this comment.
View timestamp comparison breaks with standard ISO 8601 input
Medium Severity
The kg_current_facts VIEW compares user-provided valid_from/valid_until strings against strftime('%Y-%m-%dT%H:%M:%fZ','now'), which produces timestamps with 3 fractional digits (e.g., ...T09:00:00.000Z). SQLite compares TEXT values byte-by-byte. If a user stores a common ISO 8601 format without fractional seconds (e.g., "2026-02-27T09:00:00Z"), the Z character (ASCII 90) at the seconds boundary sorts higher than . (ASCII 46), causing valid_from <= now to return FALSE when the times are at the same second. This silently excludes valid relations or includes expired ones depending on the field.
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Note
Medium Risk
Medium risk because it introduces SQLite schema migrations (new columns, indexes, and a view) and expands KG CRUD/query behavior, which could affect existing databases and downstream consumers if assumptions about returned fields change.
Overview
Adds a standardized KG schema to the SQLite
VectorStorevia migrations: new metadata/validity columns onkg_entitiesandkg_relations,mention_typeonkg_entity_chunks, new supporting indexes, and akg_current_factsview that filters out expired/out-of-validity relations.Extends KG APIs to read/write these standard fields (updated
upsert_entity,add_relation,link_entity_chunk, and expanded getters), and adds new utilities:soft_close_relation,get_current_facts, multi-hoptraverse(recursive CTE), andresolve_entity(alias/name/canonical/FTS fallback). Introducesbrainlayer.kgshared constants pluseffective_score()for time-decayed scoring, and adds/updates tests to cover the new schema and behaviors while asserting backward compatibility.Written by Cursor Bugbot for commit db9fb56. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes