fix: workspace-scoped entity identity — share/federate copy instead of clobbering (#339)

remember()'s identity was (category, key): a cross-workspace write with a
colliding key took the UPDATE path and overwrote the other workspace's row
in place. mimir_share's "clone into target workspace" was therefore a
destructive MOVE of the source entity (source workspace lost it, the fresh
mem-* id was discarded, retrieval stats clobbered), and mimir_federate's
vault_import had the same failure mode for every remapped entity.

Changes:
- Identity is now (category, key, workspace_hash): remember()'s existence
  lookup includes workspace_hash, and the schema's unique index becomes
  idx_entities_category_key_ws (SCHEMA_VERSION 4). The index is created in
  the gated migration block after the workspace_hash ALTER — on a legacy DB
  the ungated DDL runs before that column exists. Create-then-drop so
  uniqueness is never unenforced; safe on populated DBs because the old
  constraint was strictly tighter.
- get_entity(category, key) picks deterministically when the same key now
  exists in several workspaces (global '' first, then lexicographic) instead
  of whichever row SQLite visited.
- forget()'s FTS cleanup uses IN, not a scalar subquery — it archives every
  workspace's row for (category, key), so it must clean FTS for all of them.

Single-workspace vaults (workspace_hash = '' everywhere) behave identically.

Tests: remember_identity_is_workspace_scoped_so_share_copies_instead_of_moving,
migrates_unique_index_to_workspace_scoped_identity.
Suite: 161 passed / 0 failed.

Fixes #339

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: tcconnally

src/db.rs

@@ -1495,10 +1495,16 @@ impl Database {
             entity.body_json.clone()
         };
 
+        // Identity is (category, key, workspace_hash) — #339. Matching on
+        // (category, key) alone made a cross-workspace write with a colliding
+        // key take the UPDATE path and overwrite the other workspace's row in
+        // place: mimir_share's "clone into target workspace" was actually a
+        // destructive MOVE of the source entity. Single-workspace vaults
+        // (workspace_hash = "" everywhere) are unaffected.
         let existing_id: Option<String> = conn
             .query_row(
-                "SELECT id FROM entities WHERE category = ?1 AND key = ?2",
-                params![entity.category, entity.key],
+                "SELECT id FROM entities WHERE category = ?1 AND key = ?2 AND workspace_hash = ?3",
+                params![entity.category, entity.key, entity.workspace_hash],
                 |r| r.get(0),
             )
             .ok();
@@ -2420,8 +2426,14 @@ impl Database {
                     created_at_unix_ms, last_accessed_unix_ms, NULL as embedding,
                     always_on, certainty, workspace_hash, agent_id, visibility,
                     follow_count, miss_count, follow_rate, efficacy_status
-             FROM entities WHERE category = ?1 AND key = ?2 LIMIT 1",
+             FROM entities WHERE category = ?1 AND key = ?2
+             ORDER BY workspace_hash ASC, id ASC LIMIT 1",
         )?;
+        // With workspace-scoped identity (#339) the same (category, key) can
+        // legitimately exist in several workspaces. Callers without a
+        // workspace in hand get a DETERMINISTIC pick: the global ('') row
+        // first, then the lexicographically-first workspace — not whichever
+        // row SQLite happened to visit.
 
         let mut rows = stmt.query_map(params![category, key], |row| {
             entity_from_row(row, self.encryption.as_ref())
@@ -2450,10 +2462,13 @@ impl Database {
              WHERE category = ?3 AND key = ?4 AND archived = 0",
             params![reason, now_ms(), category, key],
         )?;
-        // Clean FTS5 index for archived entity
+        // Clean FTS5 index for archived entity/entities. IN, not `=`: forget
+        // archives every row matching (category, key) — which since #339 can
+        // be one per workspace — so the FTS cleanup must cover all of them,
+        // not just the single rowid the old scalar subquery returned.
         if affected > 0 {
             let _ = tx.execute(
-                "DELETE FROM entities_fts WHERE rowid = (SELECT rowid FROM entities WHERE category = ?1 AND key = ?2)",
+                "DELETE FROM entities_fts WHERE rowid IN (SELECT rowid FROM entities WHERE category = ?1 AND key = ?2 AND archived = 1)",
                 params![category, key],
             );
         }
@@ -6966,6 +6981,52 @@ mod tests {
         let _ = fs::remove_file(&path);
     }
 
+    #[test]
+    fn remember_identity_is_workspace_scoped_so_share_copies_instead_of_moving() {
+        // #339: identity was (category, key), so handle_share's clone into a
+        // target workspace matched the SOURCE row and updated it in place —
+        // a destructive move (source workspace lost the entity, fresh id
+        // discarded, stats clobbered). Identity is now
+        // (category, key, workspace_hash).
+        let (db, path) = temp_db();
+
+        let mut a = make_entity("id-a", "note", "shared-key", r#"{"n":1}"#);
+        a.workspace_hash = "ws-alpha".to_string();
+        db.remember(&a).unwrap();
+
+        // Simulate handle_share's clone into another workspace: same
+        // (category, key), different workspace, fresh id.
+        let mut clone = a.clone();
+        clone.workspace_hash = "ws-beta".to_string();
+        clone.id = "mem-fresh".to_string();
+        let (id, action) = db.remember(&clone).unwrap();
+        assert_eq!(action, "created", "cross-workspace clone must INSERT, not update the source");
+        assert_eq!(id, "mem-fresh");
+
+        // Source untouched in its home workspace; copy exists in the target.
+        let src = db.get_entity_by_id_public("id-a").unwrap().unwrap();
+        assert_eq!(src.workspace_hash, "ws-alpha", "source must not move");
+        let cp = db.get_entity_by_id_public("mem-fresh").unwrap().unwrap();
+        assert_eq!(cp.workspace_hash, "ws-beta");
+
+        // Same-workspace re-remember still takes the idempotent update path.
+        let (id2, action2) = db.remember(&a).unwrap();
+        assert_eq!(id2, "id-a");
+        assert_eq!(action2, "updated");
+
+        // get_entity without a workspace in hand picks deterministically
+        // (lexicographically-first workspace when no global '' row exists).
+        let picked = db.get_entity("note", "shared-key").unwrap().unwrap();
+        assert_eq!(picked.workspace_hash, "ws-alpha");
+
+        // forget archives every workspace's copy and cleans FTS for all.
+        assert!(db.forget("note", "shared-key", "test cleanup").unwrap());
+        assert!(db.get_entity_by_id_public("id-a").unwrap().unwrap().archived);
+        assert!(db.get_entity_by_id_public("mem-fresh").unwrap().unwrap().archived);
+
+        let _ = fs::remove_file(&path);
+    }
+
     #[test]
     fn context_scopes_to_workspace_including_always_on() {
         // context() feeds mimir_context and `prepare` — a scoped call must not

src/schema.rs

@@ -53,7 +53,10 @@ CREATE TABLE IF NOT EXISTS entities (
     efficacy_status TEXT DEFAULT 'unverified'  -- 'unverified' | 'useful' | 'dead'
 );
 
-CREATE UNIQUE INDEX IF NOT EXISTS idx_entities_category_key ON entities(category, key);
+-- Identity index: (category, key, workspace_hash) — #339. Created in
+-- initialize_schema's gated block, NOT here: on a legacy DB this ungated DDL
+-- runs before the ALTER that adds workspace_hash, so an index referencing the
+-- column here would fail the whole batch.
 
 -- Recall ranking index: lets the browse path (WHERE archived=0 [+ residual
 -- filters] ORDER BY retrieval_count DESC, last_accessed_unix_ms DESC LIMIT k)
@@ -89,9 +92,9 @@ CREATE TABLE IF NOT EXISTS state (
 );
 
 -- Superseded fact versions (v2.4.0 — bi-temporal facts). When a remember()
--- overwrites an existing (category,key) with new content, the prior row is
--- snapshotted here with invalidated_at set, so live reads stay one-row-per-key
--- (entities + its UNIQUE(category,key) are untouched) while history is kept for
+-- overwrites an existing (category,key,workspace_hash) with new content, the prior
+-- row is snapshotted here with invalidated_at set, so live reads stay one-row-per-key
+-- (entities + its UNIQUE(category,key,workspace_hash) are untouched) while history is kept for
 -- as-of / time-travel queries. A version was live during
 -- [recorded_at_unix_ms, invalidated_at_unix_ms). superseded_by points at the
 -- live entity id that replaced it. body_json carries the same encryption as
@@ -137,7 +140,7 @@ CREATE INDEX IF NOT EXISTS idx_entity_history_catkey ON entity_history(category,
 /// the column-add migrations below have been applied. Bump this whenever you add
 /// a new ALTER-probe migration in `initialize_schema`, or existing databases
 /// (already at the previous level) will skip it.
-const SCHEMA_VERSION: i64 = 3;
+const SCHEMA_VERSION: i64 = 4;
 
 /// Initialize the v0.2.0 schema on a fresh database.
 pub fn initialize_schema(conn: &Connection) -> Result<(), Box<dyn std::error::Error>> {
@@ -262,6 +265,20 @@ pub fn initialize_schema(conn: &Connection) -> Result<(), Box<dyn std::error::Er
          ON entities(invalidated_at_unix_ms);",
     )?;
 
+    // v4 (#339): identity becomes (category, key, workspace_hash). A plain
+    // (category, key) uniqueness made cross-workspace key collisions
+    // unstorable, which is what forced mimir_share's "copy into workspace" to
+    // clobber the source row. Created here (after the workspace_hash ALTER,
+    // like idx_entities_invalidated) rather than in the ungated DDL. Safe on
+    // a populated DB — the old constraint was strictly tighter, so no
+    // existing rows can collide. Create-then-drop, so uniqueness is never
+    // unenforced.
+    conn.execute_batch(
+        "CREATE UNIQUE INDEX IF NOT EXISTS idx_entities_category_key_ws \
+         ON entities(category, key, workspace_hash); \
+         DROP INDEX IF EXISTS idx_entities_category_key;",
+    )?;
+
     // Stamp the migration level so subsequent opens skip the probe block above.
     conn.pragma_update(None, "user_version", SCHEMA_VERSION)?;
 
@@ -604,6 +621,65 @@ mod tests {
         assert_eq!(v, SCHEMA_VERSION);
     }
 
+    #[test]
+    fn migrates_unique_index_to_workspace_scoped_identity() {
+        // v4 (#339): a v3-era DB with the two-column unique index and existing
+        // rows must come out with the three-column index, the old index
+        // dropped, and cross-workspace key collisions storable.
+        let (conn, _path) = temp_db();
+        initialize_schema(&conn).expect("fresh init");
+        // Rewind to the v3 state: old index back, new index gone, version 3.
+        conn.execute_batch(
+            "DROP INDEX IF EXISTS idx_entities_category_key_ws;
+             CREATE UNIQUE INDEX idx_entities_category_key ON entities(category, key);
+             PRAGMA user_version = 3;",
+        )
+        .unwrap();
+        conn.execute(
+            "INSERT INTO entities (id, category, key, body_json, workspace_hash, created_at_unix_ms, last_accessed_unix_ms)
+             VALUES ('mig-a', 'note', 'k', '{}', 'ws-alpha', 0, 0)",
+            [],
+        )
+        .unwrap();
+
+        initialize_schema(&conn).expect("v3 -> v4 migration");
+
+        let old_idx: i64 = conn
+            .query_row(
+                "SELECT COUNT(*) FROM sqlite_master WHERE type='index' AND name='idx_entities_category_key'",
+                [],
+                |r| r.get(0),
+            )
+            .unwrap();
+        assert_eq!(old_idx, 0, "old two-column unique index must be dropped");
+        let new_idx: i64 = conn
+            .query_row(
+                "SELECT COUNT(*) FROM sqlite_master WHERE type='index' AND name='idx_entities_category_key_ws'",
+                [],
+                |r| r.get(0),
+            )
+            .unwrap();
+        assert_eq!(new_idx, 1, "workspace-scoped unique index must exist");
+
+        // Same (category, key) in a different workspace now inserts…
+        conn.execute(
+            "INSERT INTO entities (id, category, key, body_json, workspace_hash, created_at_unix_ms, last_accessed_unix_ms)
+             VALUES ('mig-b', 'note', 'k', '{}', 'ws-beta', 0, 0)",
+            [],
+        )
+        .expect("cross-workspace key collision must be storable after v4");
+        // …while a true duplicate in the SAME workspace is still rejected.
+        assert!(
+            conn.execute(
+                "INSERT INTO entities (id, category, key, body_json, workspace_hash, created_at_unix_ms, last_accessed_unix_ms)
+                 VALUES ('mig-c', 'note', 'k', '{}', 'ws-alpha', 0, 0)",
+                [],
+            )
+            .is_err(),
+            "same-workspace duplicate must still violate uniqueness"
+        );
+    }
+
     #[test]
     fn adds_bitemporal_columns_and_backfills_recorded_at() {
         // A legacy DB (no bi-temporal columns) with one row predating the migration.