diff --git a/README.md b/README.md index 4769102..c446719 100644 --- a/README.md +++ b/README.md @@ -281,6 +281,29 @@ All three advertise the mandatory Core, Recall, and Portability families. The live Docker harness and conformance command are documented in [`integration/remote-engines/`](integration/remote-engines/README.md). +One of them restricts what it will store. Supermemory removes `U+0000` and +`U+FFFD` from content server-side, so the adapter refuses such content with +`MemoryError::Invalid` rather than storing a value the service would quietly +rewrite: `MemoryCore::store` promises that what is read back equals what was +stored, and a driver may refuse a shape but may not accept one and hand back +another. The restriction is no wider than the defect — every other C0 control, +plus DEL, NEL, ZWSP, BOM and U+2028, survives — and identity is untouched, +because keys and namespaces travel in metadata, which the service does not +sanitise. Callers that might hold either character should strip or replace it +first; `U+FFFD` in particular arrives in any text that has been through a lossy +decode (issue #80). + +Behaviour like that is visible only against the real service, so +`tinymemory-remote` carries a live target that runs the full contract suite +against a hosted endpoint when credentials are present and skips when they are +not. Point it at a scratch account: the suite writes and deletes records. + +```bash +TINYMEMORY_TEST_SUPERMEMORY_URL=https://api.supermemory.ai \ +TINYMEMORY_TEST_SUPERMEMORY_KEY=sm_... \ + cargo test -p tinymemory-remote --test live_remote_engines +``` + ## Development ```bash diff --git a/crates/tinymemory-remote/src/supermemory.rs b/crates/tinymemory-remote/src/supermemory.rs index c439ae7..0d867f0 100644 --- a/crates/tinymemory-remote/src/supermemory.rs +++ b/crates/tinymemory-remote/src/supermemory.rs @@ -3,6 +3,7 @@ use async_trait::async_trait; use reqwest::Method; use serde_json::{json, Value}; +use tinymemory_api::error::MemoryError; use tinymemory_api::recall::RecallOpts; use tinymemory_api::traits::Memory; use tinymemory_api::types::MemoryTaint; @@ -180,6 +181,40 @@ struct SupermemoryDialect { client: HttpClient, } +/// The characters Supermemory removes from stored content (issue #80). +/// +/// Measured against the live API rather than inferred from documentation: +/// `POST /v4/memories` echoes the stored value back in its own 201, and for +/// these two the echo is shorter than what was sent. Everything else offered +/// to it survives unchanged — every other C0 control, DEL, NEL, ZWSP, BOM and +/// U+2028 — so the refusal below stays as narrow as the defect. +/// +/// Only `content` is affected. Identity rides in `metadata`, which the service +/// does not sanitise: `tinymemory_key` and `tinymemory_namespace` round-trip +/// both characters intact, so a key is never quietly rewritten into another +/// key's. That is why [`Dialect::upsert`] inspects the content alone. +const CONTENT_CHARACTERS_SUPERMEMORY_DROPS: [char; 2] = ['\u{0}', '\u{FFFD}']; + +/// Returns the first character Supermemory would drop, and where it sits. +fn dropped_content_character(content: &str) -> Option<(usize, char)> { + content + .char_indices() + .find(|(_, character)| CONTENT_CHARACTERS_SUPERMEMORY_DROPS.contains(character)) +} + +/// Names a character without reproducing it. +/// +/// The name goes into an error message, and a raw NUL travels from there into +/// logs, terminals, and shells that render it as nothing — turning a precise +/// refusal into a message that appears to name no character at all. +fn character_name(character: char) -> String { + match character { + '\u{0}' => "U+0000 (NUL)".to_string(), + '\u{FFFD}' => "U+FFFD (the replacement character)".to_string(), + other => format!("U+{:04X}", other as u32), + } +} + impl SupermemoryDialect { /// Maps an arbitrary TinyMemory namespace into Supermemory's bounded tag grammar. fn container_tag(namespace: &str) -> String { @@ -357,7 +392,36 @@ impl Dialect for SupermemoryDialect { } /// Replaces an existing exact record or creates a direct v4 memory. + /// + /// Refuses content Supermemory would alter. `MemoryCore::store` promises + /// that what is read back equals what was stored, and this service strips + /// [`CONTENT_CHARACTERS_SUPERMEMORY_DROPS`] server-side; storing anyway + /// would break that promise silently, which is the one outcome the + /// contract rules out — a driver may refuse a shape, but not accept one + /// and hand back something else. The check precedes the request because + /// the service answers `201` and alters the value in the same breath, so + /// there is no later point at which the adapter could still object. + /// + /// # Errors + /// + /// [`MemoryError::Invalid`] — the refusal class for caller input a driver + /// rejects — carried as the anyhow payload every other typed error here + /// uses, so a caller can match on it after the usual downcast. async fn upsert(&self, entry: StoredEntry) -> anyhow::Result<()> { + if let Some((at, character)) = dropped_content_character(&entry.content) { + return Err(anyhow::Error::new(MemoryError::Invalid(format!( + // Debug-escaped, not raw: metadata is not sanitised, so an + // identity may itself hold a NUL — and a refusal that emits + // one lands in the same logs and terminals that render it as + // nothing, which is the failure this message exists to avoid. + "supermemory removes {} from stored content, so {:?}/{:?} would read back \ + changed (first occurrence at byte {at}); remove the character, or store \ + this record through a driver that preserves it", + character_name(character), + entry.namespace, + entry.key + )))); + } let existing = self.find_entry(&entry.namespace, &entry.key).await?; let metadata = Self::metadata(&entry); if let Some(existing) = existing { diff --git a/crates/tinymemory-remote/src/supermemory_test.rs b/crates/tinymemory-remote/src/supermemory_test.rs index 284b765..7719828 100644 --- a/crates/tinymemory-remote/src/supermemory_test.rs +++ b/crates/tinymemory-remote/src/supermemory_test.rs @@ -12,6 +12,7 @@ use axum::{ }; use serde_json::{json, Value}; use tinymemory_api::{ + error::MemoryError, provider::{MemoryCore, MemoryProvider, MemoryRecall}, recall::OwnedRecallOpts, traits::Memory, @@ -323,3 +324,204 @@ async fn keyed_reads_scope_to_one_container_tag() { "the request names exactly the namespace's tag" ); } + +/// Spawns the Supermemory double and returns its fixture beside a driver. +/// +/// The refusal tests below assert on what the double *did not* receive, so +/// they need the fixture as much as the driver. +async fn spawn_double() -> (AppState, impl MemoryProvider) { + let state = AppState::default(); + let app = Router::new() + .route("/v3/container-tags/list", get(tags)) + .route("/v4/memories/list", post(list)) + .route("/v4/memories", post(add).patch(update).delete(remove)) + .route("/v4/search", post(search)) + .route("/", get(|| async { StatusCode::OK })) + .with_state(state.clone()); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind"); + let endpoint = format!("http://{}", listener.local_addr().expect("address")); + tokio::spawn(async move { + axum::serve(listener, app).await.expect("serve"); + }); + let driver = crate::supermemory_provider( + super::SupermemoryMemory::self_hosted(&endpoint, "secret").expect("client"), + ); + (state, driver) +} + +/// Issue #80: Supermemory removes NUL and U+FFFD from `content` server-side. +/// +/// `MemoryCore::store` promises the content read back equals the content +/// stored, so the adapter must refuse rather than store a value the service +/// will quietly rewrite. `Invalid` is the documented refusal class — what is +/// being rejected is the caller's input. +#[tokio::test] +async fn content_supermemory_would_alter_is_refused() { + let (state, driver) = spawn_double().await; + for (label, ch) in [("NUL", '\u{0}'), ("replacement char", '\u{FFFD}')] { + let content = format!("a{ch}b"); + let result = driver + .store( + "project", + "decision", + &content, + MemoryCategory::Core, + None, + MemoryTaint::Internal, + ) + .await; + assert!( + matches!(result, Err(MemoryError::Invalid(_))), + "{label}: content the service would alter must be refused as Invalid, \ + got {result:?}" + ); + } + // Refused before the request, not after a round trip: the service accepts + // this content with a 201 and alters it, so an adapter that asked first + // would have already lost the character by the time it could object. + assert!( + state.0.lock().expect("state lock").records.is_empty(), + "the refusal must short-circuit before any write reaches the service" + ); +} + +/// The refusal has to be actionable without re-emitting the character: a raw +/// NUL in an error string propagates into logs, terminals, and shells that +/// hide it, turning a clear refusal into a confusing one. +#[tokio::test] +async fn the_refusal_names_the_character_without_emitting_it() { + let (_state, driver) = spawn_double().await; + let result = driver + .store( + "project", + "decision", + "a\u{0}b", + MemoryCategory::Core, + None, + MemoryTaint::Internal, + ) + .await; + assert!(result.is_err(), "content carrying a NUL must be refused"); + let message = result + .err() + .map(|error| error.to_string()) + .unwrap_or_default(); + assert!( + message.contains("U+0000"), + "the message names the character: {message}" + ); + assert!( + !message.contains('\u{0}'), + "the message must not carry the character itself: {message:?}" + ); +} + +/// A refusal must stay clean even when the identity is not. +/// +/// Metadata is not sanitised, so a namespace or key may itself carry a NUL and +/// be stored perfectly happily. That makes the two halves meet here: content +/// the service would alter, under an identity that holds a control character. +/// The message interpolates the identity, so this is where a refusal would +/// leak the very character the naming exists to keep out of logs. +#[tokio::test] +async fn a_refusal_emits_no_control_character_from_the_identity_either() { + let (_state, driver) = spawn_double().await; + let result = driver + .store( + "project\u{0}alpha", + "decision\u{0}beta", + "a\u{0}b", + MemoryCategory::Core, + None, + MemoryTaint::Internal, + ) + .await; + assert!(result.is_err(), "the content must still be refused"); + let message = result + .err() + .map(|error| error.to_string()) + .unwrap_or_default(); + assert!( + !message.chars().any(|character| character.is_control()), + "the refusal must carry no control character at all: {message:?}" + ); + assert!( + message.contains("project") && message.contains("decision"), + "the identity is still named, just escaped: {message}" + ); +} + +/// The predicate stays as narrow as the defect. +/// +/// Only these two characters were measured as dropped; every other C0 control +/// — plus DEL, NEL, ZWSP, BOM and U+2028 — survives the live service intact. +/// A refusal that widened to "control characters" would reject content +/// Supermemory stores perfectly well, and `assert_awkward_content_round_trips` +/// would still pass while the driver quietly became less useful. +#[tokio::test] +async fn content_supermemory_preserves_is_still_stored() { + let (state, driver) = spawn_double().await; + for (label, content) in [ + ("C0 control", "a\u{1}b".to_string()), + ("tab and newlines", "a\tb\nc\r\nd".to_string()), + ("unicode", "héllo — 👋 まいど".to_string()), + ( + "zero-width space and BOM", + "a\u{200B}b\u{FEFF}c".to_string(), + ), + ] { + let result = driver + .store( + "project", + label, + &content, + MemoryCategory::Core, + None, + MemoryTaint::Internal, + ) + .await; + assert!(result.is_ok(), "{label} must still store: {result:?}"); + } + assert_eq!( + state.0.lock().expect("state lock").records.len(), + 4, + "every preserved shape reached the service" + ); +} + +/// The refusal is scoped to `content`, because the defect is. +/// +/// Identity rides in `metadata`, which the live service does not sanitise: +/// `tinymemory_key` and `tinymemory_namespace` round-trip both characters +/// unchanged, so a key is never silently rewritten into another key's. Were +/// that untrue the failure would be worse than mangled content — a re-store +/// would stop matching its own record and duplicate it instead. This pins the +/// scoping to the measurement, so widening it later has to re-measure first. +#[tokio::test] +async fn identity_carrying_the_same_characters_is_not_refused() { + let (_state, driver) = spawn_double().await; + for (label, character) in [("nul", '\u{0}'), ("replacement", '\u{FFFD}')] { + let namespace = format!("project{character}alpha"); + let key = format!("decision{character}beta"); + let content = format!("ordinary content for {label}"); + driver + .store( + &namespace, + &key, + &content, + MemoryCategory::Core, + None, + MemoryTaint::Internal, + ) + .await + .expect("metadata is not sanitised, so identity is not refused"); + let stored = driver.get(&namespace, &key).await.expect("get"); + assert_eq!( + stored.map(|entry| entry.content), + Some(content), + "{label}: the record is reachable under the identity it was stored with" + ); + } +} diff --git a/crates/tinymemory-remote/tests/live_remote_engines.rs b/crates/tinymemory-remote/tests/live_remote_engines.rs new file mode 100644 index 0000000..6aed17b --- /dev/null +++ b/crates/tinymemory-remote/tests/live_remote_engines.rs @@ -0,0 +1,50 @@ +//! The contract suite against a real hosted service, not a double. +//! +//! Every other test in this crate runs an adapter over a double written from +//! the same documentation the adapter was. That agreement is worth having, but +//! it cannot catch a service that behaves differently from its documentation — +//! and when it does, the adapter and the double are wrong together and the +//! suite stays green. Issue #80 is exactly that: Supermemory strips two +//! characters from stored content server-side, and nothing here could see it +//! because nothing here ever spoke to Supermemory. +//! +//! So this target exists to be pointed at the real thing. It is skipped unless +//! the credentials are present, which keeps `cargo test` offline, +//! deterministic, and independent of a vendor's uptime by default. +//! +//! ```sh +//! TINYMEMORY_TEST_SUPERMEMORY_URL=https://api.supermemory.ai \ +//! TINYMEMORY_TEST_SUPERMEMORY_KEY=sm_... \ +//! cargo test -p tinymemory-remote --test live_remote_engines +//! ``` +//! +//! The suite writes and deletes records under its own namespaces in whatever +//! account the key belongs to. Point it at a scratch account rather than one +//! holding anything you would miss. + +use std::sync::Arc; + +use tinymemory_remote::{supermemory_provider, SupermemoryMemory}; + +/// Reads one engine's endpoint and key, or `None` when either is unset. +/// +/// Both are required rather than defaulting the URL: a live test that invents +/// its own endpoint can end up silently exercising the wrong service. +fn credentials(engine: &str) -> Option<(String, String)> { + let url = std::env::var(format!("TINYMEMORY_TEST_{engine}_URL")).ok()?; + let key = std::env::var(format!("TINYMEMORY_TEST_{engine}_KEY")).ok()?; + (!url.is_empty() && !key.is_empty()).then_some((url, key)) +} + +/// Runs the full provider contract against the live Supermemory API. +/// +/// Skipped without `TINYMEMORY_TEST_SUPERMEMORY_URL` and `..._KEY`. +#[tokio::test] +async fn live_supermemory_upholds_the_provider_contract() -> anyhow::Result<()> { + let Some((url, key)) = credentials("SUPERMEMORY") else { + return Ok(()); + }; + let provider = supermemory_provider(SupermemoryMemory::api(&url, &key)?); + tinymemory_conformance::assert_provider(Arc::new(provider)).await; + Ok(()) +}