Skip to content

Bump json5 from 0.4.1 to 1.3.1#7

Closed
dependabot[bot] wants to merge 1 commit into
mainfrom
dependabot/cargo/json5-1.3.1
Closed

Bump json5 from 0.4.1 to 1.3.1#7
dependabot[bot] wants to merge 1 commit into
mainfrom
dependabot/cargo/json5-1.3.1

Conversation

@dependabot

@dependabot dependabot Bot commented on behalf of github Mar 12, 2026

Copy link
Copy Markdown
Contributor

Bumps json5 from 0.4.1 to 1.3.1.

Release notes

Sourced from json5's releases.

1.3.0

What's Changed

Full Changelog: callum-oakley/json5-rs@1.2.0...1.3.0

1.2.0

Also serialize u128/i128

Full Changelog: callum-oakley/json5-rs@1.1.0...1.2.0

1.1.0

u128/i128 support

Full Changelog: callum-oakley/json5-rs@1.0.0...1.1.0

1.0.0

Full Changelog: callum-oakley/json5-rs@0.4.1...1.0.0

Commits

Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Bumps [json5](https://github.com/callum-oakley/json5-rs) from 0.4.1 to 1.3.1.
- [Release notes](https://github.com/callum-oakley/json5-rs/releases)
- [Commits](callum-oakley/json5-rs@0.4.1...1.3.1)

---
updated-dependencies:
- dependency-name: json5
  dependency-version: 1.3.1
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @github

dependabot Bot commented on behalf of github Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

Labels

The following labels could not be found: dependencies. Please create it before Dependabot can add it to a pull request.

Please fix the above issues or remove invalid values from dependabot.yml.

@houko houko closed this Mar 12, 2026
@dependabot @github

dependabot Bot commented on behalf of github Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

OK, I won't notify you again about this release, but will get in touch when a new version is available. If you'd rather skip all updates until the next major or minor version, let me know by commenting @dependabot ignore this major version or @dependabot ignore this minor version. You can also ignore all major, minor, or patch releases for a dependency by adding an ignore condition with the desired update_types to your config file.

If you change your mind, just re-open this PR and I'll resolve any conflicts on it.

@dependabot
dependabot Bot deleted the dependabot/cargo/json5-1.3.1 branch March 12, 2026 01:59
houko added a commit that referenced this pull request Apr 18, 2026
…ation

Fifth Codex batch on #2704 — covers the skill-evolution subsystem
(code the upstream PR #2694 shipped to main but that shows up in
this PR's diff), plus the KV race on worker telemetry writes.

librefang-skills (evolution.rs):
- fuzzy_find_and_replace rejects empty old_string up front (#16).
- update_skill / patch_skill re-verify skill_dir.exists() under
  the lock so concurrent delete_skill can't be resurrected (#3, #5).
- delete_skill runs validate_name to reject path traversal (#12).
- remove_supporting_file canonicalises + contains-checks the
  target (#6), matching write_supporting_file.
- write_supporting_file scans content BEFORE writing (#7) so a
  rejected update doesn't destroy the pre-existing valid file.

librefang-runtime (tool_runner.rs):
- tool_skill_evolve_delete resolves the real installed skill's
  parent dir via registry.get() (#15) instead of unconditionally
  targeting the global skills_dir.

librefang-types / librefang-kernel (approval policy):
- Default require_approval list includes skill_evolve_* (#14).
  Updated serde `true`-shorthand + tests to match.

librefang-kernel (kernel/mod.rs):
- build_skill_summary sanitises the category key (#1) — tags are
  third-party data.
- extract_json_from_llm_response tries every '{' instead of giving
  up after the first (#8).
- Background skill review uses the agent's resolved driver and
  model for cost attribution (#13); falls back to defaults only
  when manifest resolution fails.

worker (github-stats-worker/index.js):
- Click counts sharded across 8 blobs, unioned on read (#39).
- UI error reports sharded across 4 blobs plus legacy-key fallback
  for historical data (#41).
houko added a commit that referenced this pull request Apr 20, 2026
…oard parity, smoke script

Six follow-ups bundled per request to keep the channel-progress effort in
one PR. Done from #2 onward; #1 (per-OutputFormat backtick adaptation)
was investigated and dropped — backtick renders correctly in TelegramHtml
(<code>), SlackMrkdwn, Discord/Matrix Markdown (inline code), and the
only adapter that strips backticks (PlainText for Mastodon) is already
opted out of the progress path via suppress_error_responses.

#2 buffered_text fallback test: new integration test
   test_bridge_streaming_adapter_kernel_and_transport_both_fail covers
   the V2 4th outcome (send_streaming Err + kernel Err).

#3 surface context_warning PhaseChange so the user sees when the agent's
   context window was trimmed/overflowed. Other phases stay SSE-only.

#4 collapse repeated tool calls within an iteration — replace
   last_progress_tool with iter_tools_seen HashSet cleared at every
   ContentComplete. Batch agents no longer spam "🔧 web_search" per
   parallel call; retries in a later iteration still get a fresh line.

#5 i18n for the "failed" suffix — supports en/zh-CN/es/ja/de/fr (matches
   librefang_types::i18n). Language threaded from
   kernel.config_snapshot().language through start_stream_text_bridge.

#6 prettify tool names — web_search → Web Search; MCP_call → MCP Call
   (preserves internal caps). Backticks dropped from progress lines
   since the prettified form is now the identity.

#8 dashboard ToolCallCard parity — mirror prettifyToolName() in
   dashboard/src/lib/string.ts and apply it in the card header so
   chat reply and dashboard show the same rendering.

#7 live-integration smoke script — scripts/tests/channel_progress_smoke.sh
   automates the daemon-side flow once the user supplies an LLM API key
   and a configured channel adapter. Documents the channel-delivery gap
   (needs an external webhook receiver). No driver injection hook added
   to the kernel — that would have been scope creep.

Tests added/updated:
  - test_prettify_tool_name_snake_to_title / _kebab_and_dotted /
    _preserves_internal_caps
  - test_tr_progress_failed_languages
  - test_bridge_streaming_adapter_kernel_and_transport_both_fail
  - Updated existing assertions to expect prettified names
houko added a commit that referenced this pull request Apr 20, 2026
…m fix, show_progress, i18n, prettify, dashboard parity (#2793)

* feat(channels): channel-progress v2 — Telegram fallback fix, show_progress config, integration test

Three follow-ups to #2792 (channel progress markers):

1. Telegram (streaming-adapter) buffered_text fallback now uses the
   `_status` variant. The same regression class fixed for non-streaming
   adapters in V1 was still latent on the Telegram path:
     - send_streaming Ok + kernel Err → Done reaction + success=true
     - send_streaming Err + buffered → Done reaction + success=true
   Both now correctly emit Error reaction, record_delivery(false), and
   journal Failed when the kernel actually errored. The fallback path
   also honors `suppress_error_responses` when the buffered text is a
   sanitized error (defense-in-depth — Telegram is not in the opt-in
   list today, but the path is now uniform across all adapters).

2. New `agent.toml show_progress` field (default true) plumbed through
   the streaming bridge. When false, the bridge skips both the
   `🔧 tool_name` and `⚠️ tool_name failed` injections — useful for
   agents whose output is parsed downstream, or for pristine-output
   scenarios where status markers would leak into the response. The
   trait-impl side looks up the agent's manifest from the kernel
   registry once per dispatch and passes it through to
   `start_stream_text_bridge[_with_status]`.

3. New end-to-end integration test in
   `crates/librefang-channels/tests/bridge_integration_test.rs` that
   wires a real `BridgeManager` + `MockAdapter` (non-streaming) +
   `MockProgressHandle` (override of
   `send_message_streaming_with_sender_status` that synthesises a
   delta stream with progress markers). Verifies the V2 dispatch
   pipeline actually surfaces progress to non-streaming adapters
   end-to-end via the consolidated `send_response` call.

Tests:
  - test_stream_bridge_show_progress_false_suppresses_all_markers:
    show_progress=false produces no 🔧/⚠️ markers but still flows the
    actual model prose through
  - test_bridge_non_streaming_adapter_sees_progress_markers: full
    BridgeManager → MockAdapter pipeline; verifies progress markers
    end up in the captured `send()` call

Notes:
  - True "live daemon" integration test (per CLAUDE.md) requires
    LLM API keys + a configured channel adapter (Telegram bot token,
    etc.) which the daemon environment does not currently have. The
    in-process integration test exercises the full dispatch wiring
    using real tokio tasks/channels and a mock kernel handle.

* fix(kernel/wizard): add show_progress to AgentManifest struct literal

CI compile error: wizard.rs constructed AgentManifest with an explicit
struct literal listing every field, so adding show_progress in V2 broke
that one site (the wizard agent created via setup intent).

All other AgentManifest constructors use ..Default::default() and pick
up show_progress=true automatically; only this one was a full-literal.

* feat(channels): channel-progress v3 — collapse, i18n, prettify, dashboard parity, smoke script

Six follow-ups bundled per request to keep the channel-progress effort in
one PR. Done from #2 onward; #1 (per-OutputFormat backtick adaptation)
was investigated and dropped — backtick renders correctly in TelegramHtml
(<code>), SlackMrkdwn, Discord/Matrix Markdown (inline code), and the
only adapter that strips backticks (PlainText for Mastodon) is already
opted out of the progress path via suppress_error_responses.

#2 buffered_text fallback test: new integration test
   test_bridge_streaming_adapter_kernel_and_transport_both_fail covers
   the V2 4th outcome (send_streaming Err + kernel Err).

#3 surface context_warning PhaseChange so the user sees when the agent's
   context window was trimmed/overflowed. Other phases stay SSE-only.

#4 collapse repeated tool calls within an iteration — replace
   last_progress_tool with iter_tools_seen HashSet cleared at every
   ContentComplete. Batch agents no longer spam "🔧 web_search" per
   parallel call; retries in a later iteration still get a fresh line.

#5 i18n for the "failed" suffix — supports en/zh-CN/es/ja/de/fr (matches
   librefang_types::i18n). Language threaded from
   kernel.config_snapshot().language through start_stream_text_bridge.

#6 prettify tool names — web_search → Web Search; MCP_call → MCP Call
   (preserves internal caps). Backticks dropped from progress lines
   since the prettified form is now the identity.

#8 dashboard ToolCallCard parity — mirror prettifyToolName() in
   dashboard/src/lib/string.ts and apply it in the card header so
   chat reply and dashboard show the same rendering.

#7 live-integration smoke script — scripts/tests/channel_progress_smoke.sh
   automates the daemon-side flow once the user supplies an LLM API key
   and a configured channel adapter. Documents the channel-delivery gap
   (needs an external webhook receiver). No driver injection hook added
   to the kernel — that would have been scope creep.

Tests added/updated:
  - test_prettify_tool_name_snake_to_title / _kebab_and_dotted /
    _preserves_internal_caps
  - test_tr_progress_failed_languages
  - test_bridge_streaming_adapter_kernel_and_transport_both_fail
  - Updated existing assertions to expect prettified names

* fix(channels): correct success/err pairing + treat timeout as soft success + clippy collapsible_if

Three review-driven fixes for the V3 PR:

Bug 1 — Telegram path outcome 3 (send_streaming Err + kernel Ok) was
recording delivery as success=true with err=Some(stream_error). The
fallback send_response had already delivered the buffered text and the
kernel succeeded, so the transport-side stream error is irrelevant to
delivery accounting — keeping it in the err field produced a
contradictory metric (success AND err). Now: err is Some only when
kernel actually failed.

Bug 2 — TIMEOUT_PARTIAL_OUTPUT_MARKER was being mapped to status=Err,
which flipped the lifecycle reaction to Error and record_delivery to
success=false. Pre-V2 the bridge had no status channel and treated
these turns as Done because the model emitted useful partial output
before the inactivity timer fired. Restore that semantics: status=Ok
for timeouts. The user still sees the "[Task timed out…]" tail
appended to their reply.

Clippy collapsible_if — flatten the inner if show_progress && ... into
match guards on the ToolExecutionResult and PhaseChange arms (CI
clippy -D warnings caught this in run 24651228831).

* chore(channels): drop dead 'let _ = e' annotation

The variable 'e' (stream transport error) is already used at warn! line 2886
and the empty-buffer fallback at line 2953, so the explicit underscore-binding
introduced by the Bug 1 fix is noise — clean it up.

* test(channels): regression coverage for Bug 1 (success/err pairing) + Bug 2 (timeout-as-success)

Both bugs were caught by review, not tests — add the missing coverage so
they cannot silently regress.

Bug 2 unit test: test_stream_bridge_timeout_partial_output_reports_ok_status
Constructs a kernel handle that fails with a string containing
TIMEOUT_PARTIAL_OUTPUT_MARKER. Asserts:
  - The user-facing text channel still receives the "[Task timed out…]"
    tail so the user knows the reply may be incomplete.
  - The status oneshot resolves to Ok(()) — NOT Err — so bridge.rs
    drives the lifecycle reaction to Done and record_delivery to
    success=true. Pre-V2 semantics preserved.

Bug 1 integration test: test_bridge_streaming_adapter_kernel_ok_transport_fail_records_clean_success
Adds MockKernelOkHandle (overrides send_message_streaming_with_sender_status
to emit clean text + status=Ok, AND overrides record_delivery to capture
every (success, err) pair). Combined with the existing
MockFailingStreamingAdapter (always Err on send_streaming), the test
exercises Telegram outcome 3:
  - Fallback send_response delivers the buffered text  ✓
  - record_delivery is called with success=true AND err=None — proving
    the transport-side stream error does NOT leak into the err field
    when the kernel itself succeeded. (Pre-fix this would have been
    success=true, err=Some(stream_e) — a contradictory metric.)

* polish(channels): unify progress-line spacing, codepoint-safe dashboard prettify, smoke script auto-spawn

Three minor follow-ups from review:

#3 — Symmetric blank lines around progress markers.
  ToolUseStart used \n\n…\n; ToolExecutionResult and PhaseChange used
  \n…\n. Adjacent markers (e.g. 🔧 X right before ⚠️ X failed) ended up
  on consecutive lines without a blank separator and many markdown
  renderers collapsed them into one paragraph. All three now use
  \n\n…\n\n so every renderer that respects markdown blank-line
  semantics shows them as separate blocks.

#4 — prettifyToolName() in dashboard/src/lib/string.ts now iterates by
  Unicode codepoint (via spread) instead of UTF-16 unit. A tool name
  starting with a non-BMP character (e.g. emoji) no longer drops its
  surrogate half when uppercased. Mirrors the Rust-side prettifier in
  channel_bridge.rs which uses chars().next() (also codepoint-correct).
  Tool names are usually ASCII so this is forward-looking, not a fix.

#5 — channel_progress_smoke.sh now auto-spawns a temporary agent when
  the daemon has none. Picks the first available LLM key
  (GROQ/OPENAI/ANTHROPIC/MINIMAX) for the provider, sends a minimal
  manifest_toml inline (the SpawnRequest schema requires either
  manifest_toml or template — name alone isn't valid), and despawns
  the agent on exit. Pre-existing agents are still reused untouched.

* fix(channels/tests): extract DeliveryLog type alias to satisfy clippy::type_complexity

CI clippy -D warnings rejected Arc<Mutex<Vec<(bool, Option<String>)>>>
as too complex. Hoist into a named alias — same shape, named contract.
houko added a commit that referenced this pull request Apr 26, 2026
Closes the remaining HIGH findings from PR #3205 review. Refs #3054.

H4 — `/api/config/reload` did not rebuild AuthManager. Editing
`[[users]]`, `[users.tool_policy]`, or `[tool_policy.groups]` then
hitting `/api/config/reload` was a silent no-op — design decision #7
("invalidate per-user permission cache on config reload") was being
violated. Added `HotAction::ReloadAuth`, detected when `users` or
`tool_policy.groups` change. `AuthManager::reload(&[UserConfig],
&[ToolGroup])` clears and repopulates the indexes in place.

H5 — Precedence docstring didn't match the impl. Pinned the canonical
order `tool_policy → channel_tool_rules → tool_categories` in the
`user_policy.rs` module docstring (matches `evaluate()`'s actual
short-circuit order). Added a regression test:
`evaluate_user_deny_beats_channel_allow_for_same_tool`.

H6 — `sender_index` first-write-wins on platform-id collisions.
Two users sharing a platform-id on different channels would alias on
a third unbound channel; if the first registered was Owner, an
unrecognised inbound on a third channel inherited Owner rights.
Removed the bare-`platform_id` index; `resolve_user` now requires an
explicit `(channel, sid)` tuple.

H7 — `sender_id.is_none()` failed OPEN. `resolve_user_tool_decision`
returned `Allow` whenever `sender_id` was missing, bypassing RBAC for
any internal call site. Added an explicit `system_call: bool` flag —
the kernel marks cron / system / internal channels as system calls;
every other no-sender path now goes through the guest gate
(default-deny).

H8 — End-to-end integration test (`tests/rbac_m3_evaluate_tool_call.rs`)
that boots a real `LibreFangKernel` and exercises the full chain:
user_deny / both_allow / no_allow_list_needs_approval /
categories_against_kernel_groups / unrecognised_sender_no_fail_open /
hand_agent_force_human_skips_auto_approve / reload_picks_up_new_policy.
Replaces the test claim in the PR body that pointed to no actual file.

Tests:
  - rbac_m3_platform_id_collision_no_longer_aliases_across_channels (H6)
  - rbac_m3_sender_none_no_system_flag_does_not_fail_open (H7)
  - evaluate_user_deny_beats_channel_allow_for_same_tool (H5)
  - tests/rbac_m3_evaluate_tool_call.rs — 7 cases (H4 + H8)

Refs #3054, #3205
houko added a commit that referenced this pull request Apr 26, 2026
…#3054) (#3205)

* feat(types): per-user tool policy + memory namespace ACL config schema

Adds the data layer for RBAC M3 (issue #3054 Phase 2):

- New module librefang_types::user_policy with UserToolPolicy,
  UserToolCategories, ChannelToolPolicy, UserMemoryAccess, and a
  layered ResolvedUserPolicy::evaluate helper returning
  UserToolDecision::{Allow, Deny, NeedsRoleEscalation}.
- Extends UserConfig with optional tool_policy, tool_categories,
  memory_access, and channel_tool_rules fields. All default to
  None/empty so existing config.toml files keep working unchanged.
- Round-trip serde tests (JSON + TOML) for every new struct, plus
  layering precedence tests for evaluate.

* feat(runtime): enforce per-user tool policy with channel-rule precedence

Wires RBAC M3 tool checks (#3054 Phase 2) into the agent runtime.

Kernel side (librefang-kernel):
- AuthManager now caches a ResolvedUserPolicy per user, built from
  UserConfig.{tool_policy,tool_categories,memory_access,channel_tool_rules}
  at config load. with_tool_groups() takes the kernel ToolGroup list so
  per-user category lookups can resolve group names.
- New resolve_user_tool_decision() returns UserToolGate::{Allow, Deny,
  NeedsApproval}. Order: user tool_policy -> per-user channel rule ->
  tool_categories -> role escalation. Admin/owner role passes through;
  user/viewer escalates unknown tools to NeedsApproval; explicit deny
  short-circuits.
- New memory_acl_for() merges the user UserMemoryAccess with a
  role-default ACL (owner/admin = full, user = proactive+kv:*, viewer =
  proactive read-only).
- Unknown senders fall through guest_gate(): allow well-known read-only
  tools, NeedsApproval for everything else. Empty user list keeps legacy
  behaviour (UserToolGate::Allow everywhere).

Runtime side (librefang-runtime tool_runner):
- After the existing channel-deny check and before the approval gate,
  consult kernel.resolve_user_tool_decision(). Deny short-circuits with
  a hard error containing the reason. NeedsApproval flips
  force_approval=true so the tool routes through submit_tool_approval()
  regardless of the global require_approval list. Allow defers to the
  existing approval logic (no bypass).

Trait surface (librefang-kernel-handle):
- New default-Allow KernelHandle::resolve_user_tool_decision so existing
  test stubs and downstream KernelHandle impls keep compiling unchanged.

Tests:
- 8 new auth.rs tests covering tool_policy deny, role escalation,
  channel-rule precedence, category resolution, guest gate, memory ACL
  fallback, memory ACL override.
- 3 new tool_runner.rs tests driving execute_tool() through each
  UserToolGate variant via a stub KernelHandle.

* feat(memory): gate namespace reads/writes by user memory_access ACL

Adds the memory layer for RBAC M3 (#3054 Phase 2):

- New module librefang_memory::namespace_acl with MemoryNamespaceGuard,
  NamespaceGate, and PII-redaction helpers. The guard wraps the per-user
  UserMemoryAccess ACL and exposes check_read/check_write/check_delete/
  check_export plus redact_item/redact_all.
- Redaction logic checks two signals on each MemoryItem:
  metadata['taint_labels'] containing 'Pii' (replaces full content), or
  the regex stack from taint::redact_pii_in_text (replaces just the
  matched substrings). When pii_access is true, the guard never
  redacts.
- New public taint::redact_pii_in_text() exposes the existing
  email/phone/SSN/credit-card regex stack so the memory crate can reuse
  it without copying.

Wires the guard into two real call sites:
- ProactiveMemoryStore gains search_with_guard / delete_with_guard /
  add_with_guard. Search runs PII redaction on output; delete honours
  delete_allowed; add gates writes to the 'proactive' namespace.
- StructuredStore gains get_with_guard / set_with_guard /
  delete_with_guard. The KV namespace presented to the guard is
  'kv:<key>' so policies can use prefix patterns
  (e.g. readable_namespaces = ['kv:user_*']).

Tests: 11 new tests covering namespace allow/deny, delete/export flag
gating, PII metadata path, PII regex path, pii_access bypass, KV guard
read/write/delete, and prefix matching against globs.

* fix(rbac-m3): wire memory ACL + tool gate to production paths

Closes the bypass routes and dead-code paths that PR #3205 review
flagged as BLOCKING. Refs #3054.

B1 — Memory namespace ACL was scaffolded but every read site still
called the unguarded methods. Routed through *_with_guard variants:

  * api/routes/memory.rs: search/list/get/list_agent/search_agent
    endpoints now build a MemoryNamespaceGuard from the
    AuthenticatedApiUser extension (via AuthManager::memory_acl_for)
    and return 403 on deny.
  * runtime/agent_loop.rs: auto_retrieve resolves the guard via the
    new KernelHandle::memory_acl_for_sender, short-circuits the call
    when "proactive" is denied, and applies PII redaction otherwise.
  * memory/proactive.rs: added search_all_with_guard /
    list_all_with_guard / get_with_guard / list_with_guard wrappers
    for cross-agent paths.
  * memory/namespace_acl.rs: switched the redacted=true marker to
    insert() instead of or_insert_with() so the signal is
    authoritative.

B2 — shell_exec under ExecPolicy.mode=Full silently dropped a
user-gate NeedsApproval. Re-gated skip_approval_for_full_exec on
!force_approval so a user-policy escalation MUST still route through
the approval queue even under Full mode.

B3 — Hand-tagged agent auto-approval ignored the user-gate
NeedsApproval. Threaded a force_human flag through
DeferredToolExecution (default false, #[serde(default)]-compatible).
The runtime sets it true when the user gate returned NeedsApproval;
the kernel's submit_tool_approval skips the hand:* carve-out when
force_human=true.

Tests:
  - tool_runner_rbac_full_mode_does_not_bypass_user_needs_approval (B2)
  - tool_runner_rbac_force_human_propagates_to_deferred (B3)
  - tool_runner_rbac_force_human_stays_false_for_global_require_approval
  - search_all_with_guard_denies_unauthorised_read (B1)
  - search_all_with_guard_redacts_pii (B1)

Refs #3054, #3205

* feat(rbac-m3): rebuild AuthManager on config reload + integration test

Closes the remaining HIGH findings from PR #3205 review. Refs #3054.

H4 — `/api/config/reload` did not rebuild AuthManager. Editing
`[[users]]`, `[users.tool_policy]`, or `[tool_policy.groups]` then
hitting `/api/config/reload` was a silent no-op — design decision #7
("invalidate per-user permission cache on config reload") was being
violated. Added `HotAction::ReloadAuth`, detected when `users` or
`tool_policy.groups` change. `AuthManager::reload(&[UserConfig],
&[ToolGroup])` clears and repopulates the indexes in place.

H5 — Precedence docstring didn't match the impl. Pinned the canonical
order `tool_policy → channel_tool_rules → tool_categories` in the
`user_policy.rs` module docstring (matches `evaluate()`'s actual
short-circuit order). Added a regression test:
`evaluate_user_deny_beats_channel_allow_for_same_tool`.

H6 — `sender_index` first-write-wins on platform-id collisions.
Two users sharing a platform-id on different channels would alias on
a third unbound channel; if the first registered was Owner, an
unrecognised inbound on a third channel inherited Owner rights.
Removed the bare-`platform_id` index; `resolve_user` now requires an
explicit `(channel, sid)` tuple.

H7 — `sender_id.is_none()` failed OPEN. `resolve_user_tool_decision`
returned `Allow` whenever `sender_id` was missing, bypassing RBAC for
any internal call site. Added an explicit `system_call: bool` flag —
the kernel marks cron / system / internal channels as system calls;
every other no-sender path now goes through the guest gate
(default-deny).

H8 — End-to-end integration test (`tests/rbac_m3_evaluate_tool_call.rs`)
that boots a real `LibreFangKernel` and exercises the full chain:
user_deny / both_allow / no_allow_list_needs_approval /
categories_against_kernel_groups / unrecognised_sender_no_fail_open /
hand_agent_force_human_skips_auto_approve / reload_picks_up_new_policy.
Replaces the test claim in the PR body that pointed to no actual file.

Tests:
  - rbac_m3_platform_id_collision_no_longer_aliases_across_channels (H6)
  - rbac_m3_sender_none_no_system_flag_does_not_fail_open (H7)
  - evaluate_user_deny_beats_channel_allow_for_same_tool (H5)
  - tests/rbac_m3_evaluate_tool_call.rs — 7 cases (H4 + H8)

Refs #3054, #3205

* fix(rbac-m3): clippy field_reassign_with_default in integration test

Refs #3054, #3205

* fix(rbac-m3): regen golden + close H7 fail-open at trait wrapper

CI was failing on `kernel_config_schema_matches_golden_fixture` because
the M3 schema additions (`tool_policy`, per-user `tool_categories`,
`memory_access`) never had the golden fixture regenerated — actual
14639 lines vs expected 14496.

While in here, address two PR review items:

  1. `KernelHandle::resolve_user_tool_decision` was treating
     `(sender_id=None, channel=None)` AND `Some("system")` /
     `Some("internal")` as system-internal calls, silently re-opening
     the H7 fail-open the AuthManager unit tests were written to
     close. Only the synthetic `"cron"` channel (the one actually
     synthesized in `kernel/mod.rs ~10876`) keeps the bypass; every
     other unattributed inbound now flows through the guest gate so
     RBAC fails closed end-to-end. New trait-layer test pins this.

  3. `routes/memory.rs::guard_for_request` falls back to an
     owner-equivalent guard when no AuthenticatedApiUser extension is
     attached. The original comment described this as "preserves the
     M2 contract" without flagging the security trade-off; replaced
     with an explicit SECURITY note that the per-user namespace ACL
     only applies when the auth middleware binds a real user, and
     calls out what operators must do to enforce it on every request.

Also: swap `RwLock<Vec<ToolGroup>>` for `RwLock<Arc<Vec<ToolGroup>>>`
in AuthManager so `config_reload`'s in-place swap stays cheap while
the resolution-path readers only pay an `Arc::clone` instead of a
per-call `Vec` clone.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
houko added a commit that referenced this pull request Apr 26, 2026
…tive deny + memory audit emit (#3205) (#3249)

* fix(security): RBAC M3 follow-up — namespace traversal + case-insensitive deny + memory audit emit

Three reviewer-flagged bugs in PR #3205 (RBAC M3):

#7 — Namespace path traversal (REAL):
`UserMemoryAccess::can_read/can_write` deferred to the generic
`capability::glob_matches`, where `*` greedily matches any text including
path separators. So a pattern of `kv:user_*` matched `kv:user_../admin`
or `kv:user_evil/etc/passwd` — letting a memory tool that builds the
namespace from user-controlled input cross into another user's bucket.

Fix: introduce `namespace_glob_matches` with two stricter rules:
  1. Reject any candidate containing a `..` segment (delimited by `/`,
     `:`, or whitespace) outright.
  2. `*` may not span `/` or `:` separators inside a longer pattern
     (e.g. `kv:user_*` only matches a single component after the prefix).

`*` standalone still matches any non-traversing namespace so the
owner / admin "see everything" UX is preserved. Kept separate from the
generic `glob_matches` (which is used for tool-name and capability
matching where collapsing across separators is desirable).

#8a — Case-sensitive deny lists (PARTIAL → fixed):
`UserToolPolicy`, `ChannelToolPolicy`, and `UserToolCategories` matched
tool names case-sensitively, so a deny rule for `shell_exec` would not
catch a hallucinated `SHELL_EXEC` invocation. Built-in tools dispatch
by exact case so the call would have failed downstream anyway, but
MCP / skill tool providers may accept their own case variants and the
deny list is supposed to be the authoritative gate. ASCII-lowercased
both pattern and tool name at every per-user check site.

Limited to per-user policy layers — the rate-limit buckets and
capability matchers continue using exact-case comparison since tool
identities there are content-addressed.

#8b — Memory ACL denial silently dropped from audit chain (REAL):
`routes/memory.rs::auth_denied` returned a 403 without recording a
`PermissionDenied` row, while the parallel `routes/audit.rs`,
`routes/budget.rs`, `routes/authz.rs`, and the global auth middleware
all emit one. A privilege probe against `/api/memory*` was therefore
invisible to an admin reading `/api/audit`.

Fix: extended `auth_denied` to take `&AppState` + `&Extensions` and
emit a `PermissionDenied` audit row before returning 403. Anonymous
callers are recorded with `user_id = None`; authenticated-but-denied
callers carry their attributed `user_id` plus user / role in the
detail.

Tests added:
- `memory_access_namespace_blocks_path_traversal` — `kv:user_../admin`,
  `kv:user_alice/../bob`, and separator-crossing globs all denied.
- `memory_access_star_pattern_still_rejects_traversal` — even `["*"]`
  rejects `..` candidates.
- `user_deny_is_case_insensitive`,
  `channel_deny_is_case_insensitive`,
  `categories_deny_is_case_insensitive` — pin lowercase normalisation.
- `auth_denied_emits_audit_row_for_anonymous` and
  `auth_denied_emits_audit_row_for_authenticated_user` — pin the
  audit-emit contract for both anonymous and attributed denials.

* fix(test): add user_api_keys field to memory.rs audit_test_app_state

#3233 added Arc<RwLock<Vec<ApiUserAuth>>> user_api_keys to AppState.
The new audit_test_app_state helper landed before that merged so its
struct-literal init was missing the field. CI flagged E0063 on macOS
and Windows after rebase. Initialize as empty Vec — these tests don't
exercise auth at all, they call auth_denied directly with synthesized
extensions.
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 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>
houko pushed a commit that referenced this pull request May 29, 2026
…loor, partial-status, dedup-strip, test tightening

8 follow-ups from the code review of the prior commit. All in scope
for the same proactive-memory layer.

* #1 misattributed doc-comment in `proactive.rs` near
  `AUTO_CONSOLIDATE_EVERY` / `NEGATION_WORDS`. The
  `/// Negation/contradiction words …` line was orphaned above
  `AUTO_CONSOLIDATE_EVERY` when the new const got inserted; both
  consts now carry their own intended docstring.

* #2 lowered `STALE_COUNTER_FLOOR` from `AUTO_CONSOLIDATE_EVERY / 2`
  (5) to `/ 4` (2). The /2 floor cleaned up "stuck at 1..4" agents
  but also reset slow-burn agents (single auto_memorize per
  maintenance tick) before they could climb to 10, so a steady
  1-call/hour stream effectively never consolidated. /4 keeps the
  cold-slot eviction directional while letting low-frequency
  agents still accumulate to the trigger.

* #3 `PATCH /api/memory/config` response shape is now explicit about
  partial success. `body.status` is `"applied"` when the reload
  succeeded and `"partial"` when the disk write landed but the
  live reload failed (e.g. operator hand-edited an unrelated
  section into an invalid shape between PATCH writes). Clients
  MUST inspect `status`; the HTTP code stays 200 for both branches
  since the disk write itself succeeded. 207 / 500 were considered
  and rejected — 500 misrepresents that the request was rejected
  (it wasn't) and 207 forces every existing client to re-classify
  success. Mirrors the `import_agent_memory` partial-success
  pattern.

* #4 documented the M13 cost tradeoff. Up to 4 SQLite roundtrips per
  insertion on the no-embedding fallback path (versus the pre-fix
  1), but the embedding-driver path is unaffected and the loop
  short-circuits as soon as the union hits fetch_limit, so the
  common case ("first keyword filled the slate") still runs a
  single query. Operators on the no-embedding path pay the cost
  on writes only, against an already-unindexed `content LIKE`
  scan.

* #5 defensively strip the M14 `_update_threshold_*` keys from the
  enriched item right after `decide_action` returns + assert
  callers don't pre-populate them. Both insertion branches today
  build their stored metadata from the original `item.metadata`
  (not `enriched_item.metadata`), so the threshold keys never
  reach the column — but stripping + asserting is cheap insurance
  for the next refactorer who repoints either branch at the
  enriched copy.

* #6 relaxed the `extract_search_keywords_returns_multiple_ordered_longest_first`
  test: no more exact `kws.len() == 4` — the cap and the "longest
  distinctive word survives" invariants are what we care about,
  not the exact count. Future additions to `STOP_WORDS` no longer
  silently break the test.

* #7 dropped the unused `_update_threshold_cross_cat` set in
  `decide_action_honors_config_update_thresholds`. Both candidate
  memories share the "preference" category, so the cross-cat
  threshold branch was unreachable.

* #8 moved `STALE_COUNTER_FLOOR` from a `fn`-scope `const` to
  module scope, alongside `AUTO_CONSOLIDATE_EVERY` from which it
  derives. Matches the repo convention and lets future readers
  see the floor / trigger relationship at one site.

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 — 273 passed
* cargo test -p librefang-api --test memory_routes_integration —
  14 passed
houko added a commit that referenced this pull request May 29, 2026
…CH, multi-keyword search, configurable UPDATE thresholds (#5850)

* fix(memory): MEDIUM follow-ups — counter map sweep, hot-reload on PATCH, multi-keyword search, configurable UPDATE thresholds

Continuation of the audit sweep on the proactive-memory subsystem.
4 MEDIUM findings from the same audit, all in scope for this PR.

* M11 `consolidation_counters` HashMap is now actively pruned every
  maintenance tick. Pre-fix it only swept when the map crossed 1000
  entries (truncate-to-500 by count DESC), which delayed cleanup
  until the leak was observable AND deleted the highest-count
  entries — exactly the agents about to fire a real consolidate.
  The new sweep drops every counter that hasn't passed the halfway
  mark (`< AUTO_CONSOLIDATE_EVERY / 2 = 5`) on each maintenance
  tick: HashMap::retain in-place, evicts cold entries first, never
  touches an entry that's about to fire. Added named constant
  `AUTO_CONSOLIDATE_EVERY = 10` so the trigger + the prune floor
  stay in lockstep.

* M12 `PATCH /api/memory/config` now calls `kernel.reload_config()`
  after writing `config.toml`, so dashboard saves take effect on
  the running kernel instead of staying disk-only until restart.
  Pre-fix the response always reported `restart_required: true`,
  which confused operators who could see GET return the new values
  while live behaviour (ProactiveMemoryStore::config, decay engine,
  etc.) stayed on the boot snapshot. `restart_required` now reflects
  the actual `ReloadPlan` — false when every diff field hot-reloads,
  true when any field needs a restart. A reload validation failure
  is surfaced via the new `reload_error` field instead of swallowing
  the disk write.

* M13 `extract_search_keywords` returns a `Vec<String>` of the top
  4 distinctive keywords ordered longest-first (post-stop-word
  filter, post-dedup), and the caller iterates over them unioning
  per-keyword LIKE recalls until `fetch_limit` is hit. Pre-fix it
  collapsed the four candidates to the single longest one, wasting
  the stop-word filter work on the other three and giving the
  no-embedding fallback path a frequently-too-generic substring
  (e.g. "analysis") to match against, OR a too-specific compound
  term that matched nothing. The fallback to the raw-content LIKE
  is preserved for the no-distinctive-words case so a near-verbatim
  duplicate is still detectable.

* M14 the `decide_action` UPDATE thresholds are now configurable
  via two new `ProactiveMemoryConfig` fields:
  `update_threshold_same_category` (default 0.7) and
  `update_threshold_cross_category` (default 0.8). The trait method
  signature stays stable — `add_with_decision` stashes the live
  config values in the new memory's metadata under
  `_update_threshold_same_cat` / `_update_threshold_cross_cat`, the
  default `decide_action` reads them out (falling back to the const
  defaults for direct trait-method callers), and the LLM-backed
  extractor inherits the same behaviour via its fallback to the
  default heuristic on driver failure. This separates the
  per-insertion conflict-resolution threshold (UPDATE vs ADD) from
  the post-hoc consolidation threshold (`duplicate_threshold`) —
  pre-fix both were conflated.

Drive-by fmt: two stale formatting hunks `cargo fmt` flagged in
`runtime/tool_runner/wasm_skill.rs:159` and
`api/tests/memory_routes_integration.rs:518` (neither mine, both
inherited from main).

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 — 273 passed (4 new
  regression tests:
  `extract_search_keywords_returns_multiple_ordered_longest_first`,
  `extract_search_keywords_empty_for_all_stop_words`,
  `decide_action_honors_config_update_thresholds`, and the
  existing suite re-verified against the new
  `update_threshold_*_category` config fields)
* cargo test -p librefang-runtime --lib proactive_memory — 60
  passed
* cargo test -p librefang-api --lib --test
  memory_routes_integration --test agent_kv_authz_integration
  --test auth_public_allowlist — all green

* fix(memory): review-followups on #5850 — doc-comment fix, threshold floor, partial-status, dedup-strip, test tightening

8 follow-ups from the code review of the prior commit. All in scope
for the same proactive-memory layer.

* #1 misattributed doc-comment in `proactive.rs` near
  `AUTO_CONSOLIDATE_EVERY` / `NEGATION_WORDS`. The
  `/// Negation/contradiction words …` line was orphaned above
  `AUTO_CONSOLIDATE_EVERY` when the new const got inserted; both
  consts now carry their own intended docstring.

* #2 lowered `STALE_COUNTER_FLOOR` from `AUTO_CONSOLIDATE_EVERY / 2`
  (5) to `/ 4` (2). The /2 floor cleaned up "stuck at 1..4" agents
  but also reset slow-burn agents (single auto_memorize per
  maintenance tick) before they could climb to 10, so a steady
  1-call/hour stream effectively never consolidated. /4 keeps the
  cold-slot eviction directional while letting low-frequency
  agents still accumulate to the trigger.

* #3 `PATCH /api/memory/config` response shape is now explicit about
  partial success. `body.status` is `"applied"` when the reload
  succeeded and `"partial"` when the disk write landed but the
  live reload failed (e.g. operator hand-edited an unrelated
  section into an invalid shape between PATCH writes). Clients
  MUST inspect `status`; the HTTP code stays 200 for both branches
  since the disk write itself succeeded. 207 / 500 were considered
  and rejected — 500 misrepresents that the request was rejected
  (it wasn't) and 207 forces every existing client to re-classify
  success. Mirrors the `import_agent_memory` partial-success
  pattern.

* #4 documented the M13 cost tradeoff. Up to 4 SQLite roundtrips per
  insertion on the no-embedding fallback path (versus the pre-fix
  1), but the embedding-driver path is unaffected and the loop
  short-circuits as soon as the union hits fetch_limit, so the
  common case ("first keyword filled the slate") still runs a
  single query. Operators on the no-embedding path pay the cost
  on writes only, against an already-unindexed `content LIKE`
  scan.

* #5 defensively strip the M14 `_update_threshold_*` keys from the
  enriched item right after `decide_action` returns + assert
  callers don't pre-populate them. Both insertion branches today
  build their stored metadata from the original `item.metadata`
  (not `enriched_item.metadata`), so the threshold keys never
  reach the column — but stripping + asserting is cheap insurance
  for the next refactorer who repoints either branch at the
  enriched copy.

* #6 relaxed the `extract_search_keywords_returns_multiple_ordered_longest_first`
  test: no more exact `kws.len() == 4` — the cap and the "longest
  distinctive word survives" invariants are what we care about,
  not the exact count. Future additions to `STOP_WORDS` no longer
  silently break the test.

* #7 dropped the unused `_update_threshold_cross_cat` set in
  `decide_action_honors_config_update_thresholds`. Both candidate
  memories share the "preference" category, so the cross-cat
  threshold branch was unreachable.

* #8 moved `STALE_COUNTER_FLOOR` from a `fn`-scope `const` to
  module scope, alongside `AUTO_CONSOLIDATE_EVERY` from which it
  derives. Matches the repo convention and lets future readers
  see the floor / trigger relationship at one site.

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 — 273 passed
* cargo test -p librefang-api --test memory_routes_integration —
  14 passed

* fix(memory): review-followups (round 2) — proper M11 idle-window, M12 + M14 regression coverage, comment polish

5 follow-ups from the second review pass on #5850.

* B (was #1) M11 done properly. `consolidation_counters` is now
  `HashMap<String, CounterEntry>` where `CounterEntry` carries both
  the running count and a `last_touched: DateTime<Utc>` stamp.
  The maintenance sweep evicts entries whose `last_touched` is
  older than `STALE_COUNTER_IDLE_WINDOW = 2 hours` (~2× the
  maintenance rate-limit window), regardless of count. The previous
  count-threshold fix (followup #2 in the prior commit) mitigated
  but didn't solve the slow-burn case: any agent firing ≤ 1 ×
  per maintenance window would still be reset before climbing past
  the count floor. The timestamp-based check closes that gap — an
  active slot, however slow, is preserved as long as it's been
  touched within the window; a truly idle slot is reclaimed
  within ~2 hours of going quiet.

* C (was #2) `PATCH /api/memory/config` happy-path test added at
  `memory_routes_integration::patch_memory_config_hot_reloads_and_reports_applied`.
  Pre-seeds a minimal `config.toml` (the harness's tempdir
  previously didn't materialise one, which the file-level docstring
  flagged as out-of-scope; the docstring updated accordingly) and
  asserts `body["status"] == "applied"`, `body["reload_error"]`
  null, and that the PATCHed value round-trips into the response.
  Without this, the M12 status contract could silently revert and
  the rest of the suite wouldn't catch it. `RouterHarness._tmp` is
  exposed as `tmp` to let the test reach the seed location.

* D (was #3) M14 strip regression test added at
  `proactive::tests::add_with_decision_does_not_leak_threshold_keys_to_stored_metadata`.
  Drives the full `add()` path through `add_with_decision`, then
  reads back via `list()` and asserts none of the private
  `_update_threshold_*` / `_embedding` keys leaked into the stored
  metadata column. Catches the regression where someone repoints
  the ADD or UPDATE branch at `enriched_item.metadata` (the
  decision-clone) instead of the original `item.metadata` (the
  caller's input).

* E (was #4) the `debug_assert!` panic messages on the
  `_update_threshold_*` private keys re-worded from "caller leaked
  it" to "callers must not pre-populate ..." — neutral phrasing
  that doesn't presume the caller is buggy. Also added a one-line
  note that the production path stays safe regardless (the
  unconditional `insert` overwrites any leaked value before
  `decide_action` reads it), since `debug_assert!` is compiled out
  in release.

* F (was #5) M13 cost-trade-off comment tightened. The "common
  case still runs a single query" claim was accurate for agents
  with sizeable stores (first keyword exhausts fetch_limit) but
  not for fresh / small stores where no individual keyword has
  enough matches to short-circuit. The comment now distinguishes
  the two regimes instead of overgeneralising.

Re-stamped `.secrets.baseline` line numbers shifted by the test
file edits.

Verification:
* cargo check --workspace --lib — clean
* cargo clippy -p librefang-memory -p librefang-api --tests --
  -D warnings — clean
* cargo test -p librefang-memory --lib — 274 passed (incl.
  `add_with_decision_does_not_leak_threshold_keys_to_stored_metadata`)
* cargo test -p librefang-api --test memory_routes_integration —
  15 passed (incl.
  `patch_memory_config_hot_reloads_and_reports_applied`)

* fix(memory): review-followups (round 3) — M12 test key-presence + accepts partial, M14 strips _embedding too, prune rate-limit, chrono::Duration const portability

6 followups from the third review pass on #5850.

* #1 M12 test pinned the contract properly. `serde_json::Value`
  indexed by a missing key returns `Value::Null`, so the prior
  `assert_eq!(body["reload_error"], Null)` silently passed even if
  the field had been removed. Now asserts `body.as_object()
  .contains_key(...)` for `status`, `restart_required`,
  `reload_error` first, then asserts their values.

* #2 strip + test for the `_embedding` private-stash key.
  `add_with_decision` now also calls
  `enriched_item.metadata.remove("_embedding")` after
  `decide_action` returns, so all three private stash keys
  (`_update_threshold_*` + `_embedding`) get the same defensive
  treatment. Test renamed to
  `add_with_decision_does_not_leak_private_stash_keys_to_stored_metadata`
  and attaches a tiny mock `EmbeddingFn` so the `_embedding` stash
  path actually fires — the prior assertion against `_embedding`
  was decorative because the test had no embedding driver
  configured.

* #3 rate-limited the counter prune via a new `last_counter_prune:
  Arc<Mutex<Option<DateTime<Utc>>>>` and a `maybe_prune_counters`
  helper that mirrors the `maybe_decay_confidence` /
  `maybe_cleanup_expired` once-per-hour pattern. Prior to this the
  prune ran on every `maybe_run_maintenance` call, reachable from
  `search` / `auto_retrieve` / `consolidate` at potentially many
  Hz. The retain itself was microseconds, so this is a wash today,
  but it brings the three maintenance sub-tasks under the same
  scheduling budget for future scaling.

* #4 docstring on `STALE_COUNTER_IDLE_WINDOW_HOURS` re-phrased to
  describe the slot-keeping guarantee in terms of the maintenance
  rate-limit window, not "consecutive prune passes" — the old
  phrasing happened to be true only because the prune wasn't
  rate-limited yet (now rectified by #3).

* #5 M12 test now accepts either `"applied"` or `"partial"` as
  the body status — both are valid post-fix outcomes; the pre-fix
  contract had no `status` field at all. The status field's
  presence (asserted via the #1 fix) is the actual contract we're
  pinning. Made the test robust to future `KernelConfig::default()`
  changes that might cause the seeded toml to fail reload
  validation.

* #6 swapped `const STALE_COUNTER_IDLE_WINDOW: chrono::Duration =
  chrono::Duration::hours(2)` for `const
  STALE_COUNTER_IDLE_WINDOW_HOURS: i64 = 2` plus
  `chrono::Duration::hours(STALE_COUNTER_IDLE_WINDOW_HOURS)` at
  the call site. `chrono::Duration::hours` is a `const fn` at the
  currently pinned `chrono` minor but the const-ness isn't a
  stable contract across `0.4.x` versions, so a lockfile bump
  could silently break the build. The integer-hours + runtime
  conversion stays valid regardless.

Verification:
* cargo check --workspace --lib — clean
* cargo clippy -p librefang-memory -p librefang-api --tests --
  -D warnings — clean
* cargo test -p librefang-memory --lib — 274 passed (incl.
  `add_with_decision_does_not_leak_private_stash_keys_to_stored_metadata`
  with embedding-driver coverage)
* cargo test -p librefang-api --test memory_routes_integration —
  15 passed (incl. tighter
  `patch_memory_config_hot_reloads_and_reports_applied` contract
  assertions)

* fix(memory): review-followups (round 4) — docstring honesty + direct strip helper + non-empty partial-error

3 follow-ups from the fourth review pass on #5850. All three are
about closing gaps between what the code does and what the
docstrings / tests claim it does.

* A `STALE_COUNTER_IDLE_WINDOW_HOURS` docstring: the "reclaimed
  within ~2 hours of going quiet" claim was accurate before the
  round-3 prune rate-limit landed, after which the worst-case
  reclaim latency is 2-3 hours (idle window + up to one prune
  rate-limit period because the prune itself only runs ≤ once per
  hour). Re-phrased with explicit upper/lower bounds; deleted the
  duplicate "previous fix (round-1 followup #2)" paragraph that
  had ended up in the doc twice during the round-2 edit.

* B `strip_private_stash_keys` extracted into a module-level
  function driven by a single
  `ADD_WITH_DECISION_PRIVATE_STASH_KEYS` const, and unit-tested
  directly via
  `strip_private_stash_keys_removes_all_private_keys`. The
  integration test
  `add_with_decision_does_not_leak_private_stash_keys_to_stored_metadata`
  only catches a *coordinated two-step regression* (strip removed
  AND ADD/UPDATE branch repointed at `enriched_item.metadata`) —
  the current ADD path bypasses `enriched_item.metadata` entirely,
  so single-step regressions of either kind pass that test. The
  prior commit's docstring claimed "the strip code on the
  post-decide path is the only thing keeping it out of the stored
  column", which was wrong — `item.metadata` (the caller's input)
  is what actually keeps the keys out today. Updated the docstring
  to match reality and added the direct unit test on the helper to
  cover single-step strip regressions.

* C `reload_error` partial-branch assertion in
  `patch_memory_config_hot_reloads_and_reports_applied` now also
  rejects empty / whitespace-only strings. `is_string()` alone
  passed `""` / `"   "` / any other zero-info value — operators
  would see status=partial with a useless error blob and have no
  actionable diagnostic. Trimmed-non-empty makes the contract
  honest about what "carries the validator output" means.

Verification:
* cargo check --workspace --lib — clean
* cargo clippy -p librefang-memory -p librefang-api --tests --
  -D warnings — clean
* cargo test -p librefang-memory --lib — 275 passed (1 new:
  `strip_private_stash_keys_removes_all_private_keys`)
* cargo test -p librefang-api --test memory_routes_integration —
  15 passed

---------

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant