Skip to content

fix(memory): audit sweep — 5 CRITICAL + 7 HIGH (split-brain, RBAC, decay, dedup, prompt budget, async consolidate)#5839

Merged
houko merged 8 commits into
mainfrom
fix/memory-critical-bundle
May 29, 2026
Merged

fix(memory): audit sweep — 5 CRITICAL + 7 HIGH (split-brain, RBAC, decay, dedup, prompt budget, async consolidate)#5839
houko merged 8 commits into
mainfrom
fix/memory-critical-bundle

Conversation

@houko

@houko houko commented May 28, 2026

Copy link
Copy Markdown
Contributor

Memory-system audit fixes — bundled because every finding lives in the same crate cluster (librefang-memory, the librefang-runtime proactive-memory extractor, and librefang-api's memory route surface).

Coverage map

ID Severity Status
C1 CRITICAL ✅ split-brain reads — list / get now read from semantic, not the best-effort KV mirror
C2 CRITICAL ✅ no more raw-transcript fallback when extraction yields nothing
C3 CRITICAL confidence honored from metadata["confidence"] on insert; decay reworked (rate / boost) so popular memories decay slower but strictly monotonically — also retires the H7 / H8 findings (immortal-boost + dead extraction_threshold)
C4 CRITICAL ✅ 12 write endpoints (add / update / delete / bulk-delete / reset / clear / consolidate / cleanup / export / import / decay / relations) gated through the namespace guard; root api_key attributed as Owner so operators don't lose write access
C5 CRITICAL ✅ every forget* stamps deleted_at; the retention sweep can finally hard-delete user-initiated soft-deletes
H1 HIGH duplicate_threshold default 0.5 → 0.85; decide_action hardcoded UPDATE thresholds 0.5 / 0.6 → 0.7 / 0.8; import_memories 0.9 → 0.95
H2 HIGH ✅ extraction now enforces a 4-char content floor, validates category against extract_categories, caps at 20 memories / call
H3 HIGH ⏸ skipped — memory_store/recall (KV exact-match) vs auto_memorize/retrieve (semantic) are intentionally distinct tool surfaces, not a bug
H4 HIGH format_context capped at FORMAT_CONTEXT_MAX_CHARS = 8000 (~2000 tokens), with a [+N omitted] footer
H5 HIGH ConsolidationEngine exposes set_duplicate_threshold; kernel boot pushes config.proactive_memory.duplicate_threshold so both consolidation paths agree
H6 HIGH ✅ auto-consolidation tokio::spawned; no longer blocks the agent's hot path on O(n²) merge + SQLite tx
H7 HIGH ✅ closed by C3 (boost capped via rate-division, can't saturate ≥ 2.0 anymore)
H8 HIGH ✅ closed by C3 (metadata["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)

  1. POST /api/memory with content that the extractor can't parse no longer creates a row. Pre-fix it silently captured the whole transcript.
  2. Memory write endpoints now require an authenticated caller whose ACL grants the corresponding capability. Root api_key is attributed as Owner so the obvious librefang startcurl POST workflow keeps working; per-user keys honour their configured memory_access block.
  3. auto_retrieve injects at most ~8 KB into the prompt. Operators on very long context windows who want more memory context can raise FORMAT_CONTEXT_MAX_CHARS (currently hard-coded; promote to config if the demand shows up).
  4. decide_action UPDATE 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.
  5. decay_confidence will actually shrink confidence for popular memories over long enough idle windows. Tune decay_rate (config.toml) or MAX_BOOST (crates/librefang-memory/src/proactive.rs) if the new rate is too aggressive.
  6. Auto-consolidation runs in a detached tokio::spawn — the agent's next turn no longer waits for the dedup pass. Log line stays the same.

Drive-bys

  • clippy::manual_option_zip in kernel/background_lifecycle.rs:88 was 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 — clean
  • cargo clippy -p librefang-memory -p librefang-runtime -p librefang-types -p librefang-api --tests -- -D warnings — clean
  • cargo test -p librefang-memory --lib269 passed
  • cargo test -p librefang-runtime --lib (proactive_memory subset) — 57 passed
  • cargo test -p librefang-api --lib --test memory_routes_integration --test agent_kv_authz_integration --test auth_public_allowlistall green

New regression tests pinning each fix:

  • C1 proactive::tests::list_reflects_semantic_store_not_kv_mirror
  • C2 proactive::tests::add_does_not_store_raw_transcript_fallback
  • C3 proactive::tests::popular_memory_decay_is_monotonic_not_frozen + insert_honors_extractor_supplied_confidence
  • C4 proactive::tests::write_guarded_wrappers_reject_viewer_acl
  • C5 decay::tests::forget_variants_stamp_deleted_at_so_prune_sees_them
  • H2 proactive_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}
  • H4 proactive_memory::tests::format_context_caps_prompt_budget_with_truncation_marker
    EOF

…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
@github-actions github-actions Bot added size/L 250-999 lines changed area/kernel Core kernel (scheduling, RBAC, workflows) labels May 28, 2026
github-actions Bot and others added 2 commits May 28, 2026 18:34
…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.
@houko houko changed the title fix(memory): split-brain reads, raw-transcript fallback, RBAC on writes, forget() leak, immortal decay fix(memory): audit sweep — 5 CRITICAL + 7 HIGH (split-brain, RBAC, decay, dedup, prompt budget, async consolidate) May 28, 2026
@github-actions github-actions Bot added area/runtime Agent loop, LLM drivers, WASM sandbox size/XL 1000+ lines changed and removed size/L 250-999 lines changed labels May 28, 2026
houko and others added 2 commits May 29, 2026 05:01
…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
@github-actions github-actions Bot added area/docs Documentation and guides has-conflicts PR has merge conflicts that need resolution labels May 28, 2026
Evan 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).
@houko
houko merged commit 5921aac into main May 29, 2026
17 checks passed
@houko
houko deleted the fix/memory-critical-bundle branch May 29, 2026 00:06
@github-actions github-actions Bot added ready-for-review PR is ready for maintainer review and removed has-conflicts PR has merge conflicts that need resolution labels May 29, 2026
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation and guides area/kernel Core kernel (scheduling, RBAC, workflows) area/runtime Agent loop, LLM drivers, WASM sandbox ready-for-review PR is ready for maintainer review size/XL 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant