From cabbed5a4bdd9acb26b5edf33a834ecb1587d558 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Thu, 30 Jul 2026 18:02:32 +0530 Subject: [PATCH] fix(memory): canonicalize memory identifiers symmetrically (#5164) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `document namespace/key cannot contain personal identifiers` reached Sentry 3,055 times from a single user in one day (TAURI-RUST-QWW). The rejection is deterministic in the caller's own input, so every retry re-reported it. #5171 stopped rejecting and started rewriting the namespace/key, but used `redact_pii` — the *content* scrubber — on identifiers, and only on the write side. Two defects follow: * `redact_pii` rewrites bare digit-run shapes that the crate's own boundary predicate deliberately tolerates because the scanners build identifiers out of them (WhatsApp JIDs, iMessage `+1…` chat ids, ms timestamps, padded counters). Two contacts then share one `(namespace, key)` and the upsert's `ON CONFLICT … DO UPDATE` has one contact's document overwrite the other's. * rewriting an identifier changes the row's address, so `Memory::get` / `Memory::forget` (raw key), `query.rs` / `graph.rs` (namespace without the rewrite) and the KV `get_*` / `delete_*` addressed rows the write never created. The caller reads the row as absent and writes again — the same unthrottled loop, now silent instead of erroring. Canonicalization is now single-sourced and strict-gated: `safety::canonical_identifier` (+ `canonical_document_key` for the trim) rewrite only formatted / keyword-gated national IDs, and are idempotent so read paths can apply them unconditionally. The namespace step moves into `sanitize_namespace`, the one funnel every namespace path already shares, and the by-key paths (both upserts, `Memory::get`, `Memory::forget`, the KV shim) go through the document-key helper. The rejections that remain deliberate — secret-shaped identifiers (#4947), keys that trim to empty — now classify as `ExpectedErrorKind::MemoryIdentifierRejected`, so their retry volume stays out of the error stream while real failures on the same write path (SQLite, embeddings, sidecar IO) still page. Regression coverage: PII-bearing keys/namespaces round-trip through get/list/forget and the KV shim; scanner-built identifiers keep their identity and stay distinct documents; the classifier demotes every rejection wording and no real write failure. --- src/core/observability.rs | 142 ++++++++++++ src/openhuman/memory_store/kv.rs | 36 ++- src/openhuman/memory_store/memory_trait.rs | 22 +- .../memory_store/namespace_store/README.md | 26 +++ .../memory_store/namespace_store/documents.rs | 56 ++--- .../namespace_store/documents_tests.rs | 214 +++++++++++++++++- .../memory_store/namespace_store/init.rs | 37 ++- src/openhuman/memory_store/safety/mod.rs | 109 +++++++++ 8 files changed, 588 insertions(+), 54 deletions(-) diff --git a/src/core/observability.rs b/src/core/observability.rs index ea22adce2f..dfc7c3b165 100644 --- a/src/core/observability.rs +++ b/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 { @@ -421,6 +448,16 @@ pub fn expected_error_kind(message: &str) -> Option { 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. diff --git a/src/openhuman/memory_store/kv.rs b/src/openhuman/memory_store/kv.rs index d647de6558..00e18be4f9 100644 --- a/src/openhuman/memory_store/kv.rs +++ b/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, String> { - self.tinycortex_kv()?.get_global(key) + self.tinycortex_kv()?.get_global(&canonical_identifier(key)) } pub async fn kv_set_namespace( @@ -25,7 +35,11 @@ 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( @@ -33,22 +47,26 @@ impl UnifiedMemory { namespace: &str, key: &str, ) -> Result, 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 { - 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 { - 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, String> { - self.tinycortex_kv()?.list_namespace(namespace) + self.tinycortex_kv()? + .list_namespace(&canonical_identifier(namespace)) } pub(crate) async fn kv_records_for_scope( @@ -56,7 +74,7 @@ impl UnifiedMemory { namespace: &str, ) -> Result, 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, String> { self.tinycortex_kv()? - .records_namespace(namespace) + .records_namespace(&canonical_identifier(namespace)) .map(convert_records) } diff --git a/src/openhuman/memory_store/memory_trait.rs b/src/openhuman/memory_store/memory_trait.rs index c576b5f48d..2c9bd92ce8 100644 --- a/src/openhuman/memory_store/memory_trait.rs +++ b/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> { - 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 { - 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 = { let conn = self.conn.lock(); conn.query_row( diff --git a/src/openhuman/memory_store/namespace_store/README.md b/src/openhuman/memory_store/namespace_store/README.md index 30aca9e70e..1718dcfb18 100644 --- a/src/openhuman/memory_store/namespace_store/README.md +++ b/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//` 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. diff --git a/src/openhuman/memory_store/namespace_store/documents.rs b/src/openhuman/memory_store/namespace_store/documents.rs index ca7b8534f6..38f7a657a4 100644 --- a/src/openhuman/memory_store/namespace_store/documents.rs +++ b/src/openhuman/memory_store/namespace_store/documents.rs @@ -28,25 +28,26 @@ impl UnifiedMemory { return Err("document namespace/key cannot contain secrets".to_string()); } - // Auto-sanitize PII from namespace/key rather than rejecting the entire - // write (see #5164). Previously this returned an Err, which caused - // unthrottled retry loops when caller-generated identifiers happened to - // contain structured personal identifiers (CPF, SSN, RFC, etc.). + // Canonicalize a PII-bearing key rather than rejecting the whole write + // (see #5164): rejection returned an `Err` on every attempt, and callers + // retry, so one such key produced an unthrottled error loop (3,055 + // Sentry events from a single user). + // + // `canonical_document_key` is strict-gated so scanner-built identifiers + // (WhatsApp JIDs, `+1…` chat ids, timestamps) keep their identity — see + // its docs. The namespace is canonicalized by `sanitize_namespace` + // below, and the by-key read paths (`Memory::get` / `Memory::forget`) + // canonicalize through the same helper, so a rewritten identifier stays + // addressable instead of reading back as a missing row. let input = { - let key = safety::pii::redact_pii(&input.key); - let namespace = safety::pii::redact_pii(&input.namespace); - if key.report.pii_redactions > 0 || namespace.report.pii_redactions > 0 { + let key = safety::canonical_document_key(&input.key); + if key != input.key { log::info!( - "[memory:safety] document write auto-sanitized PII from namespace/key original_len_ns={} original_len_key={}", - input.namespace.chars().count(), + "[memory:safety] document write canonicalized PII-like key key_chars={}", input.key.chars().count() ); } - NamespaceDocumentInput { - namespace: namespace.value, - key: key.value, - ..input - } + NamespaceDocumentInput { key, ..input } }; let sanitized = safety::sanitize_document_input(input); @@ -257,22 +258,17 @@ impl UnifiedMemory { return Err("document namespace/key cannot contain secrets".to_string()); } - // Auto-sanitize PII from namespace/key rather than rejecting (see #5164). + // Canonicalize a PII-bearing key rather than rejecting (see #5164, and + // the rationale on the `upsert_document` path above). let input = { - let key = safety::pii::redact_pii(&input.key); - let namespace = safety::pii::redact_pii(&input.namespace); - if key.report.pii_redactions > 0 || namespace.report.pii_redactions > 0 { + let key = safety::canonical_document_key(&input.key); + if key != input.key { log::info!( - "[memory:safety] metadata-only write auto-sanitized PII from namespace/key original_len_ns={} original_len_key={}", - input.namespace.chars().count(), + "[memory:safety] metadata-only write canonicalized PII-like key key_chars={}", input.key.chars().count() ); } - NamespaceDocumentInput { - namespace: namespace.value, - key: key.value, - ..input - } + NamespaceDocumentInput { key, ..input } }; let sanitized = safety::sanitize_document_input(input); @@ -392,7 +388,7 @@ impl UnifiedMemory { namespace: &str, ) -> Result, String> { let conn = self.conn.lock(); - let ns = Self::sanitize_namespace(&safety::pii::redact_pii(namespace).value); + let ns = Self::sanitize_namespace(namespace); let mut stmt = conn .prepare( "SELECT @@ -469,9 +465,7 @@ impl UnifiedMemory { ) .map_err(|e| format!("prepare list_documents: {e}"))?; let mut rows = stmt - .query(params![Self::sanitize_namespace( - &safety::pii::redact_pii(ns).value - )]) + .query(params![Self::sanitize_namespace(ns)]) .map_err(|e| format!("query list_documents: {e}"))?; while let Some(row) = rows .next() @@ -545,7 +539,7 @@ impl UnifiedMemory { /// for the given namespace in a single transaction. Also removes the /// on-disk markdown directory (`namespaces/{ns}/docs/`). pub async fn clear_namespace(&self, namespace: &str) -> Result<(), String> { - let ns = Self::sanitize_namespace(&safety::pii::redact_pii(namespace).value); + let ns = Self::sanitize_namespace(namespace); log::debug!("[memory] clear_namespace: starting for namespace={ns}"); { @@ -618,7 +612,7 @@ impl UnifiedMemory { namespace: &str, document_id: &str, ) -> Result { - let ns = Self::sanitize_namespace(&safety::pii::redact_pii(namespace).value); + let ns = Self::sanitize_namespace(namespace); let rel_path: Option = { let conn = self.conn.lock(); conn.query_row( diff --git a/src/openhuman/memory_store/namespace_store/documents_tests.rs b/src/openhuman/memory_store/namespace_store/documents_tests.rs index 23dd94492e..c8ffdd41f9 100644 --- a/src/openhuman/memory_store/namespace_store/documents_tests.rs +++ b/src/openhuman/memory_store/namespace_store/documents_tests.rs @@ -1127,11 +1127,19 @@ async fn kv_set_global_auto_sanitizes_pii_like_key() { .await .expect("PII-like global key should be auto-sanitized, not rejected"); - // The key in storage contains the redacted token. + // ... and the caller must be able to read it back with the identifier it + // wrote. The canonicalization is a storage-address transform, so the read + // path applies the same one (#5164). A miss here is what made the caller + // write again, which is the loop the issue was reported for. let stored = memory.kv_get_global("ssn-123-45-6789").await.unwrap(); + assert_eq!( + stored, + Some(json!({"value": "ok"})), + "a canonicalized KV key must stay readable by its original identifier" + ); assert!( - stored.is_none(), - "original PII key should not match after sanitization" + memory.kv_delete_global("ssn-123-45-6789").await.unwrap(), + "delete must address the same canonicalized row the write created" ); } @@ -1148,6 +1156,14 @@ async fn kv_set_namespace_auto_sanitizes_pii_like_key() { let records = memory.kv_records_namespace("safe").await.unwrap(); // The record should still exist; the key gets redacted internally. assert_eq!(records.len(), 1); + assert_eq!( + memory + .kv_get_namespace("safe", "ssn-123-45-6789") + .await + .unwrap(), + Some(json!({"value": "ok"})), + "a canonicalized KV key must stay readable by its original identifier" + ); } #[tokio::test] @@ -1159,6 +1175,15 @@ async fn kv_set_namespace_auto_sanitizes_pii_like_namespace() { .kv_set_namespace("user/111.444.777-35", "safe-key", &json!({"value": "ok"})) .await .expect("PII-like namespace should be auto-sanitized, not rejected"); + + assert_eq!( + memory + .kv_get_namespace("user/111.444.777-35", "safe-key") + .await + .unwrap(), + Some(json!({"value": "ok"})), + "a canonicalized KV namespace must stay readable by its original value" + ); } #[tokio::test] @@ -1298,3 +1323,186 @@ async fn upsert_document_metadata_only_auto_sanitizes_pii_like_namespace() { doc.namespace ); } + +// --------------------------------------------------------------------------- +// #5164 — the identifier canonicalization has to be symmetric, and it has to +// leave scanner-built identifiers alone. +// +// Canonicalizing a namespace/key rewrites the row's *address*. Two failure +// modes follow, and both re-create the unthrottled write loop the issue was +// filed for (silently, this time): +// +// 1. a read path that addresses the raw caller identifier never finds the +// canonicalized row, so the caller writes it again; +// 2. canonicalizing with the lenient *content* scrubber maps every +// phone-shaped identifier onto one placeholder, so distinct chats collapse +// onto one `(namespace, key)` and the upsert's `ON CONFLICT … DO UPDATE` +// has one contact's document overwrite another's. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn pii_like_document_key_round_trips_through_get_and_forget() { + use crate::openhuman::memory::traits::Memory; + + let tmp = TempDir::new().unwrap(); + let memory = UnifiedMemory::new(tmp.path(), Arc::new(NoopEmbedding), None).unwrap(); + + memory + .upsert_document(make_doc_input( + "clients", + "ssn-123-45-6789", + "Title", + "Body", + )) + .await + .expect("PII-like key should be canonicalized, not rejected"); + + let entry = memory + .get("clients", "ssn-123-45-6789") + .await + .unwrap() + .expect("a canonicalized key must stay readable by its original identifier"); + assert_eq!(entry.content, "Body"); + assert!( + !entry.key.contains("123-45-6789"), + "the SSN must not be persisted as the storage address, got: {}", + entry.key + ); + + assert!( + memory.forget("clients", "ssn-123-45-6789").await.unwrap(), + "forget must address the same canonicalized row the write created" + ); + assert!(memory + .get("clients", "ssn-123-45-6789") + .await + .unwrap() + .is_none()); +} + +#[tokio::test] +async fn pii_like_namespace_round_trips_through_get_and_list() { + use crate::openhuman::memory::traits::Memory; + + let tmp = TempDir::new().unwrap(); + let memory = UnifiedMemory::new(tmp.path(), Arc::new(NoopEmbedding), None).unwrap(); + + memory + .upsert_document(make_doc_input( + "cliente-RFC-VECJ880326XK4", + "notes", + "Title", + "Body", + )) + .await + .expect("PII-like namespace should be canonicalized, not rejected"); + + assert!( + memory + .get("cliente-RFC-VECJ880326XK4", "notes") + .await + .unwrap() + .is_some(), + "a canonicalized namespace must stay readable by its original value" + ); + let listed = memory + .list(Some("cliente-RFC-VECJ880326XK4"), None, None) + .await + .unwrap(); + assert_eq!( + listed.len(), + 1, + "list() must canonicalize its namespace the same way the write did" + ); +} + +#[tokio::test] +async fn scanner_built_phone_shaped_keys_stay_distinct_documents() { + let tmp = TempDir::new().unwrap(); + let memory = UnifiedMemory::new(tmp.path(), Arc::new(NoopEmbedding), None).unwrap(); + + // Two different WhatsApp contacts, same day. The digit runs differ only in + // the phone number — the lenient content scrubber replaces both with one + // `[REDACTED_PII_PHONE]` token, which would collapse them onto a single row. + for (key, content) in [ + ("12025551234@c.us:2026-05-30", "alice thread"), + ("12025559999@c.us:2026-05-30", "bob thread"), + ] { + memory + .upsert_document(make_doc_input("whatsapp-web", key, "Chat", content)) + .await + .unwrap(); + } + + let docs = memory + .load_documents_for_scope("whatsapp-web") + .await + .unwrap(); + assert_eq!( + docs.len(), + 2, + "scanner-built phone-shaped keys must stay distinct documents, got: {:?}", + docs.iter().map(|d| d.key.clone()).collect::>() + ); + for key in ["12025551234@c.us:2026-05-30", "12025559999@c.us:2026-05-30"] { + assert!( + docs.iter().any(|d| d.key == key), + "key {key} must be stored verbatim, got: {:?}", + docs.iter().map(|d| d.key.clone()).collect::>() + ); + } +} + +#[tokio::test] +async fn scanner_built_identifiers_are_preserved_verbatim() { + let tmp = TempDir::new().unwrap(); + let memory = UnifiedMemory::new(tmp.path(), Arc::new(NoopEmbedding), None).unwrap(); + + // The strict boundary predicate deliberately tolerates these shapes + // (WhatsApp group JID, iMessage E.164 chat id, padded ms timestamp); the + // content scrubber does not. Canonicalization must follow the strict set, + // or every scanner rewrites its own storage addresses. + for key in [ + "12025551234-1543890267@g.us:2026-05-30", + "imessage:+12025551234:2026-05-30", + "accepted:000001747729035001", + ] { + let doc_id = memory + .upsert_document(make_doc_input("scanner", key, "Title", "Body")) + .await + .unwrap(); + let docs = memory.load_documents_for_scope("scanner").await.unwrap(); + let doc = docs.iter().find(|d| d.document_id == doc_id).unwrap(); + assert_eq!( + doc.key, key, + "scanner-built identifier must not be rewritten" + ); + } +} + +#[tokio::test] +async fn metadata_only_write_round_trips_through_pii_like_key() { + use crate::openhuman::memory::traits::Memory; + + let tmp = TempDir::new().unwrap(); + let memory = UnifiedMemory::new(tmp.path(), Arc::new(NoopEmbedding), None).unwrap(); + + memory + .upsert_document_metadata_only(make_doc_input( + "clients", + "cuit-20-11111111-2", + "Title", + "Body", + )) + .await + .expect("PII-like key should be canonicalized, not rejected"); + + assert!( + memory + .get("clients", "cuit-20-11111111-2") + .await + .unwrap() + .is_some(), + "the metadata-only path must canonicalize keys the same way the full upsert does" + ); +} diff --git a/src/openhuman/memory_store/namespace_store/init.rs b/src/openhuman/memory_store/namespace_store/init.rs index 82c48ed817..6d8b1b12d3 100644 --- a/src/openhuman/memory_store/namespace_store/init.rs +++ b/src/openhuman/memory_store/namespace_store/init.rs @@ -14,6 +14,7 @@ use parking_lot::Mutex; use rusqlite::Connection; use crate::openhuman::embeddings::EmbeddingProvider; +use crate::openhuman::memory_store::safety::canonical_identifier; use crate::openhuman::memory_store::types::GLOBAL_NAMESPACE; use super::UnifiedMemory; @@ -338,8 +339,16 @@ impl UnifiedMemory { .unwrap_or(0.0) } + /// Canonical storage form of a namespace: PII-bearing namespaces are + /// canonicalized (#5164), then path-hostile characters collapse to `_`. + /// + /// The PII step lives here, in the one funnel every namespace path already + /// goes through — writes, reads, recall/search (`query.rs`), graph relations + /// (`graph.rs`), deletes, and the on-disk `namespaces//` directory — so + /// a canonicalized write stays addressable by its original namespace + /// instead of looking like a missing row and driving the caller to retry. pub(crate) fn sanitize_namespace(namespace: &str) -> String { - let trimmed = namespace.trim(); + let trimmed = canonical_identifier(namespace.trim()); if trimmed.is_empty() { return GLOBAL_NAMESPACE.to_string(); } @@ -386,6 +395,32 @@ mod tests { assert_eq!(UnifiedMemory::sanitize_namespace("a-b_c/ok"), "a-b_c/ok"); } + /// #5164: the PII step lives in this one funnel so every namespace path + /// (write, read, recall/search, graph, delete, on-disk dir) derives the same + /// address. Strict-gated — scanner-built namespaces keep their identity. + #[test] + fn sanitize_namespace_canonicalizes_pii_and_preserves_scanner_namespaces() { + let canonical = UnifiedMemory::sanitize_namespace("cliente-RFC-VECJ880326XK4"); + assert!( + !canonical.contains("VECJ880326XK4"), + "the national ID must not become the storage address, got: {canonical}" + ); + assert!( + canonical.contains("REDACTED_PII"), + "expected a redaction placeholder, got: {canonical}" + ); + // Idempotent, so read paths can canonicalize unconditionally. + assert_eq!(UnifiedMemory::sanitize_namespace(&canonical), canonical); + + for namespace in ["whatsapp-web:12025551234@c.us", "skill-gmail", "global"] { + assert_eq!( + UnifiedMemory::sanitize_namespace(namespace), + namespace.replace(['@', ':', '.'], "_"), + "scanner-built namespace must only get the character scrub: {namespace}" + ); + } + } + #[test] fn namespace_dir_uses_sanitized_namespace() { let tmp = TempDir::new().unwrap(); diff --git a/src/openhuman/memory_store/safety/mod.rs b/src/openhuman/memory_store/safety/mod.rs index d224f700a0..db174383d9 100644 --- a/src/openhuman/memory_store/safety/mod.rs +++ b/src/openhuman/memory_store/safety/mod.rs @@ -19,6 +19,49 @@ pub use tinycortex::memory::store::safety::{ has_likely_pii, has_likely_secret, sanitize_json, sanitize_text, SanitizationReport, Sanitized, }; +/// Canonical storage form of a caller-supplied memory **identifier** — a +/// namespace, a document key, or a KV key. +/// +/// An identifier is an address, not content: whatever this returns is what the +/// row is stored under, so every read / update / delete that addresses a row by +/// identifier has to canonicalize through this same function, or it looks up a +/// row the write never created (#5164). +/// +/// Two properties make that safe, and both follow the split the crate's PII +/// module documents between its **strict boundary predicate** and its **lenient +/// content scrubber**: +/// +/// * **Strict gating.** Only identifiers that trip [`has_likely_pii`] — +/// formatted / keyword-gated national IDs (`ssn-123-45-6789`, +/// `cliente-RFC-VECJ880326XK4`, `cuit-20-11111111-2`) — are rewritten. +/// `redact_pii` on its own also rewrites bare digit-run shapes, and the +/// scanners legitimately build identifiers out of those: WhatsApp JIDs +/// (`12025551234-1543890267@g.us`), iMessage `+1…` chat ids, millisecond +/// timestamps, padded counters. Rewriting those maps two distinct contacts +/// onto one `(namespace, key)`, where the upsert's `ON CONFLICT … DO UPDATE` +/// has one contact's document silently overwrite the other's. +/// * **Idempotence.** The `[REDACTED_PII_*]` placeholders carry no PII pattern +/// of their own, so canonicalizing an already-canonical identifier is a +/// no-op — which is what lets read paths canonicalize unconditionally. +pub fn canonical_identifier(value: &str) -> String { + if !has_likely_pii(value) { + return value.to_string(); + } + pii::redact_pii(value).value +} + +/// Canonical storage form of a document key: the exact transform +/// `upsert_document` / `upsert_document_metadata_only` apply before writing the +/// `memory_docs.key` column (trim, then [`canonical_identifier`]). +/// +/// Single-sourced so the by-key read paths (`Memory::get`, `Memory::forget`) +/// cannot drift from the write path. Drift there is invisible — the lookup +/// simply misses, the caller treats the row as absent and writes again, which +/// is the unthrottled loop #5164 was reported for. +pub fn canonical_document_key(key: &str) -> String { + canonical_identifier(key.trim()) +} + /// Scrub a namespace-document input, field by field, via the crate scrubbers. /// /// Sanitization is content-cleaning only; provenance `taint` survives untouched @@ -204,6 +247,72 @@ mod tests { .contains(&format!("\"{REDACTED_SECRET}\""))); } + /// #5164: identifiers are storage addresses, so canonicalization follows + /// the **strict** boundary predicate. Formatted / keyword-gated national IDs + /// are rewritten; the bare digit-run shapes the scanners build identifiers + /// out of are left alone (rewriting those maps distinct contacts onto one + /// `(namespace, key)` and the upsert silently overwrites). + #[test] + fn canonical_identifier_rewrites_only_strict_pii() { + for identifier in [ + "ssn-123-45-6789", + "cliente-RFC-VECJ880326XK4", + "cuit-20-11111111-2", + "user/111.444.777-35", + ] { + let canonical = canonical_identifier(identifier); + assert_ne!( + canonical, identifier, + "strict PII identifier must be canonicalized: {identifier}" + ); + assert!( + canonical.contains("[REDACTED_PII_"), + "expected a redaction placeholder, got: {canonical}" + ); + } + + for identifier in [ + // WhatsApp group JID / 1:1 JID / broadcast, iMessage E.164 chat id, + // telegram numeric peer id, padded ms timestamp, plain namespaces. + "12025551234-1543890267@g.us:2026-05-30", + "12025551234@c.us:2026-05-30", + "imessage:+12025551234:2026-05-30", + "4123456789:2026-05-30", + "accepted:000001747729035001", + "memory/global/preferences", + "skill-gmail", + ] { + assert_eq!( + canonical_identifier(identifier), + identifier, + "scanner-built identifier must keep its identity: {identifier}" + ); + } + } + + /// Read paths canonicalize unconditionally, so the transform has to be a + /// fixed point on its own output. + #[test] + fn canonical_identifier_is_idempotent() { + for identifier in ["ssn-123-45-6789", "cliente-RFC-VECJ880326XK4", "safe-key"] { + let once = canonical_identifier(identifier); + assert_eq!(canonical_identifier(&once), once, "not idempotent: {once}"); + } + } + + /// `canonical_document_key` single-sources the write-path transform, trim + /// included — otherwise `Memory::get` would address an untrimmed key that + /// `upsert_document` never wrote. + #[test] + fn canonical_document_key_trims_before_canonicalizing() { + assert_eq!(canonical_document_key(" doc-a "), "doc-a"); + assert_eq!( + canonical_document_key(" ssn-123-45-6789 "), + canonical_identifier("ssn-123-45-6789") + ); + assert_eq!(canonical_document_key(" "), ""); + } + #[test] fn sanitize_document_input_preserves_taint() { let input = NamespaceDocumentInput {