From 32c1d411db9f761b74d5d65d3996ea3c02b3587f Mon Sep 17 00:00:00 2001 From: docwilde Date: Wed, 11 Nov 2026 14:43:33 -0700 Subject: [PATCH] fix(memory): reject empty replacements and verify historical sync replay --- CHANGELOG.md | 11 +++ lore_core/memory.py | 5 +- rust/lore-core/src/memory.rs | 97 ++++++++++++++++++- rust/lore-core/tests/native_network.rs | 127 +++++++++++++++++++++++++ tests/test_write_gate.py | 23 +++++ 5 files changed, 258 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca565c2..81d9108 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## Unreleased + +- Fix **empty memory replacements** (#132): Rust direct/internal mutations and + Python `memory_replace` reject text that normalizes to empty before changing + the entry, provenance or sync log. Native model/derived requests are refused + before staging; deletion still uses `memory remove`. +- Add end-to-end coverage for **historical belief recovery** (#112), implemented + in 0.62.7: authenticated peer replay preserves signed quarantine, banks its + cursor, matches portable state, and remains idempotent. Unsigned hub copies + stay unapplied; their receipt cursor alone cannot prove recovery. + ## 0.62.25 — 2026-10-20 **Memory match errors** diff --git a/lore_core/memory.py b/lore_core/memory.py index 0846d46..f359aed 100644 --- a/lore_core/memory.py +++ b/lore_core/memory.py @@ -253,6 +253,9 @@ def memory_replace(scope: str, slug: str, needle: str, text: str, *, via: str = "direct", origin: "str | None" = None, source_engine: "str | None" = None, writer: "str | None" = None, exact: bool = False) -> str | None: + new = one_line(text) + if not new: + return "empty text" path = memory_path(scope, slug) entries = read_entries(path) hits = ([i for i, entry in enumerate(entries) if entry == needle] @@ -263,7 +266,7 @@ def memory_replace(scope: str, slug: str, needle: str, text: str, *, if len(hits) > 1: listing = "\n".join(f" - {entries[i]}" for i in hits) return f"{needle!r} is ambiguous ({len(hits)} matches) — use a longer substring:\n{listing}" - old, new = entries[hits[0]], one_line(text) + old = entries[hits[0]] entries[hits[0]] = new err = write_entries(path, entries, memory_cap(scope), scope) if err is None: diff --git a/rust/lore-core/src/memory.rs b/rust/lore-core/src/memory.rs index d04e260..e1da00a 100644 --- a/rust/lore-core/src/memory.rs +++ b/rust/lore-core/src/memory.rs @@ -385,11 +385,11 @@ pub fn mutate_locked( let mut entries = read_entries(&path)?; let bucket = scope.bucket(key); let text = gate::one_line(&crate::scrub::scrub(text)?); + if action != "remove" && text.is_empty() { + return Err(Error::InvalidRequest); + } let old = match action { "add" => { - if text.is_empty() { - return Err(Error::InvalidRequest); - } if entries .iter() .any(|entry| entry.to_lowercase() == text.to_lowercase()) @@ -562,7 +562,7 @@ pub fn direct_action(cfg: &Config, req: &Value, authority: &Authority) -> Result .map_or(Some(""), Value::as_str) .ok_or(Error::InvalidRequest)?; let text = gate::one_line(&crate::scrub::scrub(text)?); - if action == "add" && text.is_empty() { + if action != "remove" && text.is_empty() { return Err(Error::InvalidRequest); } // An empty substring matches every entry, so it can only ever be refused @@ -988,6 +988,95 @@ mod tests { assert_eq!(action(&cfg, &action_req, &auth()), Err(Error::Changed)); } #[test] + fn empty_replacements_preserve_memory_provenance_and_sync_log() { + let temp = tempfile::tempdir().unwrap(); + let cfg = Config::for_root(temp.path().join("lore")); + let path = cfg.root.join("USER.md"); + mutate( + &cfg, + Scope::User, + "", + "add", + "", + "load-bearing fact", + "direct", + None, + None, + &auth(), + ) + .unwrap(); + let snapshot = || { + let conn = crate::store::connect(&cfg).unwrap(); + let ops: i64 = conn + .query_row("SELECT count(*) FROM sync_ops", [], |row| row.get(0)) + .unwrap(); + ( + fs::read(&path).unwrap(), + fs::read(cfg.root.join("provenance.json")).unwrap(), + ops, + ) + }; + let before = snapshot(); + for text in ["", " ", "\t\r\n", "\u{00a0}\u{2003}"] { + for authority in [ + auth(), + Authority::Interactive { + agent: "fixture".into(), + engine: "codex".into(), + }, + Authority::Model { + agent: "fixture".into(), + engine: "codex".into(), + session_id: "s".into(), + }, + Authority::Derived { + agent: "fixture".into(), + engine: "claude".into(), + }, + ] { + let req = json!({"cwd":temp.path(),"scope":"user","action":"replace","match":"load-bearing fact","text":text}); + assert_eq!( + direct_action(&cfg, &req, &authority), + Err(Error::InvalidRequest), + "{text:?} {authority:?}" + ); + assert_eq!(snapshot(), before); + assert!(!cfg.root.join("pending").exists()); + } + for verb in ["replace", "replace-exact"] { + assert_eq!( + mutate( + &cfg, + Scope::User, + "", + verb, + "load-bearing fact", + text, + "direct", + None, + None, + &auth() + ), + Err(Error::InvalidRequest), + "{verb} {text:?}" + ); + assert_eq!(snapshot(), before); + } + } + let req = json!({"cwd":temp.path(),"scope":"user","action":"replace","match":"load-bearing fact"}); + assert_eq!( + direct_action(&cfg, &req, &auth()), + Err(Error::InvalidRequest) + ); + assert_eq!(snapshot(), before); + let req = json!({"cwd":temp.path(),"scope":"user","action":"remove","match":"load-bearing fact"}); + assert_eq!( + direct_action(&cfg, &req, &auth()).unwrap()["status"], + "applied" + ); + assert!(read_entries(&path).unwrap().is_empty()); + } + #[test] fn direct_substring_misses_name_the_match_and_leave_memory_alone() { let temp = tempfile::tempdir().unwrap(); let cfg = Config::for_root(temp.path().join("lore")); diff --git a/rust/lore-core/tests/native_network.rs b/rust/lore-core/tests/native_network.rs index f5aaef2..dc6fb07 100644 --- a/rust/lore-core/tests/native_network.rs +++ b/rust/lore-core/tests/native_network.rs @@ -1268,6 +1268,133 @@ fn cursor(cfg: &Config, transport: &Transport) -> String { ) .unwrap() } + +#[test] +fn historical_belief_peer_replay_banks_cursor_and_preserves_signed_quarantine() { + // #112: a fresh stream must retain signed history, including quarantined + // entries, rather than repairing missing identities or unsigned hub copies. + let (temp, source) = config(); + let cases = [ + ("insert", json!({"uid":"retained-belief","subject":"user", + "claim":"Synthetic historical belief","confidence":0.8})), + ("reinforce", json!({"uid":null,"confidence":0.9})), + ("edge", json!({"src_uid":"retained-belief","dst_uid":null, + "rel":"explains","source":"derived"})), + ("supersede", json!({"uid":"retained-belief","by_uid":null, + "reason":"Synthetic missing historical identity"})), + ]; + let ops: Vec = cases.iter().enumerate().map(|(i, (verb, payload))| { + let mut value = op("unused", i as i64 + 1); + value["class"] = json!("belief"); + value["op"] = json!(verb); + value["payload"] = payload.clone(); + value["mac"] = json!(lore_core::sync_apply::compute_mac(&value, KEY).unwrap()); + value + }).collect(); + let admitted = lore_core::sync_apply::apply_ops(&source, &ops).unwrap(); + assert_eq!(admitted["applied"], 1); + assert_eq!(admitted["quarantined"], 3); + + // Old immutable unsigned copies must still fail admission, including null + // references: quarantine is available only after successful verification. + let unsigned: Vec = ops.iter().enumerate().map(|(i, op)| { + let mut value = op.clone(); + value["mac"] = Value::Null; + value["hub_seq"] = json!(i + 1); + value + }).collect(); + let (_old_temp, old_destination) = config(); + let (old_url, old_server) = fixture(vec![(200, json!({"ops":unsigned,"next":null}))]); + let old_transport = Transport::new(&old_url, None, "hub".into()).unwrap(); + let refused = net::pull(&old_destination, &old_transport).unwrap(); + assert_eq!(refused["unverified"], 4); + assert_eq!(refused["quarantined"], 0); + assert_eq!(refused["applied"], 0); + // A receipt cursor can bank after durable unverified staging. It cannot + // establish the issue's stronger zero-unverified replay success gate. + assert_eq!(refused["cursor_advanced"], true); + assert_eq!(cursor(&old_destination, &old_transport), "4"); + assert_eq!(store::read_only(&old_destination).unwrap().query_row( + "SELECT count(*) FROM beliefs", [], |r| r.get::<_, i64>(0) + ).unwrap(), 0); + old_server.join().unwrap(); + + let server = PeerServer { + machine_id: "historical-source".into(), + cfg: source.clone(), + loopback: true, + auth: "none".into(), + allow: BTreeSet::new(), + secret: Some("historical-fixture-secret".into()), + }; + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let task = thread::spawn(move || { + for _ in 0..2 { + server.connection(listener.accept().unwrap().0).unwrap(); + } + }); + let (_dest_temp, destination) = config(); + let transport = Transport::new(&url, Some("historical-fixture-secret".into()), + "peer:historical-source".into()).unwrap(); + let report = net::pull(&destination, &transport).unwrap(); + assert_eq!(report["fetched"], 4); + assert_eq!(report["applied"], 1); + assert_eq!(report["quarantined"], 3); + for field in ["unverified", "failed", "deferred", "unknown", "skipped"] { + assert_eq!(report[field], 0, "{field}: {report}"); + } + assert_eq!(report["cursor_advanced"], true); + assert_eq!(cursor(&destination, &transport), "4"); + let conn = store::read_only(&destination).unwrap(); + for original in &ops { + let (payload, mac, state): (String, String, i64) = conn.query_row( + "SELECT payload,mac,applied FROM sync_ops WHERE op_id=?", + [original["op_id"].as_str().unwrap()], + |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), + ).unwrap(); + let mut retained = original.clone(); + retained["payload"] = serde_json::from_str(&payload).unwrap(); + retained["mac"] = json!(mac); + assert_eq!(retained, *original); + assert!(lore_core::sync_apply::verify_mac(&retained, Some(KEY))); + assert_eq!(state, if original["op"] == "insert" { 1 } else { 5 }); + } + // A log digest alone does not prove state convergence. Compare this + // fixture's portable belief fields and related counts as well. + let belief_state = |cfg: &Config| { + let conn = store::read_only(cfg).unwrap(); + let state: Value = conn.query_row( + "SELECT uid,subject,claim,confidence,status FROM beliefs", + [], |r| Ok(json!([r.get::<_, String>(0)?, r.get::<_, String>(1)?, + r.get::<_, String>(2)?, r.get::<_, f64>(3)?, + r.get::<_, String>(4)?])), + ).unwrap(); + for table in ["belief_evidence", "belief_edges", "belief_outcomes"] { + let count: i64 = conn.query_row(&format!("SELECT count(*) FROM {table}"), + [], |r| r.get(0)).unwrap(); + assert_eq!(count, if table == "belief_evidence" { 1 } else { 0 }); + } + assert_eq!(conn.query_row("SELECT count(*) FROM beliefs", [], + |r| r.get::<_, i64>(0)).unwrap(), 1); + lore_core::digest(&store::canonical_bytes(&state).unwrap()) + }; + assert_eq!(belief_state(&source), belief_state(&destination)); + let source_bundle = temp.path().join("source.json"); + let replay_bundle = temp.path().join("replay.json"); + net::export_bundle(&source, &source_bundle).unwrap(); + net::export_bundle(&destination, &replay_bundle).unwrap(); + let bundle = |path| serde_json::from_slice::(&std::fs::read(path).unwrap()).unwrap(); + assert_eq!(bundle(&source_bundle), bundle(&replay_bundle)); + + let repeated = net::pull_from(&destination, &transport, Some(0), false).unwrap(); + assert_eq!(repeated["duplicate"], 4); + assert_eq!(repeated["applied"], 0); + assert_eq!(cursor(&destination, &transport), "4"); + assert_eq!(belief_state(&source), belief_state(&destination)); + task.join().unwrap(); +} + #[test] fn large_signed_session_hub_pull_keeps_memory_order_signature_and_duplicates() { let (_temp, mut cfg) = config(); diff --git a/tests/test_write_gate.py b/tests/test_write_gate.py index 5e7e757..2feabdd 100644 --- a/tests/test_write_gate.py +++ b/tests/test_write_gate.py @@ -240,6 +240,29 @@ def test_codex_add_replace_and_remove_apply_immediately(self): self.assertEqual(_entries(), []) self.assertEqual(_pending_items(), []) + def test_empty_replacement_preserves_memory_provenance_and_sync_log(self): + with as_writer(AGENT_ENV): + for scope in ("user", "project", "machine"): + slug = SLUG if scope != "machine" else "fixture-host" + self.assertIsNone(lore.memory_add(scope, slug, "load-bearing fact")) + path = lore.memory_path(scope, slug) + before = path.read_bytes() + provenance = lore.PROVENANCE_PATH().read_bytes() + with contextlib.closing(lore.db_connect()) as conn: + ops = conn.execute("SELECT * FROM sync_ops ORDER BY seq").fetchall() + for exact in (False, True): + for text in ("", " ", "\t\r\n", "\u00a0\u2003"): + with self.subTest(scope=scope, exact=exact, text=text): + self.assertEqual(lore.memory_replace( + scope, slug, "load-bearing fact", text, exact=exact), "empty text") + self.assertEqual(path.read_bytes(), before) + self.assertEqual(lore.PROVENANCE_PATH().read_bytes(), provenance) + with contextlib.closing(lore.db_connect()) as conn: + self.assertEqual(conn.execute( + "SELECT * FROM sync_ops ORDER BY seq").fetchall(), ops) + self.assertIsNone(lore.memory_remove(scope, slug, "load-bearing fact")) + self.assertEqual(lore.read_entries(path), []) + def test_hook_replace_and_remove_cannot_mutate_the_store(self): with as_writer(AGENT_ENV), quiet(): lore.cmd_memory(Namespace(mcmd="add", scope="project", cwd=CWD,