From e57cd98b68d6cabe17e3503c64e708d574977c79 Mon Sep 17 00:00:00 2001 From: oceanwaves630 <33291608+oceanwaves630@users.noreply.github.com> Date: Wed, 8 Jul 2026 23:58:15 -0400 Subject: [PATCH 1/3] feat(memory): optional namespace param on recall (#733) Adds an optional exact-match namespace string param to memory.recall. Absent: unchanged (reads the caller token's default visible namespace set). Present: candidate fetch (FTS + vector + the ANN over-fetch retry loop) scopes to exactly that namespace via NamespaceToken::with_namespace, mirroring handle_remember's existing write-namespace override pattern. Invalid values are rejected via Namespace::parse, never silently coerced. This is largely defense-in-depth: VerbRegistry::dispatch's Rule-3 explicit-namespace escape (ADR-007 Rev 4/6) already mints the token with the effective namespace before the handler runs, for any caller going through the registry. The explicit RecallParams.namespace field + ParamDef entry make the escape hatch handler-visible/documented and safe for direct (non-dispatch) callers. Adds regression + coverage tests for the absent/present/no-match/invalid cases and a dedicated ANN over-fetch retry-loop test proving the widening loop still respects the effective (narrowed) namespace, with a widening- disabled control case. Adds docs/benchmarks/recall-arm-isolation.md: recipe for an isolated recall measurement arm via memory.remember/recall namespace + the ADR-104 profile_id override, with an explicit list of what remains un-isolated (brain.auto_feedback posteriors, knowledge.compose). Co-Authored-By: Claude Fable 5 --- .../khive-pack-memory/src/handlers/common.rs | 16 + .../khive-pack-memory/src/handlers/recall.rs | 344 +++++++++++++++++- .../khive-pack-memory/src/handlers/tests.rs | 5 + crates/khive-pack-memory/src/pack.rs | 6 + docs/benchmarks/recall-arm-isolation.md | 71 ++++ 5 files changed, 440 insertions(+), 2 deletions(-) create mode 100644 docs/benchmarks/recall-arm-isolation.md diff --git a/crates/khive-pack-memory/src/handlers/common.rs b/crates/khive-pack-memory/src/handlers/common.rs index 8fbdab8d..11eb6785 100644 --- a/crates/khive-pack-memory/src/handlers/common.rs +++ b/crates/khive-pack-memory/src/handlers/common.rs @@ -264,6 +264,22 @@ pub(super) struct RecallParams { /// a silent fallback to defaults. #[serde(default)] pub(super) profile_id: Option, + /// ADR-007 Rev 6 §"multi-record ops default to local + explicit escape" + /// (#733): exact-match read-namespace override. When absent, recall reads + /// the caller token's namespace (which defaults to `local`) — byte-identical + /// to pre-#733 behavior. When present, the candidate fetch (FTS + vector + + /// the ANN over-fetch retry loop) is scoped to exactly this namespace + /// instead of the token's (possibly wider) visible-namespace set. Invalid + /// values are rejected via the same `Namespace::parse` machinery used + /// elsewhere — never silently coerced. + /// + /// This is normally already pre-applied by `VerbRegistry::dispatch`'s + /// Rule-3 explicit-namespace escape (the token this handler receives + /// already carries `visible=[namespace]` in that path) — this field's + /// handling below is defense-in-depth for direct (non-dispatch) callers, + /// mirroring `RememberParams::namespace` / `handle_remember`. + #[serde(default)] + pub(super) namespace: Option, } impl RecallParams { diff --git a/crates/khive-pack-memory/src/handlers/recall.rs b/crates/khive-pack-memory/src/handlers/recall.rs index 7415d35c..55aabb1b 100644 --- a/crates/khive-pack-memory/src/handlers/recall.rs +++ b/crates/khive-pack-memory/src/handlers/recall.rs @@ -10,7 +10,9 @@ use uuid::Uuid; use khive_brain_core::PackTunable; use khive_fusion::FusionStrategy; -use khive_runtime::{micros_to_iso, NamespaceToken, RuntimeError, SearchSource, VerbRegistry}; +use khive_runtime::{ + micros_to_iso, Namespace, NamespaceToken, RuntimeError, SearchSource, VerbRegistry, +}; use khive_storage::types::{EdgeFilter, PageRequest}; use khive_storage::EdgeRelation; @@ -40,6 +42,30 @@ impl MemoryPack { let recall_start = Instant::now(); let p: RecallParams = deser(params)?; + + // #733: exact-match read-namespace escape. `VerbRegistry::dispatch`'s + // Rule-3 explicit-namespace escape already mints `token` with + // `visible=[namespace]` when the caller passed `namespace=` at the + // dispatch boundary, so this is normally a no-op re-derivation of the + // token we were already handed. It is defense-in-depth for direct + // (non-dispatch) callers, mirroring `handle_remember`'s identical + // pattern for the write-namespace override. Every subsequent use of + // `token` in this function (FTS/vector candidate fetch, the ANN + // over-fetch retry loop's visible-namespace gate, the supersedes + // graph read, and the serve-ledger namespace stamp) reads this + // shadowed binding, so the effective namespace flows uniformly + // through the whole pipeline. + let effective_token: NamespaceToken = match p.namespace.as_deref() { + Some(ns_str) => { + let ns = Namespace::parse(ns_str).map_err(|e| { + RuntimeError::InvalidInput(format!("invalid namespace {ns_str:?}: {e}")) + })?; + token.with_namespace(ns) + } + None => token.clone(), + }; + let token = &effective_token; + let prof = super::common::recall_profile_enabled(); let call_id = if prof { let id = RECALL_CALL_ID.fetch_add(1, Ordering::Relaxed); @@ -839,7 +865,7 @@ impl MemoryPack { #[cfg(test)] mod tests { - use std::collections::HashMap; + use std::collections::{HashMap, HashSet}; use std::sync::Arc; use async_trait::async_trait; @@ -3285,4 +3311,318 @@ mod tests { p95_with - p95_without, ); } + + // ── #733 slice 1: optional `namespace` param on memory.recall ────────── + + /// Seeds a fresh in-memory runtime with `kg` + `memory` registered, three + /// memories — two in `local`, one in `bench-a` — all sharing a query term + /// so a namespace-agnostic FTS/RRF recall would surface all three absent + /// any namespace filtering. Returns `(registry, local_id_1, local_id_2, + /// bench_id)`. + async fn ns733_seed_three_memories() -> (khive_runtime::VerbRegistry, Uuid, Uuid, Uuid) { + let rt = KhiveRuntime::memory().expect("in-memory runtime"); + let mut builder = VerbRegistryBuilder::new(); + builder.register(KgPack::new(rt.clone())); + builder.register(MemoryPack::new(rt.clone())); + let registry = builder.build().expect("registry"); + + async fn remember( + registry: &khive_runtime::VerbRegistry, + content: &str, + namespace: &str, + ) -> Uuid { + let result = registry + .dispatch( + "memory.remember", + serde_json::json!({ + "content": content, + "memory_type": "semantic", + "namespace": namespace, + }), + ) + .await + .expect("memory.remember"); + result["id"] + .as_str() + .expect("id") + .parse::() + .expect("valid uuid") + } + + let local_id_1 = remember(®istry, "ns733 probe term local arm one", "local").await; + let local_id_2 = remember(®istry, "ns733 probe term local arm two", "local").await; + let bench_id = remember(®istry, "ns733 probe term bench arm alpha", "bench-a").await; + + (registry, local_id_1, local_id_2, bench_id) + } + + /// Regression (spec item 1): `namespace` absent must be byte-identical to + /// pre-#733 behavior — recall reads the caller token's default visible + /// namespace set (`local` here), so a no-arg recall over a corpus with + /// two `local` memories and one `bench-a` memory surfaces only the two + /// `local` hits. + #[tokio::test] + #[serial(background_tasks)] + async fn ns733_recall_namespace_absent_regresses_to_local_only() { + let (registry, local_id_1, local_id_2, _bench_id) = ns733_seed_three_memories().await; + + let result = registry + .dispatch( + "memory.recall", + serde_json::json!({ + "query": "ns733 probe term", + "limit": 10 + }), + ) + .await + .expect("memory.recall with no namespace param"); + let hits = result.as_array().expect("bare array result"); + let ids: HashSet = hits + .iter() + .map(|h| h["id"].as_str().expect("id").parse::().expect("uuid")) + .collect(); + + assert_eq!( + ids, + HashSet::from([local_id_1, local_id_2]), + "no namespace param => must resolve to exactly the caller's default \ + visible namespace set (local), never bench-a: {hits:?}" + ); + } + + /// Spec item 2: `namespace="bench-a"` returns only the bench-a memory, + /// not either `local` memory — the exact-match escape narrows the read + /// scope instead of widening it. + #[tokio::test] + #[serial(background_tasks)] + async fn ns733_recall_namespace_explicit_returns_only_that_namespace() { + let (registry, _local_id_1, _local_id_2, bench_id) = ns733_seed_three_memories().await; + + let result = registry + .dispatch( + "memory.recall", + serde_json::json!({ + "query": "ns733 probe term", + "namespace": "bench-a", + "limit": 10 + }), + ) + .await + .expect("memory.recall with namespace=bench-a"); + let hits = result.as_array().expect("bare array result"); + let ids: HashSet = hits + .iter() + .map(|h| h["id"].as_str().expect("id").parse::().expect("uuid")) + .collect(); + + assert_eq!( + ids, + HashSet::from([bench_id]), + "namespace=\"bench-a\" must return exactly the bench-a memory and \ + neither local memory: {hits:?}" + ); + } + + /// Spec item 3: a `namespace` that matches nothing in the corpus returns + /// an empty result set with `ok:true` (dispatch succeeds), not an error. + #[tokio::test] + #[serial(background_tasks)] + async fn ns733_recall_namespace_no_match_returns_empty_ok() { + let (registry, ..) = ns733_seed_three_memories().await; + + let result = registry + .dispatch( + "memory.recall", + serde_json::json!({ + "query": "ns733 probe term", + "namespace": "bench-nonexistent", + "limit": 10 + }), + ) + .await + .expect("memory.recall with a namespace matching no memories must still be Ok"); + let hits = result.as_array().expect("bare array result"); + assert!( + hits.is_empty(), + "namespace matching no memories must yield an empty result set, got: {hits:?}" + ); + } + + /// Spec item 4: an invalid `namespace` string is a per-op error naming + /// the problem, never silent coercion to a fallback namespace. Validated + /// via the same `Namespace::parse` machinery used elsewhere (a space is + /// rejected — `NamespaceError::InvalidCharacter`). + #[tokio::test] + #[serial(background_tasks)] + async fn ns733_recall_invalid_namespace_is_a_per_op_error() { + let (registry, ..) = ns733_seed_three_memories().await; + + let result = registry + .dispatch( + "memory.recall", + serde_json::json!({ + "query": "ns733 probe term", + "namespace": "bad namespace", + "limit": 10 + }), + ) + .await; + + let err = result.expect_err( + "an invalid namespace string must be a per-op error, not a silent fallback", + ); + let msg = err.to_string(); + assert!( + msg.to_lowercase().contains("namespace"), + "error message must name the problem (namespace), got: {msg}" + ); + } + + const NS733_ANN_MODEL: &str = "ns733-ann-namespace-model"; + const NS733_QUERY: &str = "ns733 ann overfetch query"; + const NS733_TARGET_CONTENT: &str = "ns733 ann overfetch bench target"; + const NS733_FILLER_COUNT: usize = 35; + + /// 8-dim vectors, first two components carry signal (ADR-104 test pattern + /// reused here). Query = (1, 0) — cos 1.0 against itself. All 35 `local` + /// filler notes share an identical vector at cos 0.9 against the query; + /// the single `bench-a` target sits at cos 0.5, strictly below every + /// filler. Cosine-ranked purely by similarity, the target is therefore + /// guaranteed last (rank 36 of 36) — not a probabilistic near-miss. + fn ns733_ann_fixed_vectors() -> HashMap> { + let mut m = HashMap::new(); + m.insert( + NS733_QUERY.to_string(), + vec![1.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0], + ); + for i in 0..NS733_FILLER_COUNT { + m.insert( + format!("ns733 ann overfetch local filler {i}"), + vec![0.9, 0.4358899, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0], + ); + } + m.insert( + NS733_TARGET_CONTENT.to_string(), + vec![0.5, 0.8660254, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0], + ); + m + } + + /// Spec item 5: the ANN over-fetch retry loop (`config.rs`'s + /// "visible-namespace candidates" widening, `ann_overfetch_max_rounds`) + /// must respect the effective (explicit-namespace-narrowed) visible set, + /// not just eventually surface *a* result. + /// + /// Setup: a single global per-model ANN index (confirmed at the source — + /// `AnnKey` carries no namespace field, `ann.rs`: "One index per model + /// covers all namespaces") holds 35 `local` filler vectors all closer to + /// the query than the one `bench-a` target vector, with `candidate_limit` + /// pinned to 1 so the initial over-fetch window (`max(limit*4, + /// limit+32)` = 33) is narrower than the filler count — round 1 excludes + /// the target outright. This proves two things in one test: + /// + /// 1. With default widening (`ann_overfetch_max_rounds` unset, env + /// fallback 3): the retry loop widens past round 1, the target enters + /// the fetch window, and `namespace="bench-a"`'s post-filter still + /// returns *only* the target — none of the 35 `local` fillers ever + /// leak into the response despite sharing the same global ANN index. + /// 2. With widening explicitly disabled (`ann_overfetch_max_rounds: 1`, + /// per `config.rs`'s "Pass `Some(1)` to disable widening entirely"): + /// the same query against the same corpus returns nothing — proving + /// the round-1 result in case 1 was not a coincidence of corpus size, + /// but genuinely produced by the widening loop. + #[tokio::test] + #[serial(background_tasks)] + async fn ns733_recall_ann_overfetch_retry_loop_respects_effective_namespace() { + let rt = KhiveRuntime::memory().expect("in-memory runtime"); + rt.register_embedder(FixedVecProvider { + model_name: NS733_ANN_MODEL.to_string(), + map: ns733_ann_fixed_vectors(), + }); + + let mut builder = VerbRegistryBuilder::new(); + builder.register(KgPack::new(rt.clone())); + builder.register(MemoryPack::new(rt.clone())); + let registry = builder.build().expect("registry"); + + // No `embedding_model` on remember: `create_note_inner`'s auto-detect + // path fans out to every *registered* model when the field is + // omitted (`resolve_embedding_model` only accepts lattice aliases — + // an explicit custom provider name here would hit `UnknownModel`, + // same gotcha documented on `recall_with_residual_fts5_char_fails_loud` + // above). `NS733_ANN_MODEL` is the only model registered on this + // runtime, so auto-detect resolves to exactly it. + for i in 0..NS733_FILLER_COUNT { + registry + .dispatch( + "memory.remember", + serde_json::json!({ + "content": format!("ns733 ann overfetch local filler {i}"), + "memory_type": "semantic", + "namespace": "local", + }), + ) + .await + .expect("remember filler"); + } + let target_id = registry + .dispatch( + "memory.remember", + serde_json::json!({ + "content": NS733_TARGET_CONTENT, + "memory_type": "semantic", + "namespace": "bench-a", + }), + ) + .await + .expect("remember target")["id"] + .as_str() + .expect("id") + .parse::() + .expect("valid uuid"); + + let base_params = serde_json::json!({ + "query": NS733_QUERY, + "namespace": "bench-a", + "fusion_strategy": "vector_only", + "embedding_model": NS733_ANN_MODEL, + "config": { "candidate_limit": 1 }, + "limit": 1, + }); + + // Case 1: default widening — the target must be found, and only the target. + let widened_result = registry + .dispatch("memory.recall", base_params.clone()) + .await + .expect("memory.recall with default widening"); + let widened_hits = widened_result.as_array().expect("bare array result"); + assert_eq!( + widened_hits.len(), + 1, + "default widening must surface exactly the bench-a target, got: {widened_hits:?}" + ); + assert_eq!( + widened_hits[0]["id"] + .as_str() + .and_then(|s| s.parse::().ok()), + Some(target_id), + "the single hit must be the bench-a target, not a local filler" + ); + + // Case 2: widening disabled (`ann_overfetch_max_rounds: 1`) — round 1's + // narrow window is exhausted entirely by `local` fillers ranked ahead + // of the target, so the namespace-scoped post-filter finds nothing. + let mut disabled_params = base_params; + disabled_params["config"]["ann_overfetch_max_rounds"] = serde_json::json!(1); + let disabled_result = registry + .dispatch("memory.recall", disabled_params) + .await + .expect("memory.recall with widening disabled"); + let disabled_hits = disabled_result.as_array().expect("bare array result"); + assert!( + disabled_hits.is_empty(), + "with widening disabled, round 1's over-fetch window is saturated by \ + closer local fillers and must not reach the bench-a target: {disabled_hits:?}" + ); + } } diff --git a/crates/khive-pack-memory/src/handlers/tests.rs b/crates/khive-pack-memory/src/handlers/tests.rs index 84832b51..35f258fd 100644 --- a/crates/khive-pack-memory/src/handlers/tests.rs +++ b/crates/khive-pack-memory/src/handlers/tests.rs @@ -67,6 +67,7 @@ fn effective_config_uses_defaults() { entity_names: None, full_content: None, profile_id: None, + namespace: None, }; let cfg = p.effective_config(RecallConfig::default()); assert!((cfg.relevance_weight - 0.70).abs() < 1e-12); @@ -93,6 +94,7 @@ fn effective_config_legacy_overrides() { entity_names: None, full_content: None, profile_id: None, + namespace: None, }; let cfg = p.effective_config(RecallConfig::default()); assert!((cfg.min_score - 0.5).abs() < 1e-12); @@ -121,6 +123,7 @@ fn effective_config_explicit_config_wins() { entity_names: None, full_content: None, profile_id: None, + namespace: None, }; let cfg = p.effective_config(RecallConfig::default()); assert!((cfg.relevance_weight - 0.50).abs() < 1e-12); @@ -153,6 +156,7 @@ fn test_weighted_strategy_preserves_pack_weights() { entity_names: None, full_content: None, profile_id: None, + namespace: None, }; let mut cfg = p.effective_config(base); @@ -208,6 +212,7 @@ fn test_weighted_strategy_from_rrf_config_uses_vector_heavy_defaults() { entity_names: None, full_content: None, profile_id: None, + namespace: None, }; let mut cfg = p.effective_config(base); diff --git a/crates/khive-pack-memory/src/pack.rs b/crates/khive-pack-memory/src/pack.rs index a42904f4..ca822a26 100644 --- a/crates/khive-pack-memory/src/pack.rs +++ b/crates/khive-pack-memory/src/pack.rs @@ -253,6 +253,12 @@ static MEMORY_HANDLERS: [HandlerDef; 10] = [ required: false, description: "Tag filter mode: \"any\" (OR, default) or \"all\" (AND). Only applies when tags is non-empty.", }, + ParamDef { + name: "namespace", + param_type: "string", + required: false, + description: "Exact-match read-namespace override (ADR-007 Rev 6 escape hatch). When absent, reads the caller's default visible namespace set (unchanged default behavior). When present, scopes the candidate fetch to exactly this namespace; invalid values are rejected.", + }, ], }, HandlerDef { diff --git a/docs/benchmarks/recall-arm-isolation.md b/docs/benchmarks/recall-arm-isolation.md new file mode 100644 index 00000000..77cbab68 --- /dev/null +++ b/docs/benchmarks/recall-arm-isolation.md @@ -0,0 +1,71 @@ +# Isolating a recall measurement arm + +`memory.recall` supports an exact-match `namespace` parameter (issue #733) that +scopes the candidate fetch — FTS, vector search, and the ANN over-fetch retry +loop — to exactly one namespace, instead of the caller's default visible set. +Combined with `memory.remember`'s existing `namespace` write override and +`memory.recall`'s `profile_id` serving-profile override (ADR-104 §4), this is +enough to run an isolated A/B measurement arm without touching production +data: write to a scratch namespace, read from that same namespace, and pin +the serving profile so scoring weights don't drift mid-measurement. + +## Recipe + +1. **Pick an arm namespace.** Any string that parses as a valid namespace + (alphanumeric segments, `.` or `::` separators, no spaces) works. Prefix it + so it is obviously scratch data, e.g. `bench-arm-a`. + +2. **Write the arm's corpus** with an explicit namespace override: + + ```text + memory.remember(content="...", namespace="bench-arm-a") + memory.remember(content="...", namespace="bench-arm-a") + ``` + + Every memory written this way lands in `bench-arm-a` regardless of the + caller's actor or default namespace. + +3. **(Optional) create and pin a serving profile** for the arm, so the same + posterior state serves every read in the measurement window: + + ```text + brain.create_profile(namespace="bench-arm-a", name="bench-arm-a-recall-v1", consumer_kind="recall") + ``` + +4. **Read back through the same namespace**, with the profile pinned via the + ADR-104 `profile_id` override (bypasses binding resolution, so no + `brain.bind` is required for a scratch arm): + + ```text + memory.recall(query="...", namespace="bench-arm-a", profile_id="bench-arm-a-recall-v1") + ``` + + `namespace` here is an exact match, not a widened visible set — the + candidate fetch never sees memories from any other namespace, including + `local`. An invalid namespace string is a hard per-op error, not a silent + fallback. + +5. **Tear down** by deleting the arm's memories (`delete(type="memory", + id=..., hard=true)`) once the measurement is done, or simply let the arm + namespace age out unread — it costs nothing beyond the storage of its own + rows. + +## What is NOT yet isolated + +- **Feedback events.** `brain.auto_feedback` / `brain.feedback` write into the + _profile's_ live posterior state, not the namespace. If a measurement arm + pins an existing (non-scratch) profile via `profile_id`, feedback recorded + during the arm still trains that profile's posteriors going forward — there + is no namespace-scoped posterior isolation. Use a freshly created, + arm-specific profile (step 3) to avoid contaminating a shared profile's + state. Tracked as issue #733 remainder. +- **`knowledge.compose`.** The knowledge/lore corpus has no namespace filter + at all — composed knowledge is global regardless of which namespace the + caller's token carries. A measurement arm that includes compose calls is + not isolated from concurrent compose traffic elsewhere. +- **The serve ledger.** `brain.record_serve` stamps the _effective_ namespace + used for the fetch (the arm namespace when `namespace=` was passed), so + ledger rows are attributable to the arm — but the ledger itself is a single + shared table, not partitioned per arm; querying it for arm-specific + analysis means filtering by namespace after the fact, not scoping the + write. From ae5380b1277ed5bee3bdb912829d156c50bbd456 Mon Sep 17 00:00:00 2001 From: oceanwaves630 <33291608+oceanwaves630@users.noreply.github.com> Date: Thu, 9 Jul 2026 00:33:30 -0400 Subject: [PATCH 2/3] fix(memory): address codex review findings on #733 recall namespace param Codex reviewed the #733 branch (APPROVE-WITH-FIXES) and returned three findings, all applied here: 1. [High] memory.recall's verbose multi-model breakdown (candidates.vector_candidates_per_model, include_breakdown=true) leaked raw, pre-hydration ANN candidate IDs from outside the effective namespace, in both handle_recall and the handle_recall_candidates sub-handler. Fixed by filtering each per-model hit list through the same memory_ids visible-namespace set that already scopes `results`, in both locations. Added a regression test verified non-vacuous by reverting the fix and confirming the leak reproduces. 2. [Medium] resolve_explicit_namespace (khive-runtime/src/pack.rs) dropped the caller's supplied namespace value from its error message. Fixed by including it in the Some(Value::String(ns_str)) arm only, per review scope. Strengthened the existing invalid-namespace test to assert the literal supplied value appears in the message (previously only checked for the word "namespace", which passed vacuously). 3. [Medium] docs/benchmarks/recall-arm-isolation.md corrected against source: namespace grammar (single-`:`-separated segments, no `::`), delete's actual param shape (id/kind/hard, not type=), and knowledge.compose's actual namespace scoping (token-scoped, not global -- it lacks an exact-match override, not all scoping). Also hardened two ANN-backed recall tests (the new breakdown regression test and the pre-existing ANN over-fetch retry test from e57cd98b) against a pre-existing, unrelated production race in ann.rs's fire-and-forget per-model warm cache: whichever queued rebuild acquires the per-model lock first wins the shared index slot permanently, even if it raced ahead of a still-in-flight sibling write. Confirmed via targeted debug capture that this is not a consequence of the namespace filter under test. No production ann.rs/pack.rs code changed for this; both tests now retry their whole seed-and-recall flow against a fresh runtime (bounded, 5 attempts) instead of touching the underlying cache. Co-Authored-By: Claude Fable 5 --- .../khive-pack-memory/src/handlers/recall.rs | 449 +++++++++++++++--- .../src/handlers/sub_handlers.rs | 8 + crates/khive-runtime/src/pack.rs | 2 +- docs/benchmarks/recall-arm-isolation.md | 26 +- 4 files changed, 399 insertions(+), 86 deletions(-) diff --git a/crates/khive-pack-memory/src/handlers/recall.rs b/crates/khive-pack-memory/src/handlers/recall.rs index 55aabb1b..d2575469 100644 --- a/crates/khive-pack-memory/src/handlers/recall.rs +++ b/crates/khive-pack-memory/src/handlers/recall.rs @@ -818,12 +818,25 @@ impl MemoryPack { } if is_verbose && candidates.vector_hits_per_model.len() > 1 { + // Review finding (#733 fix-round 1, High): the ANN index is global + // across namespaces (`ann.rs`: "one index per model covers all + // namespaces"), so `candidates.vector_hits_per_model` still + // carries raw, pre-hydration over-fetch candidates from outside + // the effective namespace at this point — unlike `results` above, + // which is scoped via `memory_ids` (populated by + // `load_memory_candidate_notes`'s visible-namespace post-filter). + // Filter each per-model list through the same `memory_ids` set + // before serializing it into this diagnostic breakdown, or a + // verbose multi-model recall with an explicit `namespace=` can + // leak off-namespace candidate UUIDs even though `results` itself + // stays correctly scoped. let per_model: Vec = candidates .vector_hits_per_model .iter() .map(|(model, hits)| { let hits_json: Vec = hits .iter() + .filter(|h| memory_ids.contains(&h.subject_id)) .map(|h| { json!({ "id": h.subject_id.to_string(), @@ -3472,9 +3485,17 @@ mod tests { "an invalid namespace string must be a per-op error, not a silent fallback", ); let msg = err.to_string(); + // Review finding (#733 fix-round 1, Medium): asserting only that the + // message contains the word "namespace" passes vacuously (every + // variant of this error, valid or not, contains that word). Assert + // the *supplied* invalid value itself appears, proving the error + // actually names the problem rather than a generic namespace + // complaint (`resolve_explicit_namespace` in + // `khive-runtime/src/pack.rs`, the path this dispatched call goes + // through, now includes `{ns_str:?}` in its message). assert!( - msg.to_lowercase().contains("namespace"), - "error message must name the problem (namespace), got: {msg}" + msg.contains("bad namespace"), + "error message must name the supplied invalid value \"bad namespace\", got: {msg}" ); } @@ -3534,95 +3555,371 @@ mod tests { #[tokio::test] #[serial(background_tasks)] async fn ns733_recall_ann_overfetch_retry_loop_respects_effective_namespace() { - let rt = KhiveRuntime::memory().expect("in-memory runtime"); - rt.register_embedder(FixedVecProvider { - model_name: NS733_ANN_MODEL.to_string(), - map: ns733_ann_fixed_vectors(), - }); + // See the identical race documented on + // `ns733b_recall_verbose_multi_model_breakdown_excludes_off_namespace_candidates` + // below: `ensure_ann_background`'s fire-and-forget per-model warm + // (ann.rs) installs whichever queued build's scan acquires the + // per-model lock first, permanently, even if it raced ahead of a + // still-in-flight `remember` and snapshotted an incomplete corpus. + // 35 rapid-fire `remember` calls before the one `bench-a` target is + // enough to occasionally trigger it here too — a pre-existing + // production characteristic, not a consequence of the namespace + // filter under test. Retry the whole seed-and-recall flow against a + // fresh `KhiveRuntime` (fresh scheduling) up to a few times. + const MAX_ATTEMPTS: usize = 5; + let mut last_failure: Option = None; + + 'attempt: for attempt in 1..=MAX_ATTEMPTS { + let rt = KhiveRuntime::memory().expect("in-memory runtime"); + rt.register_embedder(FixedVecProvider { + model_name: NS733_ANN_MODEL.to_string(), + map: ns733_ann_fixed_vectors(), + }); - let mut builder = VerbRegistryBuilder::new(); - builder.register(KgPack::new(rt.clone())); - builder.register(MemoryPack::new(rt.clone())); - let registry = builder.build().expect("registry"); + let mut builder = VerbRegistryBuilder::new(); + builder.register(KgPack::new(rt.clone())); + builder.register(MemoryPack::new(rt.clone())); + let registry = builder.build().expect("registry"); - // No `embedding_model` on remember: `create_note_inner`'s auto-detect - // path fans out to every *registered* model when the field is - // omitted (`resolve_embedding_model` only accepts lattice aliases — - // an explicit custom provider name here would hit `UnknownModel`, - // same gotcha documented on `recall_with_residual_fts5_char_fails_loud` - // above). `NS733_ANN_MODEL` is the only model registered on this - // runtime, so auto-detect resolves to exactly it. - for i in 0..NS733_FILLER_COUNT { - registry + // No `embedding_model` on remember: `create_note_inner`'s auto-detect + // path fans out to every *registered* model when the field is + // omitted (`resolve_embedding_model` only accepts lattice aliases — + // an explicit custom provider name here would hit `UnknownModel`, + // same gotcha documented on `recall_with_residual_fts5_char_fails_loud` + // above). `NS733_ANN_MODEL` is the only model registered on this + // runtime, so auto-detect resolves to exactly it. + for i in 0..NS733_FILLER_COUNT { + registry + .dispatch( + "memory.remember", + serde_json::json!({ + "content": format!("ns733 ann overfetch local filler {i}"), + "memory_type": "semantic", + "namespace": "local", + }), + ) + .await + .expect("remember filler"); + } + let target_id = registry .dispatch( "memory.remember", serde_json::json!({ - "content": format!("ns733 ann overfetch local filler {i}"), + "content": NS733_TARGET_CONTENT, "memory_type": "semantic", - "namespace": "local", + "namespace": "bench-a", }), ) .await - .expect("remember filler"); + .expect("remember target")["id"] + .as_str() + .expect("id") + .parse::() + .expect("valid uuid"); + + let base_params = serde_json::json!({ + "query": NS733_QUERY, + "namespace": "bench-a", + "fusion_strategy": "vector_only", + "embedding_model": NS733_ANN_MODEL, + "config": { "candidate_limit": 1 }, + "limit": 1, + }); + + // Case 1: default widening — the target must be found, and only the + // target. Poll (bounded) for the ANN warm to settle on the full + // corpus before treating a mismatch as a real assertion failure. + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(500); + let widened_hits: Vec; + loop { + let widened_result = registry + .dispatch("memory.recall", base_params.clone()) + .await + .expect("memory.recall with default widening"); + let hits = widened_result + .as_array() + .cloned() + .expect("bare array result"); + let settled = hits.len() == 1 + && hits[0]["id"].as_str().and_then(|s| s.parse::().ok()) + == Some(target_id); + if settled { + widened_hits = hits; + break; + } + if std::time::Instant::now() >= deadline { + last_failure = Some(format!( + "attempt {attempt}/{MAX_ATTEMPTS}: ANN warm for the bench-a target \ + did not settle within 500ms; last response: {hits:?}" + )); + continue 'attempt; + } + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + assert_eq!( + widened_hits.len(), + 1, + "default widening must surface exactly the bench-a target, got: {widened_hits:?}" + ); + assert_eq!( + widened_hits[0]["id"] + .as_str() + .and_then(|s| s.parse::().ok()), + Some(target_id), + "the single hit must be the bench-a target, not a local filler" + ); + + // Case 2: widening disabled (`ann_overfetch_max_rounds: 1`) — round 1's + // narrow window is exhausted entirely by `local` fillers ranked ahead + // of the target, so the namespace-scoped post-filter finds nothing. + let mut disabled_params = base_params; + disabled_params["config"]["ann_overfetch_max_rounds"] = serde_json::json!(1); + let disabled_result = registry + .dispatch("memory.recall", disabled_params) + .await + .expect("memory.recall with widening disabled"); + let disabled_hits = disabled_result.as_array().expect("bare array result"); + assert!( + disabled_hits.is_empty(), + "with widening disabled, round 1's over-fetch window is saturated by \ + closer local fillers and must not reach the bench-a target: {disabled_hits:?}" + ); + return; } - let target_id = registry - .dispatch( - "memory.remember", - serde_json::json!({ - "content": NS733_TARGET_CONTENT, - "memory_type": "semantic", - "namespace": "bench-a", - }), - ) - .await - .expect("remember target")["id"] - .as_str() - .expect("id") - .parse::() - .expect("valid uuid"); - let base_params = serde_json::json!({ - "query": NS733_QUERY, - "namespace": "bench-a", - "fusion_strategy": "vector_only", - "embedding_model": NS733_ANN_MODEL, - "config": { "candidate_limit": 1 }, - "limit": 1, - }); + panic!( + "ns733: corpus/ANN-warm race did not settle in {MAX_ATTEMPTS} fresh attempts; \ + last failure: {}", + last_failure.unwrap_or_default() + ); + } - // Case 1: default widening — the target must be found, and only the target. - let widened_result = registry - .dispatch("memory.recall", base_params.clone()) - .await - .expect("memory.recall with default widening"); - let widened_hits = widened_result.as_array().expect("bare array result"); - assert_eq!( - widened_hits.len(), - 1, - "default widening must surface exactly the bench-a target, got: {widened_hits:?}" + // ── #733 fix-round 1 (codex High): verbose multi-model breakdown must not + // leak off-namespace ANN candidate IDs ────────────────────────────────── + + const NS733B_MODEL_A: &str = "ns733b-breakdown-model-a"; + const NS733B_MODEL_B: &str = "ns733b-breakdown-model-b"; + const NS733B_QUERY: &str = "ns733b breakdown query"; + const NS733B_TARGET_CONTENT: &str = "ns733b breakdown bench target"; + const NS733B_FILLER_COUNT: usize = 5; + + /// Same fixed-vector scheme as `ns733_ann_fixed_vectors` (query cos 1.0, + /// `local` fillers cos 0.9, `bench-a` target cos 0.5) — reused for both + /// registered models so the ANN over-fetch genuinely returns filler IDs + /// under each model, not just the target. + fn ns733b_fixed_vectors() -> HashMap> { + let mut m = HashMap::new(); + m.insert( + NS733B_QUERY.to_string(), + vec![1.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0], ); - assert_eq!( - widened_hits[0]["id"] + for i in 0..NS733B_FILLER_COUNT { + m.insert( + format!("ns733b breakdown local filler {i}"), + vec![0.9, 0.4358899, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0], + ); + } + m.insert( + NS733B_TARGET_CONTENT.to_string(), + vec![0.5, 0.8660254, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0], + ); + m + } + + /// Codex review finding (#733 fix-round 1, High): with `namespace="bench-a"`, + /// more than one registered embedding model, and `include_breakdown=true`, + /// `memory.recall`'s verbose response embeds + /// `candidates.vector_candidates_per_model` — built directly from the + /// (pre-hydration, namespace-agnostic) global ANN over-fetch results, + /// bypassing the `memory_ids` visible-namespace filter that scopes + /// `results` itself. This seeds two registered models, five `local` + /// filler memories ranked closer to the query than the one `bench-a` + /// target (guaranteeing the raw per-model over-fetch actually contains + /// filler IDs under both models — a non-vacuous corpus), and asserts no + /// `local` filler UUID appears anywhere in the breakdown for either + /// model, while the target UUID is present. + #[tokio::test] + #[serial(background_tasks)] + async fn ns733b_recall_verbose_multi_model_breakdown_excludes_off_namespace_candidates() { + // Every `memory.remember` fires `ann::invalidate_namespace` (the ANN + // index is global per model, so any write clears *all* loaded slots) + // and re-queues `ensure_ann_background`'s fire-and-forget rebuild for + // every registered model (ann.rs). `ensure_ann_for_model` installs a + // finished build with `indexes.entry(key).or_insert(bridge)` and + // treats an already-present key as `AlreadyLoaded` — so whichever + // queued build's `spawn_blocking` scan happens to acquire the + // per-model lock *first* wins permanently, even if it raced ahead of + // a still-in-flight sibling write and snapshotted an incomplete + // corpus (the double-fingerprint check only discards a build that + // observes the corpus mutate *during its own* scan, which does not + // catch "raced ahead of a write that hadn't started yet"). Six rapid + // `remember` calls across two brand-new custom models is enough to + // occasionally trigger this pre-existing scheduling race, and once a + // stale entry wins, nothing here can invalidate it again (no further + // writes occur) — `memory.recall` polling never self-corrects. + // + // This is a genuine pre-existing production characteristic, not a + // consequence of the #733 namespace filter under test (confirmed via + // targeted debug capture: the losing runs consistently showed a + // 5-filler, target-absent vector index that never changed across a + // bounded poll window). Rather than touch the production ANN warm + // path, retry the whole corpus-seed-and-recall flow against a fresh + // `KhiveRuntime` (fresh scheduling, fresh race) up to a few times — + // deterministic in the success case (which is the overwhelming + // majority), bounded and loud on failure otherwise. + const MAX_ATTEMPTS: usize = 5; + let mut last_failure: Option = None; + + 'attempt: for attempt in 1..=MAX_ATTEMPTS { + let rt = KhiveRuntime::memory().expect("in-memory runtime"); + rt.register_embedder(FixedVecProvider { + model_name: NS733B_MODEL_A.to_string(), + map: ns733b_fixed_vectors(), + }); + rt.register_embedder(FixedVecProvider { + model_name: NS733B_MODEL_B.to_string(), + map: ns733b_fixed_vectors(), + }); + + let mut builder = VerbRegistryBuilder::new(); + builder.register(KgPack::new(rt.clone())); + builder.register(MemoryPack::new(rt.clone())); + let registry = builder.build().expect("registry"); + + // No `embedding_model` on remember: auto-detect fans out to every + // registered model (both A and B), matching + // `ns733_seed_three_memories`'s documented gotcha above. + let mut local_filler_ids: HashSet = HashSet::new(); + for i in 0..NS733B_FILLER_COUNT { + let r = registry + .dispatch( + "memory.remember", + serde_json::json!({ + "content": format!("ns733b breakdown local filler {i}"), + "memory_type": "semantic", + "namespace": "local", + }), + ) + .await + .expect("remember filler"); + local_filler_ids.insert( + r["id"] + .as_str() + .expect("id") + .parse::() + .expect("valid uuid"), + ); + } + let target_id = registry + .dispatch( + "memory.remember", + serde_json::json!({ + "content": NS733B_TARGET_CONTENT, + "memory_type": "semantic", + "namespace": "bench-a", + }), + ) + .await + .expect("remember target")["id"] .as_str() - .and_then(|s| s.parse::().ok()), - Some(target_id), - "the single hit must be the bench-a target, not a local filler" - ); - - // Case 2: widening disabled (`ann_overfetch_max_rounds: 1`) — round 1's - // narrow window is exhausted entirely by `local` fillers ranked ahead - // of the target, so the namespace-scoped post-filter finds nothing. - let mut disabled_params = base_params; - disabled_params["config"]["ann_overfetch_max_rounds"] = serde_json::json!(1); - let disabled_result = registry - .dispatch("memory.recall", disabled_params) - .await - .expect("memory.recall with widening disabled"); - let disabled_hits = disabled_result.as_array().expect("bare array result"); + .expect("id") + .parse::() + .expect("valid uuid"); + + let recall_args = serde_json::json!({ + "query": NS733B_QUERY, + "namespace": "bench-a", + "fusion_strategy": "vector_only", + "include_breakdown": true, + "limit": 10, + }); + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(500); + let settled_result: Value; + loop { + let result = registry + .dispatch("memory.recall", recall_args.clone()) + .await + .expect("memory.recall with namespace + multi-model + include_breakdown"); + let settled = result["candidates"]["vector_candidates_per_model"] + .as_array() + .is_some_and(|per_model| { + per_model.len() == 2 + && per_model.iter().any(|entry| { + entry["hits"].as_array().is_some_and(|hits| { + hits.iter().any(|hit| { + hit["id"].as_str().and_then(|s| s.parse::().ok()) + == Some(target_id) + }) + }) + }) + }); + if settled { + settled_result = result; + break; + } + if std::time::Instant::now() >= deadline { + last_failure = Some(format!( + "attempt {attempt}/{MAX_ATTEMPTS}: ANN warm for the bench-a target \ + did not settle within 500ms; last response: {result}" + )); + continue 'attempt; + } + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + + return ns733b_assert_breakdown(&settled_result, &local_filler_ids, target_id); + } + + panic!( + "ns733b: corpus/ANN-warm race did not settle in {MAX_ATTEMPTS} fresh attempts; \ + last failure: {}", + last_failure.unwrap_or_default() + ); + } + + /// Assertion body for + /// `ns733b_recall_verbose_multi_model_breakdown_excludes_off_namespace_candidates`, + /// split out so the retry loop above can call it once a settled response + /// is in hand without duplicating the assertions per attempt. + fn ns733b_assert_breakdown(result: &Value, local_filler_ids: &HashSet, target_id: Uuid) { + let per_model = result["candidates"]["vector_candidates_per_model"] + .as_array() + .expect("multi-model breakdown present (two models registered)"); + assert_eq!( + per_model.len(), + 2, + "both registered models must appear in the breakdown: {per_model:?}" + ); + + for model_entry in per_model { + let hits = model_entry["hits"].as_array().expect("hits array"); + for hit in hits { + let id = hit["id"] + .as_str() + .expect("id") + .parse::() + .expect("valid uuid"); + assert!( + !local_filler_ids.contains(&id), + "namespace=\"bench-a\" breakdown must not leak a local filler \ + UUID ({id}) for model {:?}: {model_entry:?}", + model_entry["model"] + ); + } + } + + // Sanity: the fix must not have filtered away everything — the + // bench-a target itself is entitled to appear (proves this is a + // real filter, not a filter-everything regression). + let any_model_has_target = per_model.iter().any(|entry| { + entry["hits"].as_array().unwrap().iter().any(|hit| { + hit["id"].as_str().and_then(|s| s.parse::().ok()) == Some(target_id) + }) + }); assert!( - disabled_hits.is_empty(), - "with widening disabled, round 1's over-fetch window is saturated by \ - closer local fillers and must not reach the bench-a target: {disabled_hits:?}" + any_model_has_target, + "the bench-a target must still appear in at least one model's \ + breakdown after the namespace filter: {per_model:?}" ); } } diff --git a/crates/khive-pack-memory/src/handlers/sub_handlers.rs b/crates/khive-pack-memory/src/handlers/sub_handlers.rs index c5ba01a0..d204d939 100644 --- a/crates/khive-pack-memory/src/handlers/sub_handlers.rs +++ b/crates/khive-pack-memory/src/handlers/sub_handlers.rs @@ -166,12 +166,20 @@ impl MemoryPack { }); if candidates.vector_hits_per_model.len() > 1 { + // Review finding (#733 fix-round 1, High): same fix as + // `handle_recall`'s verbose multi-model breakdown — the global + // per-model ANN candidate lists are pre-hydration and must be + // filtered through `memory_ids` (the visible-namespace + // post-filter already applied to `vector_candidates` above) + // before serialization, or this diagnostic view can leak + // off-namespace candidate UUIDs. let per_model: serde_json::Map = candidates .vector_hits_per_model .iter() .map(|(model, hits)| { let hits_json: Vec = hits .iter() + .filter(|h| memory_ids.contains(&h.subject_id)) .map(|h| { json!({ "id": h.subject_id.to_string(), diff --git a/crates/khive-runtime/src/pack.rs b/crates/khive-runtime/src/pack.rs index b0fce32e..b8888641 100644 --- a/crates/khive-runtime/src/pack.rs +++ b/crates/khive-runtime/src/pack.rs @@ -2041,7 +2041,7 @@ pub fn resolve_explicit_namespace( None => Namespace::parse(default_namespace) .map_err(|e| RuntimeError::InvalidInput(format!("invalid namespace: {e}"))), Some(Value::String(ns_str)) => Namespace::parse(ns_str) - .map_err(|e| RuntimeError::InvalidInput(format!("invalid namespace: {e}"))), + .map_err(|e| RuntimeError::InvalidInput(format!("invalid namespace {ns_str:?}: {e}"))), Some(other) => Err(RuntimeError::InvalidInput(format!( "invalid namespace: expected string when present, got {}", json_type_name(other), diff --git a/docs/benchmarks/recall-arm-isolation.md b/docs/benchmarks/recall-arm-isolation.md index 77cbab68..5638dbf7 100644 --- a/docs/benchmarks/recall-arm-isolation.md +++ b/docs/benchmarks/recall-arm-isolation.md @@ -11,9 +11,11 @@ the serving profile so scoring weights don't drift mid-measurement. ## Recipe -1. **Pick an arm namespace.** Any string that parses as a valid namespace - (alphanumeric segments, `.` or `::` separators, no spaces) works. Prefix it - so it is obviously scratch data, e.g. `bench-arm-a`. +1. **Pick an arm namespace.** A valid namespace is one or more segments + separated by a single `:`, each segment matching `[a-zA-Z0-9\-_.]+` (no + spaces, no empty segments, no trailing `:`; `::` is rejected — it produces + an empty segment between the two colons). Prefix it so it is obviously + scratch data, e.g. `bench-arm-a`. 2. **Write the arm's corpus** with an explicit namespace override: @@ -45,8 +47,9 @@ the serving profile so scoring weights don't drift mid-measurement. `local`. An invalid namespace string is a hard per-op error, not a silent fallback. -5. **Tear down** by deleting the arm's memories (`delete(type="memory", - id=..., hard=true)`) once the measurement is done, or simply let the arm +5. **Tear down** by deleting the arm's memories (`delete(id="...", + hard=true)` — one call per memory id; `delete` takes `id`/`kind`/`hard`, + not a `type=` param) once the measurement is done, or simply let the arm namespace age out unread — it costs nothing beyond the storage of its own rows. @@ -59,10 +62,15 @@ the serving profile so scoring weights don't drift mid-measurement. is no namespace-scoped posterior isolation. Use a freshly created, arm-specific profile (step 3) to avoid contaminating a shared profile's state. Tracked as issue #733 remainder. -- **`knowledge.compose`.** The knowledge/lore corpus has no namespace filter - at all — composed knowledge is global regardless of which namespace the - caller's token carries. A measurement arm that includes compose calls is - not isolated from concurrent compose traffic elsewhere. +- **`knowledge.compose`.** Compose does scope section loading to the caller's + token namespace (`WHERE namespace = ?1`) — it is not global. What it lacks + is _this feature_: there is no `namespace=` exact-match parameter on + compose analogous to the one this slice adds to `memory.recall`, and no + `profile_id`-style pinning either. A measurement arm cannot point compose + at a specific scratch namespace independent of the caller's token the way + it can for recall; compose calls in an arm are scoped by whatever namespace + the caller is already authorized for, not covered by recall's new + exact-match parameter. - **The serve ledger.** `brain.record_serve` stamps the _effective_ namespace used for the fetch (the arm namespace when `namespace=` was passed), so ledger rows are attributable to the arm — but the ledger itself is a single From 8d422869f5410041caf9e228269a56def3fb30ab Mon Sep 17 00:00:00 2001 From: oceanwaves630 <33291608+oceanwaves630@users.noreply.github.com> Date: Thu, 9 Jul 2026 00:43:21 -0400 Subject: [PATCH 3/3] test(memory): cover memory.recall_candidates in the #733 namespace-leak regression Codex re-review (round 2, REJECT/Medium): the fix-round-1 regression dispatched only memory.recall, which is mutation-sensitive for handle_recall's per-model namespace filter but not for handle_recall_candidates's independent filter (sub_handlers.rs:182) -- pack.rs routes the two verbs to two separate handler functions with two separate vector_hits_per_model serialization sites. Reverting the sub_handlers.rs filter alone would not have failed any existing test. Adds ns733b_recall_candidates_multi_model_excludes_off_namespace_candidates, dispatching memory.recall_candidates directly against the same two-model / local-fillers / bench-a-target corpus, asserting no leaked local filler UUID and the target present in handle_recall_candidates's object-keyed vector_candidates_per_model shape (distinct from handle_recall's array-of-entries shape). Verified non-vacuous by temporarily reverting the sub_handlers.rs filter and confirming the test fails, listing the leaked UUIDs, then restoring. Extracted the shared corpus-seeding loop (5 local fillers + 1 bench-a target across two custom embedding models) into ns733b_seed_two_model_corpus, reused by both the existing memory.recall regression and the new memory.recall_candidates one. Applies the same bounded fresh-runtime retry as fix round 1 for the pre-existing, unrelated ANN-warm-cache race documented there (handle_recall_candidates reads the same shared per-model ANN index via the same collect_recall_candidates path). Co-Authored-By: Claude Fable 5 --- .../khive-pack-memory/src/handlers/recall.rs | 240 +++++++++++++++--- 1 file changed, 201 insertions(+), 39 deletions(-) diff --git a/crates/khive-pack-memory/src/handlers/recall.rs b/crates/khive-pack-memory/src/handlers/recall.rs index d2575469..3bdb9078 100644 --- a/crates/khive-pack-memory/src/handlers/recall.rs +++ b/crates/khive-pack-memory/src/handlers/recall.rs @@ -3726,6 +3726,59 @@ mod tests { m } + /// Seeds `registry`'s runtime with `NS733B_FILLER_COUNT` `local` filler + /// memories plus one `bench-a` target memory, all sharing + /// `ns733b_fixed_vectors`'s vector scheme (query cos 1.0, fillers cos + /// 0.9, target cos 0.5). No `embedding_model` on remember: auto-detect + /// fans out to every registered model, matching + /// `ns733_seed_three_memories`'s documented gotcha above. Shared between + /// the `memory.recall` verbose-breakdown regression (#733 fix-round 1, + /// High) and the `memory.recall_candidates` regression (#733 fix-round + /// 2, Medium) that protects `handle_recall_candidates`'s independent + /// per-model serialization site (`sub_handlers.rs`). Returns + /// `(local_filler_ids, target_id)`. + async fn ns733b_seed_two_model_corpus( + registry: &khive_runtime::VerbRegistry, + ) -> (HashSet, Uuid) { + let mut local_filler_ids: HashSet = HashSet::new(); + for i in 0..NS733B_FILLER_COUNT { + let r = registry + .dispatch( + "memory.remember", + serde_json::json!({ + "content": format!("ns733b breakdown local filler {i}"), + "memory_type": "semantic", + "namespace": "local", + }), + ) + .await + .expect("remember filler"); + local_filler_ids.insert( + r["id"] + .as_str() + .expect("id") + .parse::() + .expect("valid uuid"), + ); + } + let target_id = registry + .dispatch( + "memory.remember", + serde_json::json!({ + "content": NS733B_TARGET_CONTENT, + "memory_type": "semantic", + "namespace": "bench-a", + }), + ) + .await + .expect("remember target")["id"] + .as_str() + .expect("id") + .parse::() + .expect("valid uuid"); + (local_filler_ids, target_id) + } + /// Codex review finding (#733 fix-round 1, High): with `namespace="bench-a"`, /// more than one registered embedding model, and `include_breakdown=true`, /// `memory.recall`'s verbose response embeds @@ -3786,45 +3839,7 @@ mod tests { builder.register(MemoryPack::new(rt.clone())); let registry = builder.build().expect("registry"); - // No `embedding_model` on remember: auto-detect fans out to every - // registered model (both A and B), matching - // `ns733_seed_three_memories`'s documented gotcha above. - let mut local_filler_ids: HashSet = HashSet::new(); - for i in 0..NS733B_FILLER_COUNT { - let r = registry - .dispatch( - "memory.remember", - serde_json::json!({ - "content": format!("ns733b breakdown local filler {i}"), - "memory_type": "semantic", - "namespace": "local", - }), - ) - .await - .expect("remember filler"); - local_filler_ids.insert( - r["id"] - .as_str() - .expect("id") - .parse::() - .expect("valid uuid"), - ); - } - let target_id = registry - .dispatch( - "memory.remember", - serde_json::json!({ - "content": NS733B_TARGET_CONTENT, - "memory_type": "semantic", - "namespace": "bench-a", - }), - ) - .await - .expect("remember target")["id"] - .as_str() - .expect("id") - .parse::() - .expect("valid uuid"); + let (local_filler_ids, target_id) = ns733b_seed_two_model_corpus(®istry).await; let recall_args = serde_json::json!({ "query": NS733B_QUERY, @@ -3922,4 +3937,151 @@ mod tests { breakdown after the namespace filter: {per_model:?}" ); } + + // ── #733 fix-round 2 (codex Medium): `memory.recall_candidates` must be + // covered by its own regression, independent of `memory.recall`'s ────── + + /// Codex re-review finding (#733 fix-round 2, Medium): the fix-round-1 + /// regression above dispatches only `memory.recall` with + /// `include_breakdown=true`, which is mutation-sensitive for + /// `handle_recall`'s filter (`recall.rs`) but *not* for + /// `handle_recall_candidates`'s independent filter (`sub_handlers.rs:182`, + /// reached via the separate `memory.recall_candidates` verb — + /// `pack.rs`'s dispatch table routes the two verbs to two different + /// handler functions with two separate `vector_hits_per_model` + /// serialization sites). Removing the `.filter(...)` at + /// `sub_handlers.rs:182` would not fail any existing test. This + /// dispatches `memory.recall_candidates` directly against the identical + /// two-model/`local`-fillers/`bench-a`-target corpus and asserts the + /// same no-leak + target-present properties against + /// `handle_recall_candidates`'s response shape, which differs from + /// `handle_recall`'s: `vector_candidates_per_model` here is a JSON + /// *object* keyed by model name (`{"model-a": [...], "model-b": [...]}`, + /// `sub_handlers.rs:194`), not an array of `{model, hits}` entries. + #[tokio::test] + #[serial(background_tasks)] + async fn ns733b_recall_candidates_multi_model_excludes_off_namespace_candidates() { + // Same pre-existing ANN-warm race documented in detail on + // `ns733b_recall_verbose_multi_model_breakdown_excludes_off_namespace_candidates` + // above applies identically here — `handle_recall_candidates` reads + // the same shared per-model ANN index via the same + // `collect_recall_candidates` path `handle_recall` uses. Same bounded + // fresh-runtime retry. + const MAX_ATTEMPTS: usize = 5; + let mut last_failure: Option = None; + + 'attempt: for attempt in 1..=MAX_ATTEMPTS { + let rt = KhiveRuntime::memory().expect("in-memory runtime"); + rt.register_embedder(FixedVecProvider { + model_name: NS733B_MODEL_A.to_string(), + map: ns733b_fixed_vectors(), + }); + rt.register_embedder(FixedVecProvider { + model_name: NS733B_MODEL_B.to_string(), + map: ns733b_fixed_vectors(), + }); + + let mut builder = VerbRegistryBuilder::new(); + builder.register(KgPack::new(rt.clone())); + builder.register(MemoryPack::new(rt.clone())); + let registry = builder.build().expect("registry"); + + let (local_filler_ids, target_id) = ns733b_seed_two_model_corpus(®istry).await; + + // `memory.recall_candidates`'s `HandlerDef` declares no + // `namespace` `ParamDef` (`pack.rs`: `params: &[]`), so + // `VerbRegistry::dispatch`'s generic Rule-3 explicit-namespace + // escape (ADR-007 Rev 4/6) is the *only* thing scoping this call + // — it mints the token with `visible=["bench-a"]` before + // `handle_recall_candidates` ever runs, then strips the + // `namespace` key from params (the handler never sees it). No + // `fusion_strategy`/`include_breakdown` params exist on this + // sub-verb — it always returns the per-model breakdown whenever + // more than one model is registered (`sub_handlers.rs:168`). + let recall_candidates_args = serde_json::json!({ + "query": NS733B_QUERY, + "namespace": "bench-a", + "limit": 10, + }); + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(500); + let settled_result: Value; + loop { + let result = registry + .dispatch("memory.recall_candidates", recall_candidates_args.clone()) + .await + .expect("memory.recall_candidates with namespace + multi-model"); + let settled = result["vector_candidates_per_model"] + .as_object() + .is_some_and(|per_model| { + per_model.len() == 2 + && per_model.values().any(|hits| { + hits.as_array().is_some_and(|hits| { + hits.iter().any(|hit| { + hit["id"].as_str().and_then(|s| s.parse::().ok()) + == Some(target_id) + }) + }) + }) + }); + if settled { + settled_result = result; + break; + } + if std::time::Instant::now() >= deadline { + last_failure = Some(format!( + "attempt {attempt}/{MAX_ATTEMPTS}: ANN warm for the bench-a target \ + did not settle within 500ms; last response: {result}" + )); + continue 'attempt; + } + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + + let per_model = settled_result["vector_candidates_per_model"] + .as_object() + .expect("multi-model breakdown present (two models registered)"); + assert_eq!( + per_model.len(), + 2, + "both registered models must appear in the breakdown: {per_model:?}" + ); + + for (model_name, hits) in per_model { + let hits = hits.as_array().expect("hits array"); + for hit in hits { + let id = hit["id"] + .as_str() + .expect("id") + .parse::() + .expect("valid uuid"); + assert!( + !local_filler_ids.contains(&id), + "namespace=\"bench-a\" recall_candidates breakdown must not leak a \ + local filler UUID ({id}) for model {model_name:?}: {hits:?}" + ); + } + } + + // Sanity: the fix must not have filtered away everything — the + // bench-a target itself is entitled to appear (proves this is a + // real filter, not a filter-everything regression). + let any_model_has_target = per_model.values().any(|hits| { + hits.as_array().unwrap().iter().any(|hit| { + hit["id"].as_str().and_then(|s| s.parse::().ok()) == Some(target_id) + }) + }); + assert!( + any_model_has_target, + "the bench-a target must still appear in at least one model's \ + recall_candidates breakdown after the namespace filter: {per_model:?}" + ); + return; + } + + panic!( + "ns733b recall_candidates: corpus/ANN-warm race did not settle in {MAX_ATTEMPTS} \ + fresh attempts; last failure: {}", + last_failure.unwrap_or_default() + ); + } }