feat(memory): optional exact-match namespace param on recall + bench arm-isolation recipe (#733)#751
Merged
Merged
Conversation
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 <noreply@anthropic.com>
…aram 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 e57cd98) 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 <noreply@anthropic.com>
…ak 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 <noreply@anthropic.com>
ohdearquant
approved these changes
Jul 9, 2026
ohdearquant
left a comment
ohdearquant
left a comment
Owner
There was a problem hiding this comment.
Verified at content level: the effective-token shadowing flows the namespace override uniformly through FTS/vector fetch, ANN over-fetch gate, supersedes read, and serve-ledger stamp; absent param stays byte-identical per ADR-007 Rev 6's escape-hatch model with dispatch Rule-3 doing the primary enforcement and this as defense-in-depth (mirroring handle_remember). The High fix is correct at the root: the ANN index is namespace-global, so the verbose diagnostic had to filter through the same memory_ids visible-namespace set as results — and codex's r2 REJECT for guarding only one site, answered with a mutation-verified second regression, is the review lane doing exactly its job. #750 (or_insert mid-write snapshot race) noted as tonight's fifth defect — good that it's filed as production-correctness, not test-flake. Approved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements #733 slice 1 (the read-side escape hatch;
brain.auto_feedbackevent scoping remains open on the issue).What
memory.recallgains an optional exact-matchnamespaceparam. Absent → byte-identical to prior behavior (token namespace, defaultlocal, per ADR-007 Rev 6's multi-record-op model). Present → the entire read pipeline (FTS + vector candidate fetch, the ANN over-fetch retry loop's visibility gate, the supersedes graph read, and the serve-ledger namespace stamp) scopes to exactly that namespace.handle_remember: dispatch's ADR-007 Rule-3 explicit-namespace escape already pre-mints the token; the handler re-derives for direct (non-dispatch) callers. ThenamespaceParamDef declaration keeps dispatch's strip-vs-pass-through behavior consistent.vector_hits_per_model/vector_candidates_per_modelare filtered through the visible-namespace id set at both serialization sites (memory.recall include_breakdownandmemory.recall_candidates), so off-namespace candidate UUIDs cannot leak through the breakdown paths.resolve_explicit_namespaceerrors now name the supplied invalid value.docs/benchmarks/recall-arm-isolation.md— how to run isolated measurement arms (per-arm namespace writes + reads, profile pinning via the serving-profile override), with explicit notes on what is not yet isolated.Tests
12 new tests: absent-param regression (exact id-set equality against a mixed-namespace corpus), explicit-namespace scoping, no-match-empty-ok, invalid-value error naming the literal input, ANN over-fetch widening with a widening-disabled control, and mutation-verified breakdown-leak regressions for both diagnostic paths (two embedding models, closer-ranked off-namespace fillers).
Note: two tests use a bounded fresh-runtime retry to tolerate a pre-existing ANN warm-cache race, tracked separately as #750 — retries occur only before response settlement; assertions stay strict.
Review
Codex rounds: r1 APPROVE-WITH-FIXES (1 High, 2 Medium) → fixes applied → r2 REJECT (1 Medium: missing mutation coverage on the candidates path) → fix applied → r3 clean APPROVE.
Gates: fmt, clippy (
khive-pack-memory,khive-runtime) with-D warnings,cargo test -p khive-pack-memory(222 passed / 1 ignored),cargo test -p khive-runtime(59 passed) — all exit 0.Closes nothing (leaves #733 open for the feedback-scoping remainder).
🤖 Generated with Claude Code