feat: port openfang PR #522 WeCom support and plain-text replies#9
Conversation
- Add wecom.rs channel adapter implementation - Add WeComConfig in config.rs - Register WeCom adapter in channel_bridge.rs WeCom channel supports: - Inbound messages via callback webhook - Outbound messages via WeCom API - Access token caching and auto-refresh Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add 6 language versions of README: - README.md (English) - README.zh.md (Chinese) - README.ja.md (Japanese) - README.ko.md (Korean) - README.es.md (Spanish) - README.de.md (German) Each version includes cross-links to all translations in the Links section. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e9a884845
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f35d2bae3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| let addr = std::net::SocketAddr::from(([0, 0, 0, 0], port)); | ||
| let listener = tokio::net::TcpListener::bind(addr).await.unwrap(); |
There was a problem hiding this comment.
Handle webhook bind errors without panicking
start() returns success before the webhook listener is bound, but the spawned task calls tokio::net::TcpListener::bind(addr).await.unwrap(). If the port is already in use or cannot be bound, this panics the task and silently disables inbound WeCom processing while the adapter appears started, which is a production-facing reliability failure.
Useful? React with 👍 / 👎.
| let iv = &key[..16]; | ||
| let cipher = Aes256CbcDecrypt::new(key.into(), iv.into()); |
There was a problem hiding this comment.
Reject invalid AES key lengths before decrypting
The decrypt path assumes a valid AES key size and slices key[..16] without validating the decoded encoding_aes_key length first. A malformed key in config (or any value that base64-decodes to fewer than 16/32 bytes) will panic during callback handling instead of returning a parse error, which can terminate the webhook task unexpectedly.
Useful? React with 👍 / 👎.
… key overwrite **Hook-layer conformance (#19, #54)**: WizardPage now imports from `lib/queries/providers` and `lib/mutations/providers` instead of calling `api.ts` functions directly. That brings it in line with the contract in `dashboard/AGENTS.md` — centralised staleTime, shared query keys, and onSuccess invalidations. The direct-call path was diverging from the rest of the dashboard on provider cache behaviour every time the shared hooks were tuned. **Guarded overwrite of a working key (#9)**: when the selected provider already has `auth_status = ready` and the user types a new key, a warning checkbox must be acknowledged before the Connect button is enabled. Previously the wizard wrote the new key first and then ran the test; a typo destroyed the working credential with no recovery path (the backend only reads from env/vault — it has no rollback). The checkbox forces explicit consent so a mistyped key doesn't silently brick a working install. Any edit in the input resets the confirmation so stale approvals don't carry over to different strings.
…esolution (#4534) * feat(api): trusted_proxies + trust_forwarded_for for real-client-IP Adds two top-level KernelConfig fields that let proxied deployments (Cloudflare Tunnel, nginx, Traefik, …) recover the real client IP from forwarding headers without being exploitable when no proxy is present. Closes the long-standing TODO referenced in the now-retired rate_limiter::resolve_client_ip doc comment. trusted_proxies = ["172.19.0.0/16", "127.0.0.1", "::1"] trust_forwarded_for = true When BOTH are set AND the TCP peer matches the allowlist, the daemon resolves the real client IP from forwarding headers (preference: CF-Connecting-IP → X-Real-IP → Forwarded (RFC 7239) → rightmost- untrusted hop in X-Forwarded-For) for: * the GCRA rate limiter (per-IP keying) * the auth-login rate limiter (per-IP keying) * the per-IP WebSocket connection cap + the WS connect log line Fail-closed by default — empty allowlist OR master-switch off OR peer not in allowlist OR malformed headers all collapse back to the TCP peer. A spoofed X-Forwarded-For from an untrusted internet client still hits the limiter on its real source, so the per-IP brute-force properties are preserved. Implementation * New crates/librefang-api/src/client_ip.rs with TrustedProxies (hand-rolled CIDR matcher, no new deps), resolve_real_client_ip, resolve_from_request. * GcraState + new AuthRateLimitState carry Arc<TrustedProxies> + trust_forwarded_for; server.rs compiles the allowlist once at boot and threads it through both middleware layers. * ws.rs::agent_ws uses the resolved IP for the WS slot key and the client_ip log field. * Old fail-closed-only rate_limiter::resolve_client_ip removed. * Auth-bypass loopback gates (no-auth-allow-loopback, LIBREFANG_ALLOW_NO_AUTH) intentionally still key on the TCP peer via addr.ip(), not the resolved IP — auth bypass must never follow a forwarded-header claim. Tests * 19 unit tests in client_ip: CIDR v4/v6, bare IPs, unmasked input, invalid-entry skip, all four header preferences, RFC 7239 bracketed v6 + obfuscated _token, multi-header XFF concatenation, v4:port suffix, malformed-fallback, all-trusted chain. * 3 integration tests in rate_limiter: - browsers behind a trusted proxy get independent buckets - rotating XFF from an untrusted peer does not bypass the limiter - master switch off ignores XFF even from a trusted peer * Existing 611 librefang-api lib + 271 integration tests still pass; workspace cargo clippy --all-targets -D warnings clean. * refactor(api): cache compiled TrustedProxies on AppState Threads `Arc<TrustedProxies>` + `trust_forwarded_for` through `AppState`, compiled once at boot in `server.rs`. The GCRA + auth-login middlewares now read from the cached instance instead of recompiling, and `ws::agent_ws` no longer re-parses the raw config strings (and re-emits the malformed-entry warning) on every WebSocket upgrade. Also adds explicit "Behaviour change" comments at the two `agent_ws` sites that propagate the resolved client IP into `SenderContext.user_id`, so a future reader / operator flipping the new flags on understands that any per-`user_id` kernel state (audit attribution, channel-sender keying, session continuity that keys on it) re-keys from proxy IP to real client IP at that moment. Pure documentation — no behaviour change vs the prior PR commit. Updates the test fixtures (`librefang-testing::test_app::build_state` and the three in-tree `AppState` init sites in `routes/{agents,memory,network}.rs`) to set the new fields to default (header trust off), matching the production default. Addresses review #2 (no per-upgrade recompilation), #5 (migration-notes comments), and #10 (drops the cosmetic `drop(proxy_cfg)` since the allowlist is now read straight off `AppState`). * fix(api): resolve real client IP in terminal_ws WS slot key `terminal_ws` was still keying its per-IP WS slot on `addr.ip()`, which is the same bug `agent_ws` was just fixed for: behind a trusted reverse proxy (cloudflared / nginx / Traefik), the TCP peer is the proxy and every terminal connection from every browser collapses onto a single shared slot — `max_ws_per_ip` then throttles the whole organisation the moment the second tab opens. Mirrors the `agent_ws` fix exactly: pulls the boot-compiled `Arc<TrustedProxies>` and `trust_forwarded_for` flag off `AppState` and calls `client_ip::resolve_real_client_ip` before `try_acquire_ws_slot`. Untrusted peers fall through to `addr.ip()` — a spoofed `X-Forwarded-For` from the open internet still hits the per-IP cap on its real source. `SenderContext.user_id` parity is N/A here: `terminal_ws` does not construct a `SenderContext` (the terminal is a PTY pipe, not a kernel agent message), so the only behavioural surface that needs to swap to the resolved IP is the WS slot key + the rejection log line. Addresses review #1. * fix(api): tighten client_ip parser correctness + safety Five header-parsing fixes lifted from review: - **XFF: bracketed IPv6 with port** — `[2001:db8::1]:1234` fell off the fallback parser because `parse::<IpAddr>` rejects the bracket form and the prior `rsplit_once(':')` left the brackets attached. The walker now strips a surrounding `[...]` (with or without `:port`) before retry, while still bailing on ambiguous unbracketed v6 + port rather than silently truncating. - **RFC 7239 `for=` parameter case-insensitive** — RFC 7230 §3.2.4 makes parameter names case-insensitive; we were only matching `for=` and `For=`. Now compares the key with `eq_ignore_ascii_case("for")`. - **RFC 7239 `unknown` token case-insensitive** — companion fix; now compares with `eq_ignore_ascii_case("unknown")` so `Unknown` / `UNKNOWN` short-circuit the same way as the lowercase form. - **Single-value headers reject the unspecified address** — a misconfigured proxy that injects `0.0.0.0` / `::` into `cf-connecting-ip` / `x-real-ip` no longer poisons the per-IP slot key with an address that can't be a real client. Loopback and RFC1918/ULA addresses are accepted by design — the trusted proxy may legitimately pass an internal-network client. Doc-comment spells out the policy. - **Documentation comments** for the asymmetries reviewers flagged: `single_ip_header` reads only the first instance of the named header (vs the XFF path which concatenates all); the `Forwarded` parser only inspects the first list element and obfuscated tokens short-circuit by design. Adds 8 new unit tests covering: bracketed-v6 with/without port, case-insensitive `for=` (4 variants), case-insensitive `unknown` (3 variants), unspecified-address rejection on both single-value headers (4 variants), loopback + private acceptance (5 variants), plus two WS-slot-key composition tests asserting the resolver collapses spoof attempts from an untrusted peer down to the real TCP source AND separates real clients behind the same trusted proxy into distinct slot keys. Addresses review #3, #4, #6, #7, #8, #9. --------- Co-authored-by: Evan <[email protected]>
…#4906) * feat(workflows): add run cancel, total timeout, retry backoff (#4844) Closes gaps #1, #9, and #10 from issue #4844. Gap #1 — Cancel run - Add WorkflowRunState::Cancelled variant (serialises as "cancelled") - Add WorkflowEngine::cancel_run(): atomically transitions Pending/Running/Paused runs to Cancelled, clears pause snapshot, persists immediately - Add POST /api/workflows/runs/{run_id}/cancel handler (200/400/404/409) - Executor checks for Cancelled at every step boundary and exits early - pause_run, cleanup_terminal_pause_state, list_runs filter, and workflow_run_to_row updated for the new variant Gap #9 — Workflow-wide total timeout - Add Workflow::total_timeout_secs: Option<u64> - Add KernelConfig::workflow_default_total_timeout_secs: Option<u64> - Add WorkflowEngine::default_total_timeout_secs: Option<u64>, wired from config - execute_run wraps inner body in tokio::time::timeout using resolved effective timeout (workflow > kernel default > unbounded); expiry sets state to Failed - API layer threads total_timeout_secs through JSON <-> struct boundary Gap #10 — Retry backoff with jitter - Extend ErrorMode::Retry with backoff_ms: Option<u64> and jitter_pct: Option<u8> (both serde default/skip for backward compat) - Add compute_retry_backoff(): exponential backoff capped at 60 s, optional uniform jitter via rand::random::<u64>() % range; falls back to classify_backoff() when backoff_ms is None - API layer extracts backoff_ms/jitter_pct from both flat and nested JSON forms Tests (crates/librefang-api/tests/workflow_lifecycle_test.rs, 11 new) - Cancel Pending run -> 200, state Cancelled, completed_at set - Cancel Paused run (lodged pause_request) -> 200, pause_request cleared - Cancel terminal (Cancelled) run -> 409 Conflict - Cancel unknown run ID -> 404; malformed run ID -> 400 - total_timeout_secs round-trips through create/get - total_timeout_secs absent when not set (backward compat) - PUT update preserves total_timeout_secs when field omitted from payload - backoff_ms/jitter_pct round-trip through create/get - Retry step without backoff fields deserialises cleanly (backward compat) - list_runs?state=cancelled filter returns only Cancelled runs * chore(codegen): auto-regenerate openapi.json + sdk + schema baselines [skip ci] * fix(workflows): address review on #4906 — typed cancel error, behavioral tests, retry-sleep cancellation, success_rate semantics R1 — Behavioral executor tests (5 new kernel tests in workflow.rs #[cfg(test)]): - cancel_mid_step_stops_execution_and_state_is_cancelled: 3-step workflow, cancel fired after step 1 completes (via Notify gate on step 2's send_message), asserts Err("cancelled"), state=Cancelled, step 3 never ran - total_timeout_fires_and_sets_state_to_failed: workflow with total_timeout_secs=1, step sleeps 3s; uses tokio::time::pause()+advance() to fire the timeout instantly; asserts Failed + timeout error message - compute_retry_backoff_math: pure unit test — exponential doubling, 60s cap, jitter in-range over 20 calls with >= 2 distinct values, None backoff delegates to classify_backoff - cancel_run_returns_not_found_for_unknown_id: typed error variant assert - cancel_during_retry_sleep_aborts_promptly: step always fails, backoff=30s; cancel fires during retry sleep; executor returns in < 500ms real time R2 — Typed CancelRunError + correct HTTP status mapping: - Add pub CancelRunError { NotFound(WorkflowRunId), AlreadyTerminal { run_id, state: &'static str } } with Display + Error impls (no thiserror dep needed) - cancel_run returns Result<(), CancelRunError>; terminal-state arms return AlreadyTerminal with "completed"/"failed"/"cancelled" state strings - cancel_workflow_run handler: remove TOCTOU get_run pre-check; map NotFound -> 404, AlreadyTerminal -> 409 with {"error":"conflict","state":s} so callers can distinguish the three terminal states R3 — Field visibility: - WorkflowEngine::default_total_timeout_secs: pub -> pub(crate) (boot.rs is in the same crate; no external reader exists) Q1 — Cancel-aware retry sleep: - Add cancel_notify: Arc<DashMap<WorkflowRunId, Arc<tokio::sync::Notify>>> to WorkflowEngine; initialized in all three constructors - create_run inserts a fresh Arc<Notify> for each new run - cancel_run calls notify_waiters() on the run's entry after state transition - cleanup_terminal_pause_state removes the entry on terminal transitions - execute_step_with_error_mode gains run_id + cancel_notify params; both retry sleep sites replaced with tokio::select!{ sleep | notified() }, returning Err("workflow run cancelled") immediately on notification - All 4 call sites updated to pass run_id and &self.cancel_notify Q2 — success_rate excludes Cancelled from denominator: - RunAgg gains cancelled: usize field; Cancelled arm counted separately (not merged into failed) - success_rate formula: completed / (completed + failed) only - list_workflows JSON gains "cancelled_count" field - New test: list_workflows_success_rate_excludes_cancelled (3 completed + 3 cancelled via execute_run mock + cancel_run; asserts rate=1.0, cancelled_count=3) * fix(workflows): include Cancelled in retained-run eviction filter Eviction filter at WorkflowEngine::create_run was Completed|Failed only — so a burst of user-initiated cancels pinned cancelled records in the DashMap forever (capped only by MAX_RETAINED_RUNS, but those slots could no longer be reclaimed), pushing out evictable Completed/Failed runs. Add Cancelled to the filter and a regression test that creates + cancels 250 runs and asserts list_runs().len() <= 200. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…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
…cay, dedup, prompt budget, async consolidate) (#5839) * fix(memory): split-brain reads, raw-transcript fallback, RBAC on writes, 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 * chore(codegen): auto-regenerate openapi.json + sdk + schema baselines [skip ci] * fix(memory): tighten dedup thresholds, validate LLM extraction, cap prompt 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. * fix(memory): review-followups — sentinel root user_id, hot-reload threshold, 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 * fix(api): attribute Owner on no-auth loopback so memory writes aren't 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. * fix(memory): read-only recall for listing paths; correct decay doc 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). --------- Co-authored-by: Evan <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Ports the changes from RightNow-AI/openfang#522 into LibreFang.
Included:
Validation: