Skip to content

feat: port openfang PR #522 WeCom support and plain-text replies#9

Merged
houko merged 10 commits into
mainfrom
pr-522-wecom-channel
Mar 12, 2026
Merged

feat: port openfang PR #522 WeCom support and plain-text replies#9
houko merged 10 commits into
mainfrom
pr-522-wecom-channel

Conversation

@houko

@houko houko commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

Ports the changes from RightNow-AI/openfang#522 into LibreFang.

Included:

  • add the WeCom (WeChat Work) channel adapter and config surface
  • handle WeCom callback verification/decryption and preserve hand extension tools
  • render WeCom replies as plain text by default

Validation:

  • cargo test -p librefang-channels formatter::tests::test_wecom_plain_text_strips_common_markdown_blocks -- --nocapture
  • cargo test -p librefang-kernel test_hand_activation_does_not_seed_runtime_tool_filters -- --nocapture

houko and others added 6 commits March 12, 2026 18:12
- 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]>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread crates/librefang-channels/src/wecom.rs Outdated
Comment thread crates/librefang-channels/src/formatter.rs Outdated
@houko
houko merged commit 634d0a5 into main Mar 12, 2026
7 checks passed
@houko
houko deleted the pr-522-wecom-channel branch March 12, 2026 10:33

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment on lines +47 to +48
let iv = &key[..16];
let cipher = Aes256CbcDecrypt::new(key.into(), iv.into());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

houko added a commit that referenced this pull request Apr 17, 2026
… 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.
houko added a commit that referenced this pull request May 4, 2026
…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]>
houko added a commit that referenced this pull request May 11, 2026
…#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>
houko pushed a commit that referenced this pull request May 28, 2026
…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
houko added a commit that referenced this pull request May 29, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant