fix(memory): canonicalize memory identifiers symmetrically (#5164)

src/core/observability.rs

@@ -358,6 +358,33 @@ pub enum ExpectedErrorKind {
     /// (`"MCP unauthorized for "` + `"(HTTP 401"`) so an unrelated MCP transport
     /// failure still reaches Sentry.
     McpServerNeedsAuth,
+    /// The memory store refused a write because the caller-supplied
+    /// **namespace / key** failed a boundary check — it carries secret-shaped
+    /// text, or it trimmed to empty. The rejection is deterministic in the
+    /// caller's own input (`memory_store::namespace_store::documents`,
+    /// `namespace_store::fts5`, the tinycortex KV store): the same call retried
+    /// with the same identifier fails identically, so every retry produced
+    /// another `report_error_or_expected` capture through the RPC dispatcher.
+    /// That is how the PII variant of this family reached 3,055 events from a
+    /// single user in one day (TAURI-RUST-QWW, #5164) — the flood was retry
+    /// volume, not 3,055 distinct defects.
+    ///
+    /// The PII half of the family no longer rejects at all: those identifiers
+    /// are canonicalized on write and on read (see
+    /// [`crate::openhuman::memory_store::safety::canonical_identifier`]). This
+    /// arm covers the rejections that remain deliberate — a secret must never
+    /// be persisted as a storage address (#4947), and an empty key has no row
+    /// to address — and keeps their retry volume out of the error stream.
+    /// Sentry has no remediation path either way: the fix is the caller passing
+    /// a stable opaque identifier, which is a code change in the calling sync
+    /// provider, not a signal that repeats per attempt.
+    ///
+    /// Anchored on the store's own rejection wording (`"cannot contain
+    /// secrets"` / `"document key cannot be empty"` scoped to a
+    /// document/kv/episodic subject) so unrelated failures on the same write
+    /// path — SQLite errors, embedding failures, sidecar IO — still reach
+    /// Sentry as errors.
+    MemoryIdentifierRejected,
 }
 
 pub fn expected_error_kind(message: &str) -> Option<ExpectedErrorKind> {
@@ -421,6 +448,16 @@ pub fn expected_error_kind(message: &str) -> Option<ExpectedErrorKind> {
     if is_mcp_server_needs_auth_message(&lower) {
         return Some(ExpectedErrorKind::McpServerNeedsAuth);
     }
+    // TAURI-RUST-QWW (#5164) — the memory store rejected a write because the
+    // caller's namespace/key failed a boundary check. Deterministic in the
+    // caller's input, so the same call retried fails identically and each retry
+    // captured another event (3,055 events / 1 user / 1 day). Highly specific
+    // anchors, checked before the generic matchers; see
+    // `is_memory_identifier_rejection_message` and
+    // `ExpectedErrorKind::MemoryIdentifierRejected`.
+    if is_memory_identifier_rejection_message(&lower) {
+        return Some(ExpectedErrorKind::MemoryIdentifierRejected);
+    }
     if lower.contains("local ai is disabled") {
         return Some(ExpectedErrorKind::LocalAiDisabled);
     }
@@ -1066,6 +1103,38 @@ fn is_mcp_server_needs_auth_message(lower: &str) -> bool {
     lower.contains("mcp unauthorized for ") && lower.contains("(http 401")
 }
 
+/// Detect a memory-store **identifier** rejection: the caller's namespace/key
+/// (or episodic `session_id`/`role`) failed a write-boundary check.
+///
+/// Matches the store's own rejection wording, verbatim and subject-scoped:
+///   - `document namespace/key cannot contain secrets`
+///     (`namespace_store::documents`, both upsert paths)
+///   - `document key cannot be empty` (same paths, post-trim)
+///   - `kv key cannot contain secrets` / `kv namespace/key cannot contain
+///     secrets` (`tinycortex::memory::store::kv`)
+///   - `episodic session_id/role cannot contain secrets`
+///     (`namespace_store::fts5`)
+///   - the retired `… cannot contain personal identifiers` wording, so a client
+///     still running a pre-#5164 core (the releases the flood came from) is
+///     demoted too
+///
+/// Requiring the subject prefix (`document` / `kv` / `episodic`) keeps the
+/// demotion inside the memory store: a real defect on the same write path —
+/// SQLite failure, embedding provider error, markdown sidecar IO — carries none
+/// of these bodies and still reaches Sentry as an error. See
+/// [`ExpectedErrorKind::MemoryIdentifierRejected`].
+fn is_memory_identifier_rejection_message(lower: &str) -> bool {
+    let rejects_identifier = lower.contains("cannot contain secrets")
+        || lower.contains("cannot contain personal identifiers")
+        || lower.contains("key cannot be empty");
+    rejects_identifier
+        && (lower.contains("document namespace/key")
+            || lower.contains("document key")
+            || lower.contains("kv key")
+            || lower.contains("kv namespace/key")
+            || lower.contains("episodic session_id/role"))
+}
+
 /// Detect the "a configured provider has no API key" user-config state.
 ///
 /// Single source of truth for the `ApiKeyMissing` wording so the
@@ -2017,6 +2086,24 @@ fn report_expected_message(kind: ExpectedErrorKind, message: &str, domain: &str,
                 "[observability] {domain}.{operation} skipped expected MCP needs-auth (401) error: {message}"
             );
         }
+        ExpectedErrorKind::MemoryIdentifierRejected => {
+            // The memory store refused a write whose namespace/key failed a
+            // boundary check (secret-shaped, or empty after trim). Deterministic
+            // in the caller's input: retrying the same call rejects again, so
+            // this repeats at the caller's retry rate rather than carrying new
+            // signal each time (TAURI-RUST-QWW: 3,055 events / 1 user / 1 day,
+            // #5164). The remedy is the calling sync provider passing a stable
+            // opaque identifier — a code change, not a per-attempt signal — so
+            // demote to warn: the breadcrumb survives for triage, no error event
+            // fires.
+            tracing::warn!(
+                domain = domain,
+                operation = operation,
+                kind = "memory_identifier_rejected",
+                error = %message,
+                "[observability] {domain}.{operation} skipped expected memory identifier rejection: {message}"
+            );
+        }
         ExpectedErrorKind::ProviderConfigRejection => {
             // User-config state: a custom cloud provider rejected the
             // request because of the user's model / parameter setup — an
@@ -3720,6 +3807,61 @@ mod tests {
         );
     }
 
+    /// Sentry TAURI-RUST-QWW (#5164): a memory-store identifier rejection is
+    /// deterministic in the caller's own input, so it repeats at the caller's
+    /// retry rate (3,055 events / 1 user / 1 day) without carrying new signal.
+    /// Every rejection wording the store emits must classify as
+    /// `MemoryIdentifierRejected`, including the retired PII wording that
+    /// pre-#5164 cores still send.
+    #[test]
+    fn classifies_memory_identifier_rejections_as_expected() {
+        for msg in [
+            "document namespace/key cannot contain secrets",
+            "document namespace/key cannot contain personal identifiers",
+            "document key cannot be empty",
+            "kv key cannot contain secrets",
+            "kv namespace/key cannot contain secrets",
+            "episodic session_id/role cannot contain secrets",
+            // The stringified RPC re-report shape that reaches the dispatcher.
+            "openhuman.memory_store failed: document namespace/key cannot contain secrets",
+        ] {
+            assert_eq!(
+                expected_error_kind(msg),
+                Some(ExpectedErrorKind::MemoryIdentifierRejected),
+                "must classify as MemoryIdentifierRejected: {msg}"
+            );
+        }
+        // Full demotion path (classifier -> report arm) must not panic.
+        report_error_or_expected(
+            "document namespace/key cannot contain secrets",
+            "rpc",
+            "openhuman.memory_store",
+            &[],
+        );
+    }
+
+    /// Guard against over-suppression: a real failure on the same memory write
+    /// path — SQLite, embeddings, the markdown sidecar — carries none of the
+    /// rejection wording and MUST still reach Sentry (stay `None`). Nor may a
+    /// bare "cannot contain secrets" from an unrelated domain borrow the
+    /// memory-store demotion.
+    #[test]
+    fn does_not_classify_real_memory_write_failures_as_identifier_rejections() {
+        for msg in [
+            "upsert memory_docs: database is locked",
+            "insert vector chunk: disk I/O error",
+            "lookup existing document_id: no such table: memory_docs",
+            "write_markdown_doc: permission denied",
+            "webhook payload cannot contain secrets",
+        ] {
+            assert_ne!(
+                expected_error_kind(msg),
+                Some(ExpectedErrorKind::MemoryIdentifierRejected),
+                "must NOT classify as MemoryIdentifierRejected: {msg}"
+            );
+        }
+    }
+
     /// Guard against over-suppression: an MCP transport failure that is NOT the
     /// typed 401 (a 500, or a generic "unauthorized" with no MCP anchor) MUST
     /// still reach Sentry (stay `None`) so a real defect isn't blinded.

src/openhuman/memory_store/kv.rs

@@ -1,8 +1,17 @@
 //! Compatibility methods for tinycortex's shared-connection KV store.
+//!
+//! Every method canonicalizes its namespace/key through
+//! [`canonical_identifier`] before delegating. The crate's `set_*` already
+//! canonicalizes PII-bearing keys on the way in (#5164) but its `get_*` /
+//! `delete_*` / `list_*` address the raw key, so without this shim a write
+//! whose key was rewritten reads back as absent — and the caller writes it
+//! again. Canonicalizing here is a no-op for the write path (the transform is
+//! idempotent and identical) and makes the read path symmetric.
 
 use tinycortex::memory::store::kv::KvStore;
 
 use crate::openhuman::memory_store::namespace_store::UnifiedMemory;
+use crate::openhuman::memory_store::safety::canonical_identifier;
 use crate::openhuman::memory_store::types::MemoryKvRecord;
 
 impl UnifiedMemory {
@@ -12,11 +21,12 @@ impl UnifiedMemory {
     }
 
     pub async fn kv_set_global(&self, key: &str, value: &serde_json::Value) -> Result<(), String> {
-        self.tinycortex_kv()?.set_global(key, value)
+        self.tinycortex_kv()?
+            .set_global(&canonical_identifier(key), value)
     }
 
     pub async fn kv_get_global(&self, key: &str) -> Result<Option<serde_json::Value>, String> {
-        self.tinycortex_kv()?.get_global(key)
+        self.tinycortex_kv()?.get_global(&canonical_identifier(key))
     }
 
     pub async fn kv_set_namespace(
@@ -25,38 +35,46 @@ impl UnifiedMemory {
         key: &str,
         value: &serde_json::Value,
     ) -> Result<(), String> {
-        self.tinycortex_kv()?.set_namespace(namespace, key, value)
+        self.tinycortex_kv()?.set_namespace(
+            &canonical_identifier(namespace),
+            &canonical_identifier(key),
+            value,
+        )
     }
 
     pub async fn kv_get_namespace(
         &self,
         namespace: &str,
         key: &str,
     ) -> Result<Option<serde_json::Value>, String> {
-        self.tinycortex_kv()?.get_namespace(namespace, key)
+        self.tinycortex_kv()?
+            .get_namespace(&canonical_identifier(namespace), &canonical_identifier(key))
     }
 
     pub async fn kv_delete_global(&self, key: &str) -> Result<bool, String> {
-        self.tinycortex_kv()?.delete_global(key)
+        self.tinycortex_kv()?
+            .delete_global(&canonical_identifier(key))
     }
 
     pub async fn kv_delete_namespace(&self, namespace: &str, key: &str) -> Result<bool, String> {
-        self.tinycortex_kv()?.delete_namespace(namespace, key)
+        self.tinycortex_kv()?
+            .delete_namespace(&canonical_identifier(namespace), &canonical_identifier(key))
     }
 
     pub async fn kv_list_namespace(
         &self,
         namespace: &str,
     ) -> Result<Vec<serde_json::Value>, String> {
-        self.tinycortex_kv()?.list_namespace(namespace)
+        self.tinycortex_kv()?
+            .list_namespace(&canonical_identifier(namespace))
     }
 
     pub(crate) async fn kv_records_for_scope(
         &self,
         namespace: &str,
     ) -> Result<Vec<MemoryKvRecord>, String> {
         self.tinycortex_kv()?
-            .records_for_scope(namespace)
+            .records_for_scope(&canonical_identifier(namespace))
             .map(convert_records)
     }
 
@@ -65,7 +83,7 @@ impl UnifiedMemory {
         namespace: &str,
     ) -> Result<Vec<MemoryKvRecord>, String> {
         self.tinycortex_kv()?
-            .records_namespace(namespace)
+            .records_namespace(&canonical_identifier(namespace))
             .map(convert_records)
     }
 

src/openhuman/memory_store/memory_trait.rs

@@ -339,11 +339,13 @@ impl Memory for UnifiedMemory {
     }
 
     async fn get(&self, namespace: &str, key: &str) -> anyhow::Result<Option<MemoryEntry>> {
-        let ns = if namespace.trim().is_empty() {
-            GLOBAL_NAMESPACE.to_string()
-        } else {
-            namespace.to_string()
-        };
+        // Address the row the way `store` wrote it: `upsert_document` stores
+        // `sanitize_namespace(namespace)` and `canonical_document_key(key)`, so
+        // looking up the raw caller values misses whenever either transform
+        // changed anything — the caller then reads the row as absent and stores
+        // it again, which is the retry loop behind #5164.
+        let ns = UnifiedMemory::sanitize_namespace(namespace);
+        let key = crate::openhuman::memory_store::safety::canonical_document_key(key);
         let conn = self.conn.lock();
         let row: Option<(String, String, String, f64, String, String)> = conn
             .query_row(
@@ -416,11 +418,11 @@ impl Memory for UnifiedMemory {
     }
 
     async fn forget(&self, namespace: &str, key: &str) -> anyhow::Result<bool> {
-        let ns = if namespace.trim().is_empty() {
-            GLOBAL_NAMESPACE.to_string()
-        } else {
-            namespace.to_string()
-        };
+        // Same write/read symmetry as `get` above (#5164): a `forget` that
+        // addresses the raw caller identifiers can never delete a row whose
+        // namespace or key was canonicalized on the way in.
+        let ns = UnifiedMemory::sanitize_namespace(namespace);
+        let key = crate::openhuman::memory_store::safety::canonical_document_key(key);
         let row: Option<String> = {
             let conn = self.conn.lock();
             conn.query_row(

src/openhuman/memory_store/namespace_store/README.md

@@ -14,6 +14,32 @@ tier; this directory is intentionally not migration staging.
 - **`documents.rs`** — `memory_docs` CRUD: `upsert_document` (chunks + embeds + writes markdown sidecar), `upsert_document_metadata_only` (light path), `list_documents`, `list_namespaces`, `delete_document`, `clear_namespace`.
 - **`kv.rs`** — global and namespace-scoped get/set/delete/list against `kv_global` / `kv_namespace`.
 - **`../../safety/`** — secret redaction/validation helpers. Document, KV, and episodic writes sanitize credentials before persistence and emit `[memory:safety]` diagnostics when a payload is rewritten.
+
+### Identifier canonicalization (namespace / key)
+
+Content and identifiers are scrubbed by **different** rules, and mixing them up
+caused #5164. `safety::canonical_identifier` (namespace, KV key) and
+`safety::canonical_document_key` (document key) are the single source of truth:
+
+- **Strict gating.** Only formatted / keyword-gated national IDs are rewritten
+  (`has_likely_pii`). The lenient content scrubber (`redact_pii` on its own,
+  used for titles/bodies/metadata) also rewrites bare digit runs, which the
+  scanners legitimately use as identifiers — WhatsApp JIDs, iMessage `+1…` chat
+  ids, timestamps, padded counters. Rewriting those maps two contacts onto one
+  `(namespace, key)`, and the upsert's `ON CONFLICT … DO UPDATE` then has one
+  contact's document overwrite the other's.
+- **Symmetry.** An identifier is a storage *address*, so every path that
+  addresses a row canonicalizes the same way: `sanitize_namespace` (`init.rs`)
+  carries the namespace step for writes, reads, `query.rs`, `graph.rs`, deletes
+  and the on-disk `namespaces/<ns>/` directory, and the by-key paths
+  (`upsert_document*`, `Memory::get`, `Memory::forget`, the `kv.rs` shim) go
+  through `canonical_document_key` / `canonical_identifier`. A read that skips
+  the transform silently misses the row the write created, so the caller writes
+  again — the unthrottled loop #5164 was reported for.
+- **Never reject.** Rejecting the write instead returns an `Err` on every retry,
+  which is what flooded Sentry (3,055 events / 1 user / 1 day). The rejections
+  that remain deliberate (secret-shaped identifiers, empty keys) are demoted out
+  of the error stream by `ExpectedErrorKind::MemoryIdentifierRejected`.
 - **`graph.rs`** — `graph_namespace` / `graph_global` upserts with attribute merging and evidence accumulation, plus namespace / global / cross-namespace queries and document-scoped relation removal.
 - **`query.rs`** — hybrid retrieval. Combines graph relevance, vector similarity, keyword overlap, episodic signal and freshness; exposes `query_namespace_*` (with query) and `recall_namespace_*` (query-less) entry points used by `MemoryClient`.
 - **`helpers.rs`** — shared utilities: f32-vector byte codecs, cosine similarity, markdown chunking, text/graph normalisation, JSON attribute merging, recency scoring.