fix(kg): port source_drawer_id provenance with schema migration (#1314)#6
Merged
Merged
Conversation
Follow-up to PR #5. Ports the schema-changing part of upstream e4e25ed (MemPalace/mempalace#1314, RFC 002 §5.5) that was explicitly deferred because it requires an ALTER TABLE on the triples table. - Add source_drawer_id TEXT and adapter_name TEXT columns to the canonical triples CREATE TABLE. - Add KnowledgeGraph::migrate_schema: introspect PRAGMA table_info(triples) and ALTER TABLE ADD COLUMN only when the columns are missing, so legacy palaces are migrated in-place on open() and fresh installs stay no-op. - Extend KnowledgeGraph::add_triple with source_drawer_id and adapter_name Option<&str> kwargs; existing test call sites pass None. - Surface source_drawer_id on Triple and EntityQueryResult; surface adapter_name on Triple so callers can navigate back to the drawer that produced the fact. - Wire tool_kg_add at the MCP boundary to accept source_drawer_id and forward it; advertise it in the tool input schema. adapter_name remains internal, matching upstream. - Regression tests: unit tests for round-trip persistence, query exposure, and the legacy-schema migration path, plus an MCP-level test that the drawer id reaches SQLite via tool_kg_add. - port.txt: drop the now-resolved 'source_drawer_id' gap and append a new sync update entry.
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.
Summary
Follow-up to #5. Ports the schema-changing part of upstream
e4e25ed(milla-jovovich/mempalace#1314, RFC 002 §5.5) that the previous PR explicitly deferred because it requires anALTER TABLEon thetriplestable. With this PR, the gap entry forsource_drawer_idinport.txtis now resolved.What changed
triplesnow declaressource_drawer_id TEXTandadapter_name TEXTin the canonicalCREATE TABLE. For palaces created before these columns existed, a newKnowledgeGraph::migrate_schemaintrospectsPRAGMA table_info(triples)and only issuesALTER TABLE ADD COLUMNwhen the column is missing — mirroring the upstream_migrate_schemano-op-on-new-installs contract. Fresh palaces pay no cost; legacy palaces are migrated in-place onopen().KnowledgeGraph::add_triplegains two trailingOption<&str>kwargs (source_drawer_id,adapter_name), defaulting toNoneso all existing call sites stay source-compatible.TripleandEntityQueryResultexposesource_drawer_id;Triplealso exposesadapter_name. New fields use#[serde(default, skip_serializing_if = "Option::is_none")]so existing JSON consumers stay backward compatible.tool_kg_addaccepts a newsource_drawer_idinput and forwards it to the KG layer; the tool input schema advertises the property with a description that references RFC 002 §5.5.adapter_nameis intentionally not exposed at the MCP boundary — matching upstream, where only adapters set it.knowledge_graph::tests::test_add_triple_persists_source_drawer_and_adapter— round-trip persistence of both new columns.knowledge_graph::tests::test_query_entity_exposes_source_drawer_id—query_entityandtimelinesurface the drawer id.knowledge_graph::tests::test_migrate_schema_adds_missing_provenance_columns— creates a legacytriplestable without the new columns and verifiesKnowledgeGraph::openmigrates it and that subsequent writes use the migrated columns.mcp_server::tests::test_kg_add_forwards_source_drawer_id— mirrors upstreamtest_kg_add_forwards_source_provenance; reads the raw row to confirm the drawer id reaches SQLite via the MCP boundary.source_drawer_identry from "Remaining gaps" and appends a follow-up sync entry documenting what was ported.Review & Testing Checklist for Human
PRAGMA table_info(triples)shows the newsource_drawer_idandadapter_namecolumns afterKnowledgeGraph::openruns. The migration is idempotent — runningopen()twice is a no-op on the second call.Triple/EntityQueryResultJSON should still see no breaking change because the new fields areOption<String>withskip_serializing_if = "Option::is_none". Spot-checkmempalace_kg_queryoutput for facts written without the new fields.mempalace_kg_addschema —source_drawer_idshould be visible as an optional string property intools/listoutput.Test plan to verify end-to-end:
cargo build --workspacecargo test --workspace— expect408 passed; 0 failed(4 new regression tests inmempalace-core).cargo clippy --workspace --all-targets -- -D warnings— expect clean.cargo fmt --all -- --check— expect clean.mempalace_kg_addwithsource_drawer_id="drawer_xyz", then query thetriplesrow directly via SQLite and verify the column was persisted.mempalace_kg_queryand confirmsource_drawer_idappears in the response.Notes
adapter_nameis not exposed at the MCP boundary in this PR. Upstream only setsadapter_namefrom inside adapter code that advertisessupports_kg_triples; exposing it at the MCP boundary would let arbitrary callers spoof adapter identity. The column exists at the storage layer and is plumbed throughTriple, but the MCP tool only forwardssource_drawer_id.add_tripletest call sites were padded mechanically (all existing calls used 8 args; the two new kwargs default toNone). No test semantics changed.