fix(memory): audit sweep — 5 CRITICAL + 7 HIGH (split-brain, RBAC, decay, dedup, prompt budget, async consolidate)#5839
Merged
Merged
Conversation
…es, forget() leak, immortal decay 5 CRITICAL findings from a memory-system audit, all in the proactive memory layer: * C1 split-brain: list()/get() read from the KV mirror while search()/auto_retrieve read from the semantic store. The KV write was best-effort (warn-and-continue) so a failure left rows visible to search but invisible to list. retrieve_memory_items now reads from semantic (the authoritative source); KV writes stay as a non-load- bearing compatibility mirror. * C2 raw-transcript fallback: when the extractor returned no signal, add() stored the verbatim concatenated message content as a session-level memory with no category. This was the dominant source of `category=null` rows and duplicate transcripts on the dashboard. The fallback is removed; callers that want raw content use add_with_level explicitly. * C3 confidence hardcoded to 1.0 + immortal decay: remember_with_embedding_and_peer now honors metadata["confidence"] (clamped to 0..=1, defaulting to 1.0) so the LLM extractor's signal reaches the column. decay_confidence reworked: boost divides the rate instead of multiplying the result and clamping to 1.0. The old formula made any memory with >=2 accesses freeze at confidence 1.0 forever; the new formula keeps "popular memories decay slower" but strictly monotonic (boost capped at MAX_BOOST=4.0). * C4 RBAC on write endpoints: memory_add, memory_update, memory_delete, memory_bulk_delete, memory_reset_agent, memory_clear_level, memory_consolidate, memory_cleanup, memory_export_agent, memory_import_agent, memory_decay, memory_store_relations now route through the namespace guard. New ProactiveMemoryStore wrappers cover the previously-unguarded ops (reset, clear_level, export_all, import_memories, decay_confidence). The root api_key is attributed as an Owner- equivalent AuthenticatedApiUser in middleware so operators using only the master credential keep their POST/PUT/DELETE access. * C5 forget() never marked deleted_at: SemanticStore::forget* now stamps deleted_at alongside deleted=1 so the prune_soft_deleted_memories sweep (filter `deleted_at IS NOT NULL`) can actually hard-delete user-/API-initiated deletions. Without the stamp every soft-deleted row leaked its embedding BLOB forever. consolidation.rs's merge-loser delete now stamps it too. Drive-by: removed a clippy::manual_option_zip in kernel/background_lifecycle.rs flagged while clippy-gating the change. Verification: * cargo check --workspace --lib — clean * cargo clippy -p librefang-memory -p librefang-api --tests -- -D warnings — clean * cargo test -p librefang-memory --lib — 269 passed (5 pre-existing tests adapted to the new add()-no-fallback semantics; new regression tests for C1 list-from-semantic, C2 no-fallback, C3 monotonic-decay + extractor-confidence-roundtrip, C4 viewer-denied on every write wrapper, C5 forget* stamps deleted_at) * cargo test -p librefang-api --lib --test memory_routes_integration --test agent_kv_authz_integration --test auth_public_allowlist — all green
…rompt budget, detach auto-consolidate, unify consolidation knob Follow-up sweep on the same memory-system audit as the prior commit — picks off the HIGH findings that were in scope for the same crate / file cluster. * H1 duplicate_threshold tightened. The configured default jumps from 0.5 → 0.85 (mem0's recommended near-duplicate cut-off); both metrics — cosine and Jaccard — agree that 0.5 means "topically related", which let opposite-meaning sentences sharing keywords silently merge. `DefaultMemoryExtractor::decide_action`'s hardcoded same-category 0.5 / cross-category 0.6 UPDATE thresholds rise to 0.7 / 0.8; the 0.95 NOOP gate stays. `import_memories`' hardcoded 0.9 dedup floor rises to 0.95 with a docstring explaining why bulk-import is stricter than extraction-time dedup. * H2 LLM-extraction validation. `parse_llm_extraction_response` now enforces a 4-char content floor (drops "ok" / "no" / single-letter junk that was trivially unique and survived dedup), validates the emitted `category` against the configured `extract_categories` allowlist (out-of-allowlist values downgrade to "general" instead of polluting the dashboard's facets), and caps a single extraction call at MAX_MEMORIES_PER_EXTRACTION=20 rows so a runaway model can't churn the eviction loop. * H4 prompt-injection budget. `format_context` now goes through a shared `format_memories_with_budget` helper capped at FORMAT_CONTEXT_MAX_CHARS=8000 (~2000 tokens). Pre-fix the formatter concatenated everything with no ceiling — 10 retrieved memories × 2000-char MAX_MEMORY_CONTENT_LENGTH could push 20 KB into every request. Excess rows are reported via a "[+N additional memories omitted to keep the prompt within budget]" footer so the truncation is observable in the rendered prompt rather than silent. * H6 auto-consolidation no longer blocks the agent. The every-10 trigger in `auto_memorize` now `tokio::spawn`s the consolidate call instead of awaiting it inline; the next agent turn doesn't pay for the O(n²) merge pass plus its SQLite transaction. The detached future borrows nothing from `self` thanks to `ProactiveMemoryStore`'s manual Clone over Arc'd inner state. * H5 single source of truth for the consolidation threshold. `ConsolidationEngine` gains a `duplicate_threshold` field (defaulting to 0.85 to match the new config default) with a `set_duplicate_threshold` setter; `MemorySubstrate` exposes a passthrough; kernel boot pushes `config.proactive_memory. duplicate_threshold` down to the engine. The periodic global consolidation sweep and the on-demand `ProactiveMemoryStore::consolidate` now agree on what counts as a near-duplicate. Keeps the existing 142 callers of `MemorySubstrate::open_in_memory(decay_rate)` source-compatible (no signature break). H7 / H8 from the same audit were already addressed by the C3 fix in the previous commit (popular-memory immortality + dead extraction_threshold). H3 (memory_store/recall vs auto_memorize/ retrieve being disconnected tool surfaces) is intentional product shape rather than a bug — left out. Verification: * cargo check --workspace --lib — clean * cargo clippy -p librefang-memory -p librefang-runtime -p librefang-types -p librefang-api --tests -- -D warnings — clean * cargo test -p librefang-memory --lib — 269 passed * cargo test -p librefang-runtime --lib (proactive_memory) — 57 passed, including the 5 new regressions (parse_extraction_drops_sub_minimum_content, parse_extraction_downgrades_unknown_category, parse_extraction_preserves_category_when_allowlist_empty, parse_extraction_caps_total_memories_per_call, format_context_caps_prompt_budget_with_truncation_marker). * cargo test -p librefang-api --test memory_routes_integration --test agent_kv_authz_integration --test auth_public_allowlist — all green.
…eshold, LLM confidence, KV mirror retirement, instrumented spawn, fuzzy categories, configurable prompt cap, CHANGELOG, comment fix 10 follow-ups raised on the code review of the prior two commits in this PR. All within the same memory-system scope. * #1 root user_id is now a constant sentinel UUID (00000000-0000-0000-0000-72006f0074a0, exported as `ROOT_API_KEY_USER_ID`) rather than `UserId::from_name("root")`. The from_name UUIDv5 lives inside `LIBREFANG_USER_NAMESPACE`, so an operator-registered `[users] name = "root"` would have silently inherited the master credential's ACL + per-user budget cap. The sentinel falls outside that namespace; AuthManager returns None for it and the fail-open Owner-default ACL applies. Regression test `root_api_key_user_id_does_not_collide_with_any_named_user` pins the non-collision invariant against {root, admin, owner, system, operator, user}. * #3 `HotAction::UpdateProactiveMemory` now also calls `substrate.set_consolidation_duplicate_threshold(...)`, so when `POST /api/config/reload` swaps in a new `[proactive_memory] duplicate_threshold`, the periodic global consolidation sweep picks it up alongside the per-agent on-demand consolidate. Without this the per-agent path picked up the new value but the global sweep stayed on the old one — exactly the inconsistency H5 set out to remove. `ConsolidationEngine` switched to an `Arc<AtomicU32>` threshold (f32 bits) so the setter takes `&self`, which is required because the hot-reload code path holds only `Arc<MemorySubstrate>`. `docs/operations/config-reload.md` row updated to call out the new behaviour. * #4 `build_extraction_prompt` now asks the LLM to emit a per-memory `confidence` field with a brief calibration guide; the parser reads it, clamps to [0, 1], and stashes the value in `metadata["confidence"]` and `MemoryItem.confidence` so the C3 insert path actually lands a non-default value in the `confidence` column. Missing field still defaults to 1.0 (matches the rule-based extractor's prior behaviour — never silently drops a memory). Tests `parse_extraction_propagates_confidence_to_metadata` and `parse_extraction_clamps_confidence_to_unit_interval` pin the new behaviour. * #5 The KV `memory:*` mirror is gone — fully retired, not just "non-load-bearing". All `structured.set("memory:*", ...)` / `structured.delete("memory:*", ...)` / `list_kv` scans that walked the mirror have been deleted from `import_memories`, `add_with_decision`'s ADD + UPDATE branches, `add_with_level`, `delete`, `update`, `reset`, `clear_level`, `cleanup_expired_sessions`, the eviction loop, and the consolidation merge-loser path. The read path was already on semantic (C1); leaving the writes in place would have grown the mirror without bound and risked future divergence regressions. `test_delete_memory` rewritten to assert behaviour through `search()` (the trait-level contract) instead of probing the underlying KV store. Any legacy `memory:*` entries from older installs are silently ignored. * #6 The detached auto-consolidate `tokio::spawn` is wrapped in a `tracing::info_span!("auto_consolidate", task = "auto_consolidate", agent = ...)` via `.instrument(span)`, so a panic inside the consolidate future surfaces in tracing output (instead of disappearing silently the way bare-spawn panics do) and operators can grep `task = "auto_consolidate"` to find the detached work. * #7 Category allowlist match is now case-insensitive + tolerant of trailing `s` on either side. `"Preferences"` / `"PREFERENCE"` / `"preferences"` all snap to a configured `"preference"` and the canonical configured spelling lands in the column. Test `parse_extraction_fuzzy_matches_category_case_and_plural` pins the four variants. * #8 `format_context_max_chars` is now a field on `ProactiveMemoryConfig` (default 8000 chars / ~2000 tokens); the store's `format_context_with_query` / `format_context` read it from the live config and pass it to `format_memories_with_budget(memories, max_chars)`. The trait fallback (`DefaultMemoryExtractor::format_context`, `LlmMemoryExtractor::format_context`) keeps the const default for callers without config access. Operators on 200k+ context windows can now raise the cap via `config.toml` without recompiling. * #2 + #9 New `Fixed` entry in `[Unreleased]` documents the breaking audit-shape change (root-api_key requests now stamp a `user_id` where they previously stamped `None`) and the `add()` behaviour change (no raw-transcript fallback for extraction misses). Both were noted inline in commits but absent from the CHANGELOG. The entry also lists the full audit-sweep scope so an operator can size-up the upgrade impact from one place. * #10 `import_memories` comment rewritten to explain the 0.95 threshold without the confusing "stricter than extraction-time dedup" framing. Drive-by: collapsed three more `clippy::manual_option_zip` sites in `kernel/tests.rs` that the workspace-clippy gate would have caught on the next CI run. Auto-restamped `.secrets.baseline` line numbers shifted by the CHANGELOG insert. Verification: * cargo check --workspace --lib — clean * cargo clippy -p librefang-memory -p librefang-runtime -p librefang-types -p librefang-api -p librefang-kernel --tests -- -D warnings — clean * cargo test -p librefang-memory --lib — 269 passed * cargo test -p librefang-runtime --lib proactive_memory — 60 passed (5 new follow-up regression tests included) * cargo test -p librefang-api --lib --test memory_routes_integration --test agent_kv_authz_integration --test auth_public_allowlist — all green
added 3 commits
May 29, 2026 09:00
… 403 The RBAC gating added 12 memory-write ACL checks, but the default `librefang start` (no api_key, loopback bind) takes the no-auth bypass which returned next.run() WITHOUT attaching an AuthenticatedApiUser. Memory write handlers then saw None -> anonymous Viewer fallback -> 403 on every POST/PUT/DELETE /api/memory*, breaking the documented default workflow. No-auth + trusted origin (loopback / LIBREFANG_ALLOW_NO_AUTH) is the same trust level as the root master credential, so attribute the same Owner-equivalent user (ROOT_API_KEY_USER_ID). Non-loopback still fails closed. Add integration tests for both: loopback write != 403, non-loopback no-auth still 401.
MEDIUM (#5839): list/get/export/list_all read paths called recall(), which unconditionally bumps access_count + accessed_at. A dashboard polling the memory list would perpetually reset accessed_at = now and inflate access_count — the exact signals the C3 decay logic keys idle/popularity off — so polled listings could keep memories from ever decaying (and turned a GET into a 10k-row write). Add recall_readonly() (shared impl, no bump) and route the four listing/export reads through it; genuine semantic recalls still track access. Regression test asserts recall_readonly leaves access_count untouched while recall() bumps by 1. Also correct the decay_confidence doc: the once-per-hour cadence is enforced by the periodic maintenance scheduler, not an internal throttle (a direct call decays immediately).
…undle # Conflicts: # .secrets.baseline
houko
added a commit
that referenced
this pull request
May 29, 2026
#5853) rustfmt flags several blocks merged via #5835, #5839, #5848, and #5849 (method chains and multi-line statements rustfmt collapses/reflows). main was therefore red on the Quality "Check formatting" step, which fails the Quality job on every open PR. Pure `cargo fmt` output — no logic change. Co-authored-by: Evan <[email protected]>
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
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.
Memory-system audit fixes — bundled because every finding lives in the same crate cluster (
librefang-memory, thelibrefang-runtimeproactive-memory extractor, andlibrefang-api's memory route surface).Coverage map
list/getnow read from semantic, not the best-effort KV mirrorconfidencehonored frommetadata["confidence"]on insert; decay reworked (rate / boost) so popular memories decay slower but strictly monotonically — also retires the H7 / H8 findings (immortal-boost + deadextraction_threshold)api_keyattributed as Owner so operators don't lose write accessforget*stampsdeleted_at; the retention sweep can finally hard-delete user-initiated soft-deletesduplicate_thresholddefault 0.5 → 0.85;decide_actionhardcoded UPDATE thresholds 0.5 / 0.6 → 0.7 / 0.8;import_memories0.9 → 0.95categoryagainstextract_categories, caps at 20 memories / callmemory_store/recall(KV exact-match) vsauto_memorize/retrieve(semantic) are intentionally distinct tool surfaces, not a bugformat_contextcapped atFORMAT_CONTEXT_MAX_CHARS = 8000(~2000 tokens), with a[+N omitted]footerConsolidationEngineexposesset_duplicate_threshold; kernel boot pushesconfig.proactive_memory.duplicate_thresholdso both consolidation paths agreetokio::spawned; no longer blocks the agent's hot path on O(n²) merge + SQLite txmetadata["confidence"]now reaches the column)Findings in MEDIUM and LOW tiers aren't included — the original audit truncated mid-H8 in the report I was given and the user's follow-up request asked for the next visible severity tier.
Headline behavioural changes (reviewer eyes here)
POST /api/memorywith content that the extractor can't parse no longer creates a row. Pre-fix it silently captured the whole transcript.api_keyis attributed as Owner so the obviouslibrefang start→curl POSTworkflow keeps working; per-user keys honour their configuredmemory_accessblock.auto_retrieveinjects at most ~8 KB into the prompt. Operators on very long context windows who want more memory context can raiseFORMAT_CONTEXT_MAX_CHARS(currently hard-coded; promote to config if the demand shows up).decide_actionUPDATE thresholds are stricter (0.7 same-category / 0.8 cross-category). A weak cosine match no longer silently replaces an existing memory; net effect is more rows kept, less aggressive merging.decay_confidencewill actually shrink confidence for popular memories over long enough idle windows. Tunedecay_rate(config.toml) orMAX_BOOST(crates/librefang-memory/src/proactive.rs) if the new rate is too aggressive.tokio::spawn— the agent's next turn no longer waits for the dedup pass. Log line stays the same.Drive-bys
clippy::manual_option_zipinkernel/background_lifecycle.rs:88was pre-existing and failed the workspace clippy gate. One-line collapse to.zip(..). Noticed while running clippy on the C4 changes; in-scope per CLAUDE.md's "fix what you found".Verification
cargo check --workspace --lib— cleancargo clippy -p librefang-memory -p librefang-runtime -p librefang-types -p librefang-api --tests -- -D warnings— cleancargo test -p librefang-memory --lib— 269 passedcargo test -p librefang-runtime --lib(proactive_memory subset) — 57 passedcargo test -p librefang-api --lib --test memory_routes_integration --test agent_kv_authz_integration --test auth_public_allowlist— all greenNew regression tests pinning each fix:
proactive::tests::list_reflects_semantic_store_not_kv_mirrorproactive::tests::add_does_not_store_raw_transcript_fallbackproactive::tests::popular_memory_decay_is_monotonic_not_frozen+insert_honors_extractor_supplied_confidenceproactive::tests::write_guarded_wrappers_reject_viewer_acldecay::tests::forget_variants_stamp_deleted_at_so_prune_sees_themproactive_memory::tests::{parse_extraction_drops_sub_minimum_content, parse_extraction_downgrades_unknown_category, parse_extraction_preserves_category_when_allowlist_empty, parse_extraction_caps_total_memories_per_call}proactive_memory::tests::format_context_caps_prompt_budget_with_truncation_markerEOF