Skip to content

feat: add config-driven session_mode for agent triggers#2

Closed
neo-wanderer wants to merge 24 commits into
mainfrom
claude/agent-session-triggers-lXO4h
Closed

feat: add config-driven session_mode for agent triggers#2
neo-wanderer wants to merge 24 commits into
mainfrom
claude/agent-session-triggers-lXO4h

Conversation

@neo-wanderer

Copy link
Copy Markdown
Owner

Add SessionMode enum (persistent/new) to AgentManifest so agents can
control whether automated invocations (cron ticks, event triggers,
agent_send) reuse the persistent session or start fresh each time.

  • Per-agent default via session_mode in agent.toml
  • Per-trigger override via Trigger.session_mode: Option<SessionMode>
  • Channel-derived sessions unaffected (always deterministic per-channel)
  • Default is "persistent" (backward-compatible)
  • Applies to hands automatically (shared pipeline)

Introduces TriggerMatch struct to plumb per-trigger session_mode
through trigger evaluation and dispatch.

https://claude.ai/code/session_01WLeiKnazjiRnjyFRQy9inY

Add SessionMode enum (persistent/new) to AgentManifest so agents can
control whether automated invocations (cron ticks, event triggers,
agent_send) reuse the persistent session or start fresh each time.

- Per-agent default via `session_mode` in agent.toml
- Per-trigger override via `Trigger.session_mode: Option<SessionMode>`
- Channel-derived sessions unaffected (always deterministic per-channel)
- Default is "persistent" (backward-compatible)
- Applies to hands automatically (shared pipeline)

Introduces TriggerMatch struct to plumb per-trigger session_mode
through trigger evaluation and dispatch.

https://claude.ai/code/session_01WLeiKnazjiRnjyFRQy9inY
@github-actions

Copy link
Copy Markdown

Thanks for your first pull request! 🎉

A maintainer will review it soon. While you wait, please make sure:

  • PR title follows conventional format (feat:, fix:, docs:, etc.)
  • cargo test --workspace passes
  • cargo clippy --workspace --all-targets -- -D warnings is clean

We aim to provide initial feedback within 7 days. See CONTRIBUTING.md for details.

@github-actions github-actions Bot added area/docs Documentation and guides area/kernel Core kernel (scheduling, RBAC, workflows) labels Apr 12, 2026
@github-actions github-actions Bot added area/ci CI/CD and build tooling area/channels Messaging channel adapters labels Apr 13, 2026
neo-wanderer pushed a commit that referenced this pull request Apr 14, 2026
…ibrefang#2398)

* feat(api): add require_auth_for_reads flag to lock down dashboard reads

The dashboard public-read allowlist in the auth middleware hard-codes
/api/agents, /api/config, /api/budget, /api/sessions, /api/approvals,
/api/hands, /api/skills, /api/workflows and more as GET-public so the
SPA can render before the user enters credentials. With the default
api_listen = "0.0.0.0:4545", any host on the LAN can enumerate agents,
read configuration (minus redacted api_key), observe spend, and list
pending approvals without a token — a hard mismatch with the
"Bearer token authentication" line in SECURITY.md.

Introduce KernelConfig.require_auth_for_reads (default false, so
existing deployments keep rendering unchanged). When it is true AND
api_key is configured, the middleware collapses the allowlist to the
static-asset / OAuth / health subset and forces every dashboard read
through bearer authentication. Unauthenticated health probes, OAuth
callback, and dashboard shell HTML remain reachable.

Refactor the public-path match into matches!()-based groups so clippy
(nonminimal_bool) stays quiet and the two tiers are obviously
separated in code review.

Regression coverage:
- require_auth_for_reads=true blocks unauthenticated GET /api/agents
- require_auth_for_reads=true still allows the correct bearer
- /api/health stays public with the flag on
- require_auth_for_reads=false preserves the legacy public GET

* fix(api): close require_auth_for_reads gaps found in self-review

Three real issues found while reviewing the original commit on this branch:

1. **`/api/status` still reachable.** The status handler at routes/config.rs:48
   returns the full agents listing (id, name, state, model provider, profile)
   plus `home_dir`, `api_listen`, session count and memory usage — exactly
   the enumeration surface `require_auth_for_reads` exists to close. The
   original commit kept it in `always_public_method_free`, so flipping the
   flag did not actually stop `/api/status` from leaking. Moved it into
   `dashboard_read_exact` (still GET-only) so the flag locks it down.

2. **Flag silently no-ops when only user_api_keys or dashboard credentials
   are configured.** The gate was `require_auth_for_reads && api_key_present`
   where `api_key_present = !api_key.trim().is_empty()`. Production
   deployments commonly configure per-user keys or dashboard user/pass
   without a standalone api_key — in those cases, flipping the flag did
   nothing. Replaced with the full "any auth configured" predicate:
   `api_key || !user_api_keys.is_empty() || dashboard_auth_enabled`, which
   mirrors the existing auth-bypass check a few lines below.

3. **No operator feedback when flag is on but no auth exists.** If
   `require_auth_for_reads = true` is set without any auth configured, the
   flag is (correctly) a no-op at the middleware layer — but the operator
   gets no signal. Added a startup `warn!` in `build_router` so the misconfig
   is visible in logs at boot.

Tests added:

- `test_require_auth_for_reads_blocks_api_status` — locks in the fix for #1.
- `test_require_auth_for_reads_engages_with_user_api_keys_only` — locks in
  the fix for #2 (both the 401 on missing creds and the 200 on a valid
  per-user key).
- `test_require_auth_for_reads_is_noop_without_any_auth` — pins the
  middleware contract for #3 (the warning handles UX, middleware stays
  permissive when no auth backends exist).

Verified: `cargo test -p librefang-api --lib middleware` — 20 passed (7 new
for this feature) and `cargo clippy -p librefang-api -p librefang-types
--all-targets -- -D warnings` clean.

* fix(api): also lock /api/health/detail behind require_auth_for_reads

Follow-up finding from self-review. `/api/health/detail`'s own handler doc
comment at routes/config.rs:317 says "requires auth", but the middleware
allowlist had it in the always-public set. The handler returns operational
data that should not be reachable from a cold probe:

- `panic_count` / `restart_count` from the supervisor
- `agent_count`
- `embedding_provider` / `embedding_model` / `extraction_model` (infra leak)
- `config_warnings` — the full output of `KernelConfig::validate()`,
  which can tell a remote attacker exactly what's wrong with the deployment
- event-bus `dropped_events` count

Moved to `dashboard_read_exact` so it gets locked down when the flag is on.
`/api/health` stays public because its payload is genuinely minimal
(`status`, `version`, two-item `checks` array) and load balancers /
orchestrators need it for probing.

Test added: `test_require_auth_for_reads_blocks_api_health_detail` — pins
both contracts (/api/health stays public, /api/health/detail becomes
auth-required) in a single test.

Note: this is a partial fix. With the flag OFF, `/api/health/detail` stays
public to preserve backwards compatibility, which means the handler's own
"requires auth" doc comment is still being violated in the default
configuration. Making it always-require-auth is a separate behavioural
change that belongs in its own PR.

Tests: `cargo test -p librefang-api --lib middleware` — 21 passed
(8 for require_auth_for_reads, up from 7).

* fix(api): close residual info leaks in unauthenticated endpoints

Two more findings from self-review of the always-public set:

1. **`/api/auth/dashboard-check` echoed the configured dashboard username
   to anonymous callers.** The SPA uses this endpoint before the user has
   logged in (to pick the right login form), so the route is legitimately
   unauthenticated — but returning `"username": "<admin>"` handed an
   anonymous remote caller one half of the credential pair, enabling
   targeted credential stuffing against `/auth/dashboard-login`. The
   `mode` field is sufficient for the SPA to pick the right login form;
   the user already knows their own username. Now always returns an empty
   string.

2. **`/api/version` echoed the machine hostname.** Version endpoints
   conventionally expose build info, but the hostname is a per-machine
   identifier that lets a remote probe correlate a daemon to a specific
   deployment target. Dropped from the payload. Operators who need the
   hostname should read it from the daemon's shell environment.

No tests referenced either field, `cargo test -p librefang-api --lib`
passes with 262 tests (no behaviour change for the dashboard SPA beyond
needing the user to type their username at login time, which is how
every other dashboard already works).

Residual pre-existing gap noted for follow-up: `/api/health/detail`'s own
doc comment says it "requires auth" but the middleware kept it public
until this PR's flag was added. With the flag off it's still public,
honouring backwards compatibility for existing operator probes. Making
it unconditionally auth-required is a separate behavioural change.

* fix(api): make /api/health/detail always require auth, matching its doc

The handler at routes/config.rs:317 documents itself as
"Full health diagnostics (requires auth)" and `/api/health`'s doc
explicitly says "Use GET /api/health/detail for full diagnostics
(requires auth)". The middleware was the only thing still treating it as
public — a pre-existing mismatch that this PR's first pass only partially
closed by flag-gating.

Remove it from both `always_public` and `dashboard_read_exact`. With the
endpoint in neither public list, the middleware's default auth-required
path handles it, so `/api/health/detail` now requires a bearer token
regardless of `require_auth_for_reads`. The handler contract and the
middleware contract finally agree.

Breaking change for operators: if anyone was probing `/api/health/detail`
without auth, they'll start getting 401. `/api/health` (minimal liveness)
stays public for load balancers and orchestrators, so the standard
deployment probe path is unaffected. Monitoring setups that want the
detailed view should configure a bearer token — that was the original
design intent.

Test replaced: `test_api_health_detail_always_requires_auth` now pins
both directions — `/api/health` stays public with the flag off, and
`/api/health/detail` is 401 regardless of whether the flag is on or off.

Tests: `cargo test -p librefang-api --lib middleware` — 21 passed.
neo-wanderer pushed a commit that referenced this pull request Apr 14, 2026
…uired (librefang#2429)

The original code had two parallel paths to pull \`www_authenticate_header\`
out of an \`rmcp::ClientInitializeError\`, and both were dead code:

1. \`extract_auth_required\` walked \`std::error::Error::source()\` hoping to
   downcast to \`StreamableHttpError::AuthRequired\`, but
   \`ClientInitializeError::TransportError::error\` is not annotated with
   \`#[source]\`, so \`source()\` never reaches inside \`DynamicTransportError\`.
   A \`TODO(rmcp)\` comment blamed the missing annotation, but (a) that's
   hypothetical future work and (b) it isn't the right diagnosis — see #2.

2. \`extract_www_authenticate\` scraped \`e.to_string()\` for a
   \`www_authenticate_header: "..."\` marker. Also dead in practice:
   rmcp's \`Display\` for \`StreamableHttpError::AuthRequired\` is literally
   \`#[error("Auth required")]\` — the field name never appears in any
   Display layer of the chain, only in \`Debug\`. The scraper looked busy
   but never returned \`Some(_)\`.

Net effect: every auth-required MCP handshake silently went into OAuth
discovery with \`www_authenticate = None\`, so Tier 1 discovery
(resource_metadata from the header) was permanently skipped and we
always fell through to Tier 2 (\`.well-known\` on the server URL).

Fix: \`DynamicTransportError\` already exposes its
\`pub error: Box<dyn Error + Send + Sync>\` publicly, so we can match
\`ClientInitializeError::TransportError { error, .. }\` directly, then
\`downcast_ref::<StreamableHttpError<reqwest::Error>>()\` on the box.
The downcast reaches \`AuthRequired\` without any \`source()\` traversal
or upstream annotation changes.

- Replace both dead helpers with a single
  \`extract_auth_header_from_error\` that does the direct match + downcast.
- Keep the substring check (\`"401"\` / \`"Unauthorized"\` /
  \`"Auth required"\`) only as a defensive fallback so we don't regress
  if rmcp ever reshapes its chain.
- Drop the \`TODO(rmcp)\` comment (no longer blocked on upstream).
- Replace the two old unit tests with
  \`test_extract_auth_header_from_error_returns_none_for_non_transport_variant\`.
  The positive case can't be constructed from outside rmcp because
  \`AuthRequiredError\` is \`#[non_exhaustive]\`, but the negative-path test
  pins the "bail out on the wrong variant" invariant.

Closes librefang#2401, librefang#2402 — the bot issues auto-generated from the TODO
comment. The TODO is gone and the functionality it promised is now
actually present.

Verified: \`cargo test -p librefang-runtime-mcp --lib\` — 49 passed.
\`cargo clippy -p librefang-runtime-mcp --all-targets -- -D warnings\`
— clean.
neo-wanderer pushed a commit that referenced this pull request Apr 14, 2026
…ibrefang#2503)

* feat(gateway): add lib/identity.js identity normalization module (ID-01)

- Pure functional module: isLidJid, isGroupJid, normalizeDeviceScopedJid,
  extractE164, phoneToJid, resolvePeerId, deriveOwnerJids
- No side effects on require, no hidden state, cache passed as param
- resolvePeerId honors 5-step heuristic from CONTEXT §A Specifics
- Unit tests: 41 assertions across 7 describe blocks

* refactor(gateway): import lib/identity in index.js

Add import of identity helpers next to existing lib/echo-tracker import.
No call-site changes yet — subsequent commits migrate sites one at a time.

* refactor(gateway): use deriveOwnerJids for OWNER_JIDS

Replace inline Set(map(n => n.replace(/^\+/, '') + '@s.whatsapp.net'))
with the module helper. Behavior-preserving.

* refactor(gateway): use phoneToJid for outbound JID normalization

Unify the 3 identical inline copies in sendMessage/sendImage/sendAudio
(they were verbatim duplicates; diff confirmed identical). Group JIDs
still passthrough, phones still coerced to @s.whatsapp.net form.

* refactor(gateway): use resolvePeerId + extractE164 in main cascade (ID-01, ID-03)

- Main sender-resolution cascade (~line 1171-1197) replaced with a single
  resolvePeerId() call that honors the 5-step heuristic from CONTEXT §A.
- Phone derivation via extractE164() — also fixes latent bug where
  device-scoped incoming JIDs ('123:[email protected]') previously yielded
  malformed '+123:45' phone strings; now correctly yields '+123'.
- console.warn for unresolved identity becomes structured JSON per ID-03:
  {event: 'identity_unresolved', jid, reason, lid_cache_size, confidence}.
- Preserved side effects outside the pure function per §Concerns #1:
  senderPn cache write and CS-02 resolveLidProactively both still run
  BEFORE resolvePeerId — the function only resolves, it never writes.
- Renamed local 'isLidJid' (shadowed module fn) to 'isLid' per §Concerns
  #2 — all downstream references (ownerLidJids check at line ~1211)
  updated to the alias.

* refactor(gateway): use identity helpers in catchup path

- isLidJid/isGroupJid module fns replace inline endsWith checks.
- phoneToJid replaces inline '+'-strip + '@s.whatsapp.net' append.

* refactor(gateway): use isGroupJid in getGroupParticipants guard

Final inline endsWith('@g.us') site removed. Zero inline JID suffix
manipulation remains in index.js.

* test(gateway): equivalence + ID-03 log tests for identity refactor

- 12 equivalence assertions comparing pre-refactor inline logic to
  lib/identity helpers across: isLid, isGroup, deriveOwnerJids,
  phoneToJid, resolvePeerId (5 fixture shapes: plain phone, LID+senderPn,
  LID+cache, LID+participant, LID unresolvable), extractE164 device-strip
  latent fix, normalizeDeviceScopedJid passthrough.
- 1 assertion validating ID-03 identity_unresolved JSON log shape
  (event, jid, reason, lid_cache_size, confidence all present).
- Test count: 129 -> 142.

---------

Co-authored-by: Federico Liva <[email protected]>
github-actions Bot and others added 15 commits April 14, 2026 18:35
The backend already exposes POST /api/agents/{id}/stop (wired to the
kernel's AbortHandle in running_tasks), and the client helper
stopAgent() existed in api.ts, but no UI surfaced it — users had no
way to cancel a long-running agent loop from the chat view.

While a response is streaming, the Send button now morphs into a red
Stop button (Square icon, same position). Clicking it calls stopAgent
for the current agent and optimistically finalizes the streaming
message + releases the per-agent loading flag, so the textarea, voice
button, and Send button re-enable immediately without waiting for the
WebSocket to emit a terminal event. Late response/error frames from
the backend still reconcile the message content via the existing
handlers.

Adds chat.stop_hint translations for en and zh.
…m events in comms feed

Two related fixes for the task event bus integration (librefang#2788):

1. `POST /api/comms/task` called `memory_substrate().task_post()` directly,
   bypassing `Kernel::task_post` which publishes `SystemEvent::TaskPosted`.
   Tasks created via HTTP never hit the event bus, so triggers with
   `pattern: "TaskPosted"` didn't fire for UI-posted tasks. Route through
   the `KernelHandle::task_post` wrapper instead.

2. Extend `filter_to_comms_event` to map `SystemEvent::TaskPosted`,
   `TaskClaimed`, and `TaskCompleted` to their corresponding
   `CommsEventKind` variants (which already existed) so they appear in
   `GET /api/comms/events` alongside messages and lifecycle events.
session_mode in AgentManifest only applies to event triggers and
agent_send. Cron jobs synthesize SenderContext{channel:"cron"} which
takes the channel branch, so all cron fires for an agent share one
(agent,"cron") session regardless of session_mode. Channel messages
and forks also bypass it. Document the coverage table and the cron
gotcha in CLAUDE.md so future trigger/cron registrations pick session
behavior consciously.
houko added a commit that referenced this pull request Apr 20, 2026
…m fix, show_progress, i18n, prettify, dashboard parity (librefang#2793)

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

Three follow-ups to librefang#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.
@github-actions github-actions Bot added the has-conflicts PR has merge conflicts that need resolution label Apr 20, 2026
neo-wanderer added a commit that referenced this pull request Apr 26, 2026
…el env

Three security fixes to env_passthrough on top of the operator-side
config landed in the previous commit. All three were merge blockers
on PR librefang#3219.

1. Hard-block FORBIDDEN_PASSTHROUGH (review #2). Names like LD_PRELOAD,
   PYTHONPATH, NODE_OPTIONS, DYLD_INSERT_LIBRARIES, etc. are dropped
   regardless of skill manifest or operator config — these inject code
   or redirect imports/library lookup and would defeat the env_clear
   isolation entirely. Not overridable; even per-skill operator
   overrides cannot unblock them.

2. Hard-block KERNEL_RESERVED_ENV. Names the kernel sets explicitly
   per-runtime (PATH, HOME, PYTHONIOENCODING, NODE_NO_WARNINGS, …)
   are dropped. A skill cannot widen a deliberately narrowed kernel
   PATH or override HOME via env_passthrough.

3. Apply passthrough before kernel env settings (review #3).
   tokio::process::Command::env is last-write-wins; the original PR
   set PATH/HOME/PYTHONIOENCODING and *then* called
   apply_env_passthrough, so a skill with env_passthrough = ["PATH"]
   could clobber the kernel-curated PATH. Reordered so passthrough
   runs first; kernel settings always win. Belt-and-suspenders, the
   resolver also strips kernel-reserved names so the ordering is
   defense-in-depth, not the only line of defense.

Plumbing:
- librefang-types: EnvPassthroughPolicy with from_skills_config.
- KernelHandle: skill_env_passthrough_policy() with default-None impl;
  kernel impl pulls from KernelConfig.skills.
- librefang-skills::loader: resolve_effective_passthrough applies the
  full pipeline (forbidden → kernel-reserved → operator deny → per-
  skill override). execute_skill_tool now takes the policy and
  resolves once before dispatching to the per-runtime spawner.
- tool_runner: fetches policy via the kernel handle at the dispatch
  site so the existing execute_tool signature is unchanged.
neo-wanderer added a commit that referenced this pull request Apr 27, 2026
…ang#3219)

* feat(skills): add env_passthrough allowlist to skill manifest

Skills can now declare an env_passthrough list in skill.toml that
specifies host environment variables to forward into the skill
subprocess. The default remains empty — full env_clear isolation —
preserving the security posture for third-party skills.

Mirrors the existing [exec_policy].allowed_env_vars mechanism for
shell_exec. Concrete motivation: skills that wrap a CLI requiring
credentials in env (e.g., gog with GOG_KEYRING_PASSWORD for its
file-backed keyring) cannot work today because env_clear strips
those vars before the subprocess sees them.

The allowlist is per-skill, names only — values are not stored in
the manifest. Variables not present in the host environment are
silently skipped. Visibility is opt-in and the manifest is public,
so users can audit what each skill imports.

Plumbed through execute_python, execute_node, and execute_shell via
a small apply_env_passthrough helper. WASM skills are out of scope
(separate sandbox model). Includes serde round-trip tests and a
loader integration test that asserts allowlisted vars reach the
subprocess and non-allowlisted vars do not.

* feat(skills): operator-side env_passthrough config + denied patterns

Adds [skills].env_passthrough_denied_patterns and
[skills].env_passthrough_per_skill to KernelConfig so the operator,
not the skill author, has the final say on which host env vars cross
the env_clear boundary into a skill subprocess.

Defaults to deny-by-default for the long tail of credential
conventions (*_KEY, *_TOKEN, *_PASSWORD, *_SECRET, *_API_KEY, AWS_*,
GITHUB_*). Operators grant exceptions per-skill via
env_passthrough_per_skill — e.g.
  [skills.env_passthrough_per_skill]
  gog = ["GOG_KEYRING_PASSWORD"]

Introduces EnvPassthroughPolicy in librefang-types as the runtime
representation. Construction wired up via from_skills_config; the
loader-side resolution that consumes it lands in the next commit.

Addresses review #1 on PR librefang#3219: skill manifest's env_passthrough
allowlist is author-controlled, but credentials are an operator
concern. Without an operator-side gate, a community skill shipping
env_passthrough = ["AWS_SECRET_ACCESS_KEY"] would silently leak
every secret in the host environment to anyone who installs it
without auditing the manifest.

* fix(skills): block dangerous env vars + apply passthrough before kernel env

Three security fixes to env_passthrough on top of the operator-side
config landed in the previous commit. All three were merge blockers
on PR librefang#3219.

1. Hard-block FORBIDDEN_PASSTHROUGH (review #2). Names like LD_PRELOAD,
   PYTHONPATH, NODE_OPTIONS, DYLD_INSERT_LIBRARIES, etc. are dropped
   regardless of skill manifest or operator config — these inject code
   or redirect imports/library lookup and would defeat the env_clear
   isolation entirely. Not overridable; even per-skill operator
   overrides cannot unblock them.

2. Hard-block KERNEL_RESERVED_ENV. Names the kernel sets explicitly
   per-runtime (PATH, HOME, PYTHONIOENCODING, NODE_NO_WARNINGS, …)
   are dropped. A skill cannot widen a deliberately narrowed kernel
   PATH or override HOME via env_passthrough.

3. Apply passthrough before kernel env settings (review #3).
   tokio::process::Command::env is last-write-wins; the original PR
   set PATH/HOME/PYTHONIOENCODING and *then* called
   apply_env_passthrough, so a skill with env_passthrough = ["PATH"]
   could clobber the kernel-curated PATH. Reordered so passthrough
   runs first; kernel settings always win. Belt-and-suspenders, the
   resolver also strips kernel-reserved names so the ordering is
   defense-in-depth, not the only line of defense.

Plumbing:
- librefang-types: EnvPassthroughPolicy with from_skills_config.
- KernelHandle: skill_env_passthrough_policy() with default-None impl;
  kernel impl pulls from KernelConfig.skills.
- librefang-skills::loader: resolve_effective_passthrough applies the
  full pipeline (forbidden → kernel-reserved → operator deny → per-
  skill override). execute_skill_tool now takes the policy and
  resolves once before dispatching to the per-runtime spawner.
- tool_runner: fetches policy via the kernel handle at the dispatch
  site so the existing execute_tool signature is unchanged.

* test(skills): unit tests for resolver + serialize env-mutation test; docs

Test coverage for the env_passthrough resolver (5 unit tests):
- Forbidden vars (LD_PRELOAD, PYTHONPATH) are blocked.
- Kernel-reserved vars (PATH, HOME, …) are blocked.
- Forbidden match is case-insensitive.
- Operator deny patterns (AWS_*, *_KEY) work.
- Per-skill override unblocks denied vars only for the named skill,
  and cannot bypass the FORBIDDEN_PASSTHROUGH hard block.

Mark the existing env-mutating integration test
#[serial_test::serial(skill_env_passthrough)] (review #4): it calls
std::env::set_var/remove_var which mutates process-global state and
would race with any other env-touching test under the parallel
default of cargo test. The serial harness is already used by hands/
extensions for the same reason.

Docs (review #5): add an Environment Variable Passthrough section to
docs/src/app/agent/skills/page.mdx covering the two-party opt-in
model (skill author declares; operator gates), the resolution order,
the FORBIDDEN list, and a worked gog example. Calls out the
distinction from skill config variables — credentials should go
through config, not env.

* fix(skills): address PR 3219 review — case-insensitive deny, CLI policy, runtime warn

- resolve_effective_passthrough lowercases both pattern and name before
  glob match, closing a bypass where 'aws_secret_access_key' would slip
  past the default 'AWS_*' deny pattern (Windows env vars are
  case-insensitive at the OS level). New unit test asserts mixed-case
  manifest entries are blocked.
- librefang skill test now loads [skills] from ~/.librefang/config.toml
  and applies the same EnvPassthroughPolicy the daemon does, falling
  back to SkillsConfig::default() when no config exists. Dev runs see
  the same gate prod will apply instead of silently passing every
  declared var.
- registry.load_skill emits a tracing::warn when env_passthrough is
  non-empty for Builtin / PromptOnly / Wasm runtimes, since the field
  only flows to subprocess-spawning runtimes (Python / Node / Shell)
  and was previously inert without feedback.
- Drop stale 'Returns None' sentence on EnvPassthroughPolicy::from_skills_config
  doc; the Optional-ness lives on KernelHandle::skill_env_passthrough_policy.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
neo-wanderer pushed a commit that referenced this pull request Apr 28, 2026
librefang#3418)

* feat(kernel/workflow): pause/resume foundation for resumable workflows

Closes part of librefang#3335. ApprovalManager integration, REST/CLI surface,
and DAG-path support are explicit follow-ups (see PR description).

Adds the state machine + persistence primitives that make
"pause-mid-pipeline, resume-from-the-same-step" possible. Today the
workflow engine runs to completion or fails; with this PR a caller can
lodge a pause request, the sequential executor honors it at the next
step boundary, and a later `resume_run` call replays from the saved
snapshot.

What lands:

- `WorkflowRunState::Paused { resume_token, reason, paused_at }` —
  carries the token a caller must present to `resume_run`. Existing
  `is_paused()` helper for ergonomic matches.
- `WorkflowRun` gains four fields, all `#[serde(default)]` /
  `skip_serializing_if` for backward compat with persisted JSON:
    - `pause_request: Option<PauseRequest>` (set by `pause_run`,
      consumed at the next step boundary)
    - `paused_step_index: Option<usize>` (where to resume)
    - `paused_variables: HashMap<String, String>` (`{{var}}` bindings)
    - `paused_current_input: Option<String>` (input that would have
      fed the paused step, restored on resume)
- New `PauseRequest { reason, resume_token }`. Token is generated when
  `pause_run` is called so callers can begin waiting on the
  corresponding approval / input artifact before the executor has
  honored the request — the executor reuses the same token at the
  transition.
- `WorkflowEngine::pause_run(run_id, reason) -> Uuid`
    - Idempotent: paused-already returns the existing token.
    - Errors on Completed / Failed.
- `WorkflowEngine::resume_run(run_id, token, resolver, sender) -> Result<String>`
    - Verifies state is `Paused` and token matches.
    - Restores snapshot, flips state to `Running`, re-enters
      `execute_run_sequential`. Snapshot fields are cleared on
      successful resume; a wrong-token attempt leaves state untouched.
- `execute_run_sequential` reads `pause_request` at the top of every
  step iteration. On honoring, snapshots `(step_index, variables,
  current_input)` into the run, transitions to `Paused`, returns the
  intermediate output without erroring. In-flight steps are allowed
  to finish — partial-step rollback would be a much bigger feature.
- `execute_run_dag` refuses cleanly if a pause request was lodged,
  rather than silently dropping it. Pause/resume on DAG runs is the
  follow-up.
- `persist_runs` filter extended to include `Paused`. Paused runs
  round-trip through daemon restart with their token + snapshot
  intact; a fresh `load_runs` call restores them and a subsequent
  `resume_run` works as if no restart had happened.

Tests (5 new in `workflow::tests`):

- `pause_after_first_step_then_resume_finishes_workflow` — sender
  triggers pause from inside step 1; loop honors at the boundary
  before step 2; resume completes the run with both
  `step_results.len() == 2` and snapshot fields cleared.
- `resume_run_with_wrong_token_is_rejected` — resume with a fresh
  random token errors and leaves state Paused.
- `resume_run_on_non_paused_run_is_rejected` — resume on a run still
  in Pending errors with "expected Paused".
- `paused_run_round_trips_through_persist_and_load` (multi-thread
  flavor) — Phase 1 builds a paused run on engine #1 and lets
  `execute_run`'s built-in `persist_runs_async` flush; Phase 2 loads
  into engine #2 via `block_in_place(load_runs)` and asserts the
  resume_token + snapshot survive.
- `pause_request_on_dag_workflow_returns_explicit_error` — a paused
  request on a DAG workflow returns the explicit "follow-up needed"
  error rather than silently completing.

Pre-flight: `cargo clippy --workspace --all-targets -- -D warnings`
clean, `cargo test -p librefang-kernel --lib --no-fail-fast` 637
passed (5 new, no regressions).

Out of scope (tracked as follow-ups against the same librefang#3335):

- ApprovalManager wiring — a step returning "needs approval" should
  trigger pause + register with the existing TOTP/escalation flow.
  This PR's primitive is the substrate that integration uses.
- API endpoints `POST /api/workflows/runs/{id}/resume` and
  `GET /api/workflows/runs/paused` for dashboard / external callers.
- CLI `librefang workflow run paused list / resume <id>`.
- DAG executor pause/resume support (per-layer checkpoints).
- GC for paused workflows older than a configurable TTL.
- Capability check that the approver matches the original requester.

* fix(kernel/workflow): preserve resume snapshot on failure + tighten pause hygiene

Address review feedback on librefang#3418:

1. Resume snapshot preserved across failure. `execute_run_sequential`
   used to `take()` the snapshot on entry, so any step error after
   restoration left the run as Failed with no recovery data — a future
   retry-from-failure path was structurally impossible. Switched to
   clone-on-read; the snapshot is now authoritatively cleared only
   when the run reaches Completed. Failed runs carry their snapshot
   forward (forward-compat for retry semantics). The pause-mid-loop
   branch already overwrites the snapshot with fresh values when
   transitioning back to Paused, so no stale data leaks.

2. Orphan `pause_request` cleanup. If a pause request was lodged
   between the loop's final step-boundary check and the run's
   completion, `pause_request` survived as dead data on Completed
   runs. The Completed transition now clears `pause_request`,
   `paused_step_index`, `paused_variables`, and
   `paused_current_input` together, keeping the invariant 'snapshot
   fields valid only on Paused (or Failed retry-candidates)'.

3. `paused_variables: HashMap → BTreeMap`. The persisted JSON now
   has stable key order — re-serializing a Paused run twice produces
   byte-identical output, which keeps audit logs and external
   snapshots clean. Mirrors the project's existing
   deterministic-ordering convention for LLM-bound prompt data
   (librefang#3298) even though pause snapshots don't directly enter prompts.

4. `paused_at` doc comment: pointed at the future TTL-based GC
   sweep that will consume it (librefang#3335 GC follow-up), so future
   readers know the field has a downstream consumer.

`cargo test -p librefang-kernel --lib workflow` passes (65 tests,
including `paused_run_round_trips_through_persist_and_load`).
`cargo test -p librefang-api --test config_schema_golden` clean
(no fixture drift).

* fix(kernel/workflow): pause/resume foundation review fixups

Address review feedback on PR librefang#3418 (open). Picks up the
already-uncommitted improvements (BTreeMap-backed paused_variables for
deterministic JSON, clone-not-take in resume snapshot read,
Completed-path snapshot clear on the sequential happy path) and adds
the items the multi-angle review flagged:

1. **DAG ghost-state cleanup (quality BUG, security FIX-doc).** The
   DAG entry guard previously returned `Err(...)` and left
   `pause_request: Some(_)` on a run still in `Running`, so an
   unrunnable workflow looked alive forever. The guard now flips the
   run to `Failed` with a descriptive `error` field before returning,
   and a new `cleanup_terminal_pause_state` post-pass at the bottom
   of `execute_run` / `resume_run` clears every pause field on any
   terminal transition (Completed or Failed) — so the same five
   `clear_pause_state` lines aren't sprinkled across the ~10 mid-step
   `Failed` sites in the existing executors.

2. **Rollback safety on `load_runs` (quality OK-with-caveat).**
   Previously a single bad row in `workflow_runs.json` killed the
   whole `Vec<WorkflowRun>` deserialization, so an old daemon reading
   a file written by a newer one (e.g. one with the new tagged
   `Paused { ... }` variant) would lose all run history. Now parses
   per-row via `Vec<serde_json::Value>` first, attempts each
   `from_value::<WorkflowRun>` independently, and skips unrecognized
   rows with a WARN. Newer daemons reading older files still parse
   cleanly because every post-foundation field is `#[serde(default)]`.

3. **Security doc-comments (security FIX-doc).** Added explicit
   warnings on `PauseRequest.reason` /
   `WorkflowRunState::Paused { reason }`: persisted plaintext to
   `workflow_runs.json`, surfaced to operators — not a secret store.
   Same `# SECURITY:` block on `PauseRequest.resume_token`: tokens are
   plaintext at rest in the foundation; the future REST handler must
   hash-at-rest before that surface ships. Added a "DAG workflows
   fail-closed on pause" warning to `pause_run`'s docs so a
   confused caller does not silently brick a DAG run.

4. **Three new tests (quality MISS).**
   - `pause_run_is_idempotent_returns_same_token`: triple-call returns
     the same Uuid; the *first* reason wins (later calls don't
     overwrite the message in logs / UI).
   - `resume_run_after_completion_is_rejected`: pause → execute →
     resume to completion → second resume with the same token errors
     with "expected Paused".
   - `pause_then_execute_on_pending_pauses_at_step_zero`: lock down
     the pause-before-any-step path explicitly (was previously only
     exercised as a side-effect of `resume_run_with_wrong_token`).
   - Existing DAG-pause test extended to assert the new
     `state == Failed` + `pause_request is None` shape.

Pre-flight: `cargo clippy --workspace --all-targets -- -D warnings`
clean, `cargo test -p librefang-kernel --lib --no-fail-fast` 642
passed (8 pause/resume tests, 0 regressions).

Out of scope, still tracked against librefang#3335:
- ApprovalManager wiring + capability check (approver matches the
  original requester).
- REST `POST /api/workflows/runs/{id}/resume` and `GET /api/workflows/runs/paused`.
- CLI `librefang workflow run paused list / resume <id>`.
- Per-layer DAG pause/resume (today: fail-closed).
- GC for paused workflows older than a configurable TTL.
- Hash-at-rest for `resume_token` in `workflow_runs.json`.
- WorkflowPaused / WorkflowResumed hook events for external observers.
neo-wanderer pushed a commit that referenced this pull request Apr 28, 2026
…P / FangHub skills (librefang#3422)

* fix(dashboard): only show pointer cursor on Cards that actually click

`<Card hover>` unconditionally added `cursor-pointer` on top of the
visual hover effects (border tint + shadow lift). That made stat
cards, FangHub skill cards in browse view, and any other purely
informational cards display a pointer cursor — users naturally click
them and get nothing back.

Reported as "drawer doesn't open" on /dashboard/skills (default
source is `fanghub`, where SkillCard is rendered without
`onViewDetail` by design — those cards aren't meant to drill in,
but they still looked clickable). Same UX trap on /dashboard/plugins
and /dashboard/mcp where the cards have no detail drawer at all,
and on Hands' running-now chips.

Fix: gate `cursor-pointer` on the presence of an `onClick` handler.
The `hover` prop now only controls the visual hover tint/lift it
was always meant to control, and clickability is signalled by what
actually exists on the element.

Verified: pnpm typecheck.

* feat(dashboard): add detail drawers for plugins / MCP / FangHub skills

Followup on the cursor-pointer fix in this PR. Those pages didn't
have a detail drawer at all, so even with the cursor honest, users
can't drill into a plugin/server/FangHub-skill they're curious about.
Add inline DrawerPanel-based detail surfaces, matching the patterns
already used by HandsPage / ChannelsPage / ClawHub skill cards.

PluginsPage:
  - Plugin list rows now clickable (role=button), opens a drawer
    showing name + version + author + full description + hooks +
    metadata (size, install path), with Install-deps and Uninstall
    actions inside the drawer. Existing inline action buttons keep
    working (already stop propagation).

McpServersPage:
  - ServerCard's body region (header + stats + transport summary)
    is now clickable to open a drawer showing transport details
    (command + args, or url), env vars, headers, OAuth state when
    pending, and the full tools list. Edit / Taint / Delete actions
    are mirrored inside the drawer for one-click reach. Bottom
    Edit/Taint/Delete buttons and the Tools expand-toggle keep
    working as before (they're outside the clickable region).

SkillsPage:
  - FangHub SkillCard, previously a dead card with no detail surface,
    now passes onViewDetail to a new inline DrawerPanel showing
    name + version + author + full description + tags, with the
    same Install button as the marketplace card. ClawHub / SkillHub
    detail flows are unchanged.

Verified:
  - pnpm typecheck, pnpm build pass
  - Playwright: clicking a FangHub skill card opens the drawer with
    the right content. Plugin/MCP equivalents follow the same
    pattern (couldn't reproduce live without seed data, but the
    code paths are isomorphic to the verified SkillsPage one).

* feat(kernel/workflow): pause/resume foundation for resumable workflows (librefang#3418)

* feat(kernel/workflow): pause/resume foundation for resumable workflows

Closes part of librefang#3335. ApprovalManager integration, REST/CLI surface,
and DAG-path support are explicit follow-ups (see PR description).

Adds the state machine + persistence primitives that make
"pause-mid-pipeline, resume-from-the-same-step" possible. Today the
workflow engine runs to completion or fails; with this PR a caller can
lodge a pause request, the sequential executor honors it at the next
step boundary, and a later `resume_run` call replays from the saved
snapshot.

What lands:

- `WorkflowRunState::Paused { resume_token, reason, paused_at }` —
  carries the token a caller must present to `resume_run`. Existing
  `is_paused()` helper for ergonomic matches.
- `WorkflowRun` gains four fields, all `#[serde(default)]` /
  `skip_serializing_if` for backward compat with persisted JSON:
    - `pause_request: Option<PauseRequest>` (set by `pause_run`,
      consumed at the next step boundary)
    - `paused_step_index: Option<usize>` (where to resume)
    - `paused_variables: HashMap<String, String>` (`{{var}}` bindings)
    - `paused_current_input: Option<String>` (input that would have
      fed the paused step, restored on resume)
- New `PauseRequest { reason, resume_token }`. Token is generated when
  `pause_run` is called so callers can begin waiting on the
  corresponding approval / input artifact before the executor has
  honored the request — the executor reuses the same token at the
  transition.
- `WorkflowEngine::pause_run(run_id, reason) -> Uuid`
    - Idempotent: paused-already returns the existing token.
    - Errors on Completed / Failed.
- `WorkflowEngine::resume_run(run_id, token, resolver, sender) -> Result<String>`
    - Verifies state is `Paused` and token matches.
    - Restores snapshot, flips state to `Running`, re-enters
      `execute_run_sequential`. Snapshot fields are cleared on
      successful resume; a wrong-token attempt leaves state untouched.
- `execute_run_sequential` reads `pause_request` at the top of every
  step iteration. On honoring, snapshots `(step_index, variables,
  current_input)` into the run, transitions to `Paused`, returns the
  intermediate output without erroring. In-flight steps are allowed
  to finish — partial-step rollback would be a much bigger feature.
- `execute_run_dag` refuses cleanly if a pause request was lodged,
  rather than silently dropping it. Pause/resume on DAG runs is the
  follow-up.
- `persist_runs` filter extended to include `Paused`. Paused runs
  round-trip through daemon restart with their token + snapshot
  intact; a fresh `load_runs` call restores them and a subsequent
  `resume_run` works as if no restart had happened.

Tests (5 new in `workflow::tests`):

- `pause_after_first_step_then_resume_finishes_workflow` — sender
  triggers pause from inside step 1; loop honors at the boundary
  before step 2; resume completes the run with both
  `step_results.len() == 2` and snapshot fields cleared.
- `resume_run_with_wrong_token_is_rejected` — resume with a fresh
  random token errors and leaves state Paused.
- `resume_run_on_non_paused_run_is_rejected` — resume on a run still
  in Pending errors with "expected Paused".
- `paused_run_round_trips_through_persist_and_load` (multi-thread
  flavor) — Phase 1 builds a paused run on engine #1 and lets
  `execute_run`'s built-in `persist_runs_async` flush; Phase 2 loads
  into engine #2 via `block_in_place(load_runs)` and asserts the
  resume_token + snapshot survive.
- `pause_request_on_dag_workflow_returns_explicit_error` — a paused
  request on a DAG workflow returns the explicit "follow-up needed"
  error rather than silently completing.

Pre-flight: `cargo clippy --workspace --all-targets -- -D warnings`
clean, `cargo test -p librefang-kernel --lib --no-fail-fast` 637
passed (5 new, no regressions).

Out of scope (tracked as follow-ups against the same librefang#3335):

- ApprovalManager wiring — a step returning "needs approval" should
  trigger pause + register with the existing TOTP/escalation flow.
  This PR's primitive is the substrate that integration uses.
- API endpoints `POST /api/workflows/runs/{id}/resume` and
  `GET /api/workflows/runs/paused` for dashboard / external callers.
- CLI `librefang workflow run paused list / resume <id>`.
- DAG executor pause/resume support (per-layer checkpoints).
- GC for paused workflows older than a configurable TTL.
- Capability check that the approver matches the original requester.

* fix(kernel/workflow): preserve resume snapshot on failure + tighten pause hygiene

Address review feedback on librefang#3418:

1. Resume snapshot preserved across failure. `execute_run_sequential`
   used to `take()` the snapshot on entry, so any step error after
   restoration left the run as Failed with no recovery data — a future
   retry-from-failure path was structurally impossible. Switched to
   clone-on-read; the snapshot is now authoritatively cleared only
   when the run reaches Completed. Failed runs carry their snapshot
   forward (forward-compat for retry semantics). The pause-mid-loop
   branch already overwrites the snapshot with fresh values when
   transitioning back to Paused, so no stale data leaks.

2. Orphan `pause_request` cleanup. If a pause request was lodged
   between the loop's final step-boundary check and the run's
   completion, `pause_request` survived as dead data on Completed
   runs. The Completed transition now clears `pause_request`,
   `paused_step_index`, `paused_variables`, and
   `paused_current_input` together, keeping the invariant 'snapshot
   fields valid only on Paused (or Failed retry-candidates)'.

3. `paused_variables: HashMap → BTreeMap`. The persisted JSON now
   has stable key order — re-serializing a Paused run twice produces
   byte-identical output, which keeps audit logs and external
   snapshots clean. Mirrors the project's existing
   deterministic-ordering convention for LLM-bound prompt data
   (librefang#3298) even though pause snapshots don't directly enter prompts.

4. `paused_at` doc comment: pointed at the future TTL-based GC
   sweep that will consume it (librefang#3335 GC follow-up), so future
   readers know the field has a downstream consumer.

`cargo test -p librefang-kernel --lib workflow` passes (65 tests,
including `paused_run_round_trips_through_persist_and_load`).
`cargo test -p librefang-api --test config_schema_golden` clean
(no fixture drift).

* fix(kernel/workflow): pause/resume foundation review fixups

Address review feedback on PR librefang#3418 (open). Picks up the
already-uncommitted improvements (BTreeMap-backed paused_variables for
deterministic JSON, clone-not-take in resume snapshot read,
Completed-path snapshot clear on the sequential happy path) and adds
the items the multi-angle review flagged:

1. **DAG ghost-state cleanup (quality BUG, security FIX-doc).** The
   DAG entry guard previously returned `Err(...)` and left
   `pause_request: Some(_)` on a run still in `Running`, so an
   unrunnable workflow looked alive forever. The guard now flips the
   run to `Failed` with a descriptive `error` field before returning,
   and a new `cleanup_terminal_pause_state` post-pass at the bottom
   of `execute_run` / `resume_run` clears every pause field on any
   terminal transition (Completed or Failed) — so the same five
   `clear_pause_state` lines aren't sprinkled across the ~10 mid-step
   `Failed` sites in the existing executors.

2. **Rollback safety on `load_runs` (quality OK-with-caveat).**
   Previously a single bad row in `workflow_runs.json` killed the
   whole `Vec<WorkflowRun>` deserialization, so an old daemon reading
   a file written by a newer one (e.g. one with the new tagged
   `Paused { ... }` variant) would lose all run history. Now parses
   per-row via `Vec<serde_json::Value>` first, attempts each
   `from_value::<WorkflowRun>` independently, and skips unrecognized
   rows with a WARN. Newer daemons reading older files still parse
   cleanly because every post-foundation field is `#[serde(default)]`.

3. **Security doc-comments (security FIX-doc).** Added explicit
   warnings on `PauseRequest.reason` /
   `WorkflowRunState::Paused { reason }`: persisted plaintext to
   `workflow_runs.json`, surfaced to operators — not a secret store.
   Same `# SECURITY:` block on `PauseRequest.resume_token`: tokens are
   plaintext at rest in the foundation; the future REST handler must
   hash-at-rest before that surface ships. Added a "DAG workflows
   fail-closed on pause" warning to `pause_run`'s docs so a
   confused caller does not silently brick a DAG run.

4. **Three new tests (quality MISS).**
   - `pause_run_is_idempotent_returns_same_token`: triple-call returns
     the same Uuid; the *first* reason wins (later calls don't
     overwrite the message in logs / UI).
   - `resume_run_after_completion_is_rejected`: pause → execute →
     resume to completion → second resume with the same token errors
     with "expected Paused".
   - `pause_then_execute_on_pending_pauses_at_step_zero`: lock down
     the pause-before-any-step path explicitly (was previously only
     exercised as a side-effect of `resume_run_with_wrong_token`).
   - Existing DAG-pause test extended to assert the new
     `state == Failed` + `pause_request is None` shape.

Pre-flight: `cargo clippy --workspace --all-targets -- -D warnings`
clean, `cargo test -p librefang-kernel --lib --no-fail-fast` 642
passed (8 pause/resume tests, 0 regressions).

Out of scope, still tracked against librefang#3335:
- ApprovalManager wiring + capability check (approver matches the
  original requester).
- REST `POST /api/workflows/runs/{id}/resume` and `GET /api/workflows/runs/paused`.
- CLI `librefang workflow run paused list / resume <id>`.
- Per-layer DAG pause/resume (today: fail-closed).
- GC for paused workflows older than a configurable TTL.
- Hash-at-rest for `resume_token` in `workflow_runs.json`.
- WorkflowPaused / WorkflowResumed hook events for external observers.

* feat(runtime/kernel): BeforePromptBuild hook can contribute prompt sections (librefang#3358)

* feat(runtime/kernel): BeforePromptBuild hook can contribute prompt sections

Closes librefang#3326.

Extends the existing `librefang-runtime::hooks` system so handlers
registered for `BeforePromptBuild` can return a labeled `DynamicSection`
that gets injected into the system prompt at build time. Previously the
event was observe-only.

The original issue proposal was an async `PromptContextProvider` trait
with a 1s wall budget, but on inspection we already had a synchronous
`HookHandler` covering the same lifecycle event. Reusing it (sync, no
trait churn, no parallel registry) is strictly simpler. Providers that
need bounded blocking work (e.g. an active-memory subagent recall) can
post results to a shared cache from a background tokio task and read
the cache synchronously inside `provide_prompt_section`.

Changes:

- `librefang-runtime/src/hooks.rs`
  - Add `DynamicSection { provider, heading, body }`.
  - Add `HookHandler::provide_prompt_section` with default `Ok(None)`.
  - Add `HookRegistry::collect_prompt_sections` enforcing
    `PER_SECTION_CHAR_CAP = 8 KiB` and `TOTAL_DYNAMIC_CHAR_CAP = 32 KiB`,
    dropping over-budget contributors with a WARN log.
  - 5 new unit tests cover empty/registration-order/none-and-err/
    per-section truncation/total-cap drop.

- `librefang-runtime/src/prompt_builder.rs`
  - Add `dynamic_sections: Vec<DynamicSection>` to `PromptContext`.
  - Render at the tail of `build_system_prompt`, after Section 15
    (Live Context). Empty heading + empty body = no-op.
  - 3 new unit tests cover ordering relative to Live Context, the
    empty case, and the blank-section no-op invariant.

- `librefang-kernel/src/kernel/mod.rs`
  - At all 3 `build_system_prompt` call sites (`send_message_ephemeral`,
    `send_message_streaming_with_sender_and_opts`, `execute_llm_agent`):
    construct a `HookContext` with phase + call_site + user_message +
    channel_type + is_subagent + granted_tools, call
    `self.hooks.collect_prompt_sections`, and pass the result through
    the new `dynamic_sections` field.

- `librefang-kernel/src/kernel/tests.rs`
  - 2 new integration tests covering the ephemeral path: handler fires
    with the expected `HookContext.data` payload; a handler registered
    on a different event must not fire.

Pre-flight: `cargo check --workspace --all-targets` clean,
`cargo clippy --workspace --all-targets -- -D warnings` clean, all
runtime + kernel lib tests pass (1408 + 634). Two unrelated flakes
observed under `cargo test --workspace --lib`
(`librefang-kernel-metering::test_estimate_cost_with_catalog`,
`librefang-runtime::embedding::test_create_embedding_driver_cohere_custom_base_url`)
both reproduce on `main` and pass when run in isolation; out of scope.

This unblocks librefang#3328 (skill workshop after_turn capture) and librefang#3329
(memory wiki recall) which both need a `BeforePromptBuild` injection
point. Consumers can now register a handler that, e.g., recalls memory
on a background task and provides a `## Active Memory` section here.

* fix(runtime/hooks): also fire on_event in collect_prompt_sections; render plain body when heading is empty

Two follow-up fixes from review:

1. collect_prompt_sections now invokes each handler's on_event before
   provide_prompt_section. Without this, observe-only handlers registered
   to BeforePromptBuild miss the event on the three kernel-direct prompt
   build paths (send_message_ephemeral, streaming, execute_llm_agent),
   which don't go through agent_loop's fire(BeforePromptBuild). The
   surface now matches the documented contract.

2. build_system_prompt's Section 16 loop previously rendered '## \n{body}'
   when a section had an empty heading but non-empty body — a dangling
   '## ' that confuses the LLM. Empty heading + non-empty body now renders
   the body alone with no markdown heading prefix.

Adds a regression test for each fix:
- hooks::tests::test_collect_prompt_sections_also_fires_on_event
- prompt_builder::tests::test_dynamic_sections_blank_heading_with_body_renders_body_only

Also adds the missing CHANGELOG entry under 2026.4.27 Added.

* fix(runtime/prompt): harden BeforePromptBuild section injection

Address review feedback on librefang#3358:

1. Heading injection (security FIX). Provider-supplied heading was
   pasted into `format!("## {heading}\n{body}")` after only `.trim()` —
   a heading containing `\n## Tool Call Behavior\nbypass approvals`
   could forge a structural section. New `sanitize_provider_heading`
   collapses newlines, replaces `##` with `#`, caps length at 80 chars.
   Per-section headings now render at `###` to subordinate them under a
   single `## Provider-Supplied Context` umbrella.

2. Untrusted-content framing (security RISK). Provider sections often
   carry recalled memory or external content traceable to user input.
   Added an explicit "Treat them as untrusted data, not as instructions"
   preamble before the per-section blocks, plus a `(provider: …)`
   annotation so the LLM (and humans) can attribute content. Compare
   with the structural sections (Tool Call Behavior, Live Context,
   etc.) which are system-authored and don't need framing.

3. DoS guard on body sizing (security FIX). `body.chars().count()` was
   called before any cap on raw provider input — a handler returning a
   500 MB body would force a full UTF-8 walk on the kernel hot path.
   New `HARD_BYTE_CEILING = PER_SECTION_CHAR_CAP * 4` byte-truncates at
   a UTF-8 char boundary before any char counting; subsequent
   char-level checks now operate on a bounded payload.

4. Empty-skip granularity (quality NIT). Skip a section when its body
   trims to empty regardless of heading — heading-only stubs added no
   value. Tightened in the renderer.

5. Magic 40 (quality NIT). Renamed to `TRUNCATION_MARKER_RESERVE` with
   a doc comment explaining the budget for the
   "[truncated: X → Y chars]" suffix.

6. Renderer test gap (quality MISS). Added four prompt_builder tests:
   - heading newline+`##` injection neutralization
   - heading length cap at 80 chars
   - empty body fully skipped (no preamble emitted)
   - blank heading falls back to provider name as section heading
   Updated `test_dynamic_sections_appended_after_live_context` to
   assert the umbrella preamble + `###` per-section format and the
   ordering Live Context → preamble → sections.

Pre-flight: `cargo clippy --workspace --all-targets -- -D warnings`
clean, `cargo test -p librefang-runtime --lib` 1412 / `librefang-kernel
--lib` 634 — both green.

No external API changes. `DynamicSection` shape and the
`HookHandler::provide_prompt_section` signature are unchanged; the
caps and constants are private. Existing handlers keep working byte
identically as long as their headings don't contain newlines or `##`
sequences (which would have been malformed anyway).

* feat(dashboard): add detail drawers for plugin marketplace + MCP catalog;
                drop misleading cursor on session rows

Followup pass on the rest of the dashboard's clickable-looking-but-inert
cards. Three changes:

PluginsPage (Marketplace / Registry tab):
  - Each registry plugin card now opens a detail drawer with the full
    (untruncated) description, all hook tags, repo link, version,
    author, and an Install / Installed action. Inline Install button
    on the card stops propagation so it doesn't double-fire as a
    drawer-open + install.

McpServersPage (Catalog tab):
  - Catalog template card body opens a detail drawer with the full
    description, all tags (the card showed only first 4), required
    env vars list, multi-line setup_instructions block (the card
    didn't surface this at all), and a single Install button. The
    bottom Install link on the card itself is unchanged.

SessionsPage:
  - Session row had `cursor-pointer` baked into its className without
    any onClick handler — the cursor was lying to users. Removed.
    The row already exposes Switch / Delete / Edit-label as
    individual buttons, so there's no extra detail to surface in a
    drawer.

Verified:
  - pnpm typecheck, pnpm build pass.
  - Playwright e2e: clicking a registry plugin card opens the drawer
    with the right content (auto-summarizer with hooks + repo link).
neo-wanderer pushed a commit that referenced this pull request May 1, 2026
…librefang#4162)

* refactor(api): typed allowlist consts + enumeration test against route drift

Replace the inline string arrays in middleware::auth() with three typed
pub const slices (PUBLIC_ROUTES_ALWAYS, PUBLIC_ROUTES_GET_ONLY,
PUBLIC_ROUTES_DASHBOARD_READS) and a PublicRoute / PublicMethod /
PublicMatch set of types.  is_public() is rewritten to walk these
consts via a shared matches_route() helper.

Auth semantics are unchanged for every route; this is a pure
refactor of the allow-list representation.

Add crates/librefang-api/tests/auth_public_allowlist.rs with four tests:
- always_public_paths_are_in_catalog: catalog consistency check (no HTTP)
- dashboard_read_paths_are_in_catalog: same for dashboard-reads group
- authed_paths_are_not_in_any_public_catalog: guards against accidental
  promotion of auth-required routes into the public lists
- always_public_routes_are_reachable_without_token: HTTP check via
  tower::ServiceExt::oneshot
- authed_routes_require_token: HTTP check — Authed paths must 401
- dashboard_read_routes_reachable_without_auth_when_no_key_configured:
  HTTP check — DashboardRead paths pass through in no-auth dev mode

Routes I'm uncertain about:
- /api/config (GET): currently Authed in test table (not in any public
  list in middleware), but /api/config/schema is AlwaysPublic. If the
  full config endpoint should be dashboard-readable, add it to
  PUBLIC_ROUTES_DASHBOARD_READS and the test table.

Fixes librefang#3712

* fix(api): close MCP prefix leak, restore /api/config dashboard read

CRITICAL #1: Remove `prefix_get("/api/mcp/servers/")` from
PUBLIC_ROUTES_GET_ONLY. The prefix was added to support the OAuth
callback but its broadness exposed GET /api/mcp/servers/{name}
(returns server config including env vars) and /auth/status (returns
McpAuthState carrying OAuthTokens) without authentication. The OAuth
callback path /api/mcp/servers/*/auth/callback remains public via the
dedicated is_mcp_oauth_callback guard (prefix + suffix check) that was
already present in auth() — no functional change to the callback path.

CRITICAL #2: Add `exact_get("/api/config")` back to
PUBLIC_ROUTES_DASHBOARD_READS. The old allowlist had it in
dashboard_read_exact; it was dropped when the allowlist was refactored.
The handler already redacts secrets, so public read was intentional.
The dashboard SPA calls GET /api/config via getFullConfig() on every
render.

HIGH: Rewrite auth_public_allowlist.rs file header to accurately
describe the test as a static documentation test with a hardcoded
hand-maintained table, not a live router drift gate. The previous
comment claimed adding a server.rs route without updating the table
would fail CI — that was false.

MEDIUM: Update PUBLIC_ROUTES_ALWAYS doc comment to accurately describe
its grouping-by-semantic-role ordering rather than claiming all three
slices are sorted alphabetically.

Also: add regression test mcp_servers_prefix_does_not_leak_protected_paths
that asserts /api/mcp/servers/{name} and /auth/status return 401 while
/auth/callback stays non-401. Update is_in_always_public() helper to
mirror the is_mcp_oauth_callback guard so catalog-consistency checks
still pass for callback paths. Remove stale comments referencing the
removed prefix entry.
neo-wanderer added a commit that referenced this pull request May 4, 2026
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`).
houko added a commit that referenced this pull request May 4, 2026
…esolution (librefang#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]>
neo-wanderer pushed a commit that referenced this pull request May 4, 2026
…part 1) (librefang#4544)

* fix(memory): async wrappers for kernel substrate calls (librefang#3378 part 1)

The hottest async kernel paths today take `std::sync::Mutex<Connection>`
synchronously on the tokio worker thread. Per librefang#3378, when one holder
runs a slow `INSERT` (FTS5 tokenization, transactional cascades), the
worker stalls and every unrelated future scheduled on it is delayed
until the lock is released. Only `save_session_async` and the embedding
helpers used `spawn_blocking`; the rest of the surface did not.

This change wires every substrate method that is reached from an async
fn through `tokio::task::spawn_blocking`, so the connection mutex is
always acquired on the blocking pool, never on a runtime worker:

- `save_agent_async`
- `load_all_agents_async`
- `remove_agent_async`
- `structured_get_async`
- `get_session_async`
- `get_agent_session_ids_async`
- `delete_canonical_session_async`
- `append_canonical_async`
- `vacuum_if_shrank_async`

The original sync methods are kept verbatim. Tests, migrations, and
the (still-sync) `KernelHandle` trait methods that aren't on a hot
async path use them unchanged, so no caller's signature shifts.

Inside `crates/librefang-kernel/src/kernel/mod.rs` the seven async-fn
call sites that took the connection mutex on a runtime worker are
migrated to the new `_async` siblings:

- `execute_llm_agent` — `save_agent`, `append_canonical`
- `start_background_agents` — `load_all_agents`, `remove_agent`,
  `vacuum_if_shrank`
- `replace_tool_result_in_session` — `get_session` (×2)

These are the high-frequency paths: every LLM turn fires
`execute_llm_agent`, every reboot fires `start_background_agents`,
and every approval / replay fires `replace_tool_result_in_session`.

A regression test in `substrate.rs` (`async_wrappers_do_not_park_
current_thread_runtime`) holds the connection mutex from a non-tokio
OS thread for ~30 ms, then drives `save_agent_async` on a single-
threaded tokio runtime concurrently with a 5 ms `tokio::time::sleep`.
If the wrapper had taken the mutex on the runtime worker (the pre-fix
pattern) the sleep would block for the full hold time; the test
asserts it returns within 25 ms.

This is a partial fix for librefang#3378. Three other surfaces are still
synchronous and called from async paths and need follow-up:

1. ~21 sync kernel methods called from axum handlers (`kill_agent`,
   `reset_session`, `set_agent_model`, etc.). These need their
   `KernelHandle` trait counterparts converted to async, or each
   axum call site wrapped in `spawn_blocking`. Bigger surface,
   warrants its own PR.

2. `MeteringEngine` (`librefang-kernel-metering`) — `record`,
   `check_all_and_record`, `check_user_budget`,
   `check_global_budget` are sync and called from
   `send_message_full_with_upstream` and `execute_llm_agent`.

3. `AuditLog` (`librefang-runtime/src/audit.rs`) — `record` /
   `record_with_context` are sync, called from the same async
   hot paths as #2.

Refs librefang#3378.

* refactor(memory): share inner fns + ordering-based regression test (librefang#3378 part 1 fixup)

Addresses review feedback on PR librefang#4544.

* Extract remove_agent_inner / vacuum_inner free fns. The sync method
  takes its own conn lock and forwards; the async wrapper takes the
  lock inside spawn_blocking and forwards. One transaction body, one
  truth — a future change to the agent-deletion strategy or the VACUUM
  flow (e.g. adding a new per-agent table) only has to land in one
  place. Substrate.rs:203-205 already warned about exactly this kind
  of cross-place coupling.

* Rewrite async_wrappers_do_not_park_current_thread_runtime to assert
  on ordering instead of a 25 ms wall-clock threshold. The blocker
  thread captures released_at the instant the lock guard drops; a
  tokio task captures tick_at after a 20 ms sleep. The test asserts
  tick_at < released_at — true in the correct (offloaded) case
  because the tick fires during the 100 ms hold; false in the broken
  case because the runtime is parked until the lock releases, so
  released_at lands first. No timing budget — the test stays
  decisive under heavy CI jitter (Windows / llvm-cov instrumentation
  has been observed to add 30-80 ms per scheduler hop on this repo,
  see librefang#3989 series).

* Add tracking comments to the three async wrappers that don't yet
  have an in-tree caller (structured_get_async,
  get_agent_session_ids_async, delete_canonical_session_async).
  Staged for librefang#3378 part 2 — the comment marks them so a future
  dead-code sweep doesn't remove them before the kernel-side
  migration lands.
neo-wanderer pushed a commit that referenced this pull request May 5, 2026
…ibrefang#4600)

* feat(plugins): Ed25519 signing across workers + daemon TOFU resolver (librefang#3805)

Worker side
- registry-worker: sign canonical index.json on cron refresh; new endpoints
  /api/registry/index.json, /api/registry/index.json.sig,
  /.well-known/registry-pubkey. Backward-compatible: when REGISTRY_PRIVATE_KEY
  is unset the cron skips signing and the new endpoints return 503.
- marketplace-worker: sign per-version metadata on publish over the canonical
  string \`<slug>@<version>|<bundle_url>|<bundle_sha256>\`; new endpoints
  /v1/pubkey and /v1/download/<slug>/<version>/signature. Adds bundle_sig
  column to package_versions (NULL when the secret is unset).
- web/workers/keygen.mjs: standalone Ed25519 keypair generator. Output format
  (PKCS#8 private + raw 32-byte public) verified compatible end-to-end with
  ed25519_dalek::VerifyingKey::from_bytes.

Daemon side
- Replace the all-zero OFFICIAL_REGISTRY_PUBKEY_B64 placeholder + hard-fail
  with resolve_registry_pubkey(): env LIBREFANG_REGISTRY_PUBKEY > TOFU cache
  ~/.librefang/registry.pub > HTTP fetch from LIBREFANG_REGISTRY_PUBKEY_URL
  (default https://librefang.ai/.well-known/registry-pubkey).
- install_from_registry now downgrades Ed25519 verification to a warning when
  SHA-256 has already verified the manifest, hard-failing only when neither
  integrity check is available — fixes the case where every install attempt
  failed because no real registry pubkey was deployed yet.
- fetch_verified_index keeps the hard-fail (no per-index checksum fallback).
- Drops the three librefang#3799 placeholder regression tests; replaces with three
  resolver tests covering the validator and cache path.

Docs
- web/workers/SIGNING.md: operator runbook (keygen, deploy, rotation).
- docs/architecture/plugin-signing.md: trust model + layered defenses.

Drive-by
- Fix pre-existing clippy doc_lazy_continuation in librefang-types config
  blocking workspace clippy.

* chore(deps): re-sync Cargo.lock with librefang-testing Cargo.toml

cargo check refreshes the librefang-memory dep entry that an upstream
commit left out of Cargo.lock. Trivial single-line drive-by while in this
PR's worktree.

* chore(workers): wire real REGISTRY_PUBLIC_KEY into wrangler.toml

Generated via web/workers/keygen.mjs; private half deployed as Wrangler
secret on both librefang-registry and librefang-marketplace workers.
Public key is non-secret — committed in cleartext per design.

* chore(workers): correct REGISTRY_PUBLIC_KEY to match deployed secret

Earlier commit wired julGSb...; the live Cloudflare secret has since
been rotated. Restore PR ↔ deployed-secret consistency.

* chore(workers): finalise REGISTRY_PUBLIC_KEY (matches Bitwarden-backed secret)

Replaces the prior placeholder/intermediate keys with the keypair backed
up in Bitwarden and live as REGISTRY_PRIVATE_KEY on both workers.

* feat(web): serve plugin registry pubkey at /.well-known/registry-pubkey

The daemon's TOFU resolver (resolve_registry_pubkey) defaults to
fetching https://librefang.ai/.well-known/registry-pubkey. Without this
short-circuit the Pages SPA fallback returns index.html, which fails
base64 validation, which makes plugin install hard-fail in
fetch_verified_index.

Pubkey is non-secret. Mirror of REGISTRY_PUBLIC_KEY in the two
worker wrangler.toml files; rotation must update all three in
lockstep.

* feat(plugins): wire daemon to worker-signed registry index mirror

The worker-side signing endpoints landed previously, but the daemon was
still fetching the registry index from raw.githubusercontent.com — and
the official registry repo has no committed `index.json` at all, so
`install_plugin_with_deps` either 404'd or skipped verification entirely.

Connect the chain end-to-end:

* registry-worker now extracts `needs` and `version` from each plugin
  TOML during the cron refresh, and stores a daemon-shaped flat array
  (`[{name, version?, description?, needs?}]`, sorted by name for byte
  determinism) at `kv_store('plugins_index')`. The Ed25519 signature
  covers exactly those bytes and is stored at `plugins_index_sig`.
  `/api/registry/index.json[.sig]` now serve these — the dashboard's
  dict-shaped `/api/registry` is unchanged. The cron's signature-based
  short-circuit also bails out when the new KV row is missing so the
  first deploy after this change actually populates it.

* daemon's `fetch_verified_index` defaults to the worker mirror
  (`https://stats.librefang.ai/api/registry/index.json`) when the
  registry is the official repo. Self-hosted forks keep the GitHub raw
  fallback. Both pairs accept `LIBREFANG_REGISTRY_INDEX_URL` /
  `LIBREFANG_REGISTRY_INDEX_SIG_URL` overrides for air-gapped
  deployments. URL selection is pulled into `registry_index_urls`
  with three new unit tests covering official / fork / env-override.

Result: once the worker is redeployed and the cron has run once,
`librefang plugin install <name>` for plugins with `[[needs]]` will
verify the index against the published Ed25519 key — no longer relying
on HTTPS + GitHub commits as the trust anchor.

* fix(workers): lazy-build plugins_index from registry_data on first hit

The signed-plugins-index endpoint was 503'ing on Workers Free because
the new `kv_store('plugins_index')` row only gets populated by the
cron path (02:00 UTC) — and forcing a refresh from a request handler
overshoots the 50-subrequest budget for the GitHub manifest fetch.

Pull the build path that's safe under the request budget into a small
`ensurePluginsIndex` helper called from the two daemon-facing
endpoints. It derives the flat `[{name, version?, description?,
needs?}]` array from the existing `registry_data` KV row, sorts by
name (matches the cron path's byte ordering so the signature contract
stays consistent), signs it, and persists both the bytes and the
signature. After that, the cron-path code remains the source of
truth — `ensurePluginsIndex` is a one-shot bootstrap that no-ops once
the row exists.

Also drop the `?refresh=1`-bypasses-staleness shortcut from the
previous commit: same 50-subrequest ceiling makes it useless from
fetch handlers, and the cron already has the higher quota it needs.

* fix(plugins): default OFFICIAL_PUBKEY_URL to the worker subdomain

The Pages-worker pubkey route at librefang.ai/.well-known/registry-pubkey
only goes live after the next main-branch deploy of web/public/_worker.js.
Until then, requests fall through to the SPA index.html — which
is_valid_registry_pubkey_b64 correctly rejects, but only after taking the
network round trip and erroring out.

Point the daemon directly at the worker's own subdomain
(stats.librefang.ai/.well-known/registry-pubkey), which is the canonical
source anyway and is updated in lockstep with the signed index served at
the same host. The Pages-worker alias remains a stable fallback that
operators can opt into via LIBREFANG_REGISTRY_PUBKEY_URL.

* feat(workers): add /api/registry/refresh forced-rebuild endpoint

Lets the librefang-registry GitHub Action push fresh content to the
worker's D1 cache + signed plugins index immediately on every push to
main, instead of waiting up to ~24h for the 02:00 UTC cron tick. Auth
is a constant-time bearer-token check against the REGISTRY_REFRESH_TOKEN
worker secret; until that secret is set the endpoint returns 503 and
remains inert (no probing surface).

Pre-emptively drops registry_data / plugins_index / plugins_index_sig
before re-running refreshRegistryCache so a no-op commit (e.g. README
typo) still forces a real rebuild — the existing signature-equality
short-circuit would otherwise skip the new path entirely. Also purges
the Cache-API row so dashboard reads see the new bytes immediately.

* refactor(workers): forced refresh fetches in-repo plugins-index.json

Pair with librefang/librefang-registry@f9821d5 — the daemon-shaped
flat plugins index is now built inside the registry repo by
scripts/build-plugins-index.mjs and committed as plugins-index.json
at the repo root.

handleForcedRefresh now does a single GitHub raw fetch for that one
file, validates the JSON shape, and signs+stores it. Constant-cost
refresh regardless of registry size keeps the path under the Workers
Free 50-subrequest budget that the previous walking-Contents-API
approach blew through.

The dict-shaped /api/registry payload (dashboard) and its 02:00 UTC
cron rebuild are unchanged — that lane stays on the slower path
because dashboard tolerance for staleness is much higher.

* feat(workers): forced refresh now ingests both daemon + dashboard indexes

Pair with librefang/librefang-registry@ff6f3f2 — registry-index.json
joins plugins-index.json as a checked-in artefact built by
scripts/build-registry-index.mjs.

handleForcedRefresh fetches both files in parallel (2 subrequests
total, constant in registry size) and writes:
  plugins_index     ← plugins-index.json (signed with Ed25519)
  registry_data     ← registry-index.json (dict-shaped, dashboard)
Plus purges the Cache-API entry that backs /api/registry so the next
dashboard hit reads fresh bytes immediately instead of the 1h-cached
previous payload.

Result: dashboard updates land within seconds of a registry push, no
longer waiting up to ~24h for the 02:00 UTC cron tick. The cron path
is kept as a backstop for cases where the GitHub Action cannot
reach the worker (network blips, CI outages).

* fix(plugins): address PR review CRITICAL/HIGH security defects (1/3)

Daemon-side fixes for the issues raised in code review of PR librefang#4600:

* CRITICAL #3 / MEDIUM #12 — install_from_registry was calling
  verify_archive_signature against {listing_url}.sig where listing_url
  is a GitHub Contents API URL. That .sig file never exists in the
  official registry layout, so the function silently returned Ok(()) on
  every install — meaning the entire Ed25519 archive lane was dead code
  that always passed. Replaced with index-membership check: refuse to
  install plugins not present in the signed plugins-index. The unused
  function is removed; the trust path now flows through the worker-
  signed flat index instead of a per-plugin .sig that never existed.

* HIGH #5 / #16 — TOFU pubkey resolver was vulnerable to first-install
  MITM (cafe wifi, hostile DNS, subdomain takeover) silently pinning an
  attacker key forever. Added EMBEDDED_REGISTRY_PUBKEY compiled into
  the daemon binary as the primary trust root for the official registry.
  Resolution chain is now: env override > embedded > TOFU/HTTP (the
  latter two only consulted for self-hosted forks that opt in).

* HIGH #6 — fetch_verified_index treated a missing or unreachable
  index.json.sig as a soft warning even when pubkey resolution was
  required. An attacker who could serve a doctored index but suppress
  the .sig (404 or network error) bypassed verification entirely. Now
  hard-fails for the official mirror; self-hosted forks keep the soft
  path so they can adopt signing incrementally.

* MEDIUM #13 — TOFU cache file open was vulnerable to symlink attacks
  from compromised post-install hooks. Added O_NOFOLLOW + regular-file
  check on read, mode 0600 + O_NOFOLLOW on write (Unix). Windows path
  validates regular-file status and relies on NTFS ACLs.

* MEDIUM #15 — added scripts/check-pubkey-lockstep.sh that fails when
  the pubkey constant drifts between the daemon's EMBEDDED_REGISTRY_PUBKEY
  and the three worker locations (registry-worker wrangler.toml,
  marketplace-worker wrangler.toml, _worker.js). Wire into CI before
  any cargo / wrangler build to catch silent rotation footguns.

clippy + plugin_manager tests pass.

* fix(plugins): address PR review CRITICAL/HIGH defects (2/3)

Worker-side fixes paired with the daemon hardening in 1/3 and the
in-repo signing in librefang-registry@74745f1.

CRITICAL #1 — registry-worker is no longer a sign-anything oracle.
handleForcedRefresh now fetches plugins-index.json + .sig from raw
(committed by the registry repo's CI, which holds the private key)
and stores both verbatim. The worker carries no private key and
performs no signing — REGISTRY_REFRESH_TOKEN can no longer be used
to coerce the worker into signing attacker-supplied bytes.

CRITICAL #4 — ensurePluginsIndex deleted entirely. The lazy-build
path produced bytes that were not byte-identical to what
refreshRegistryCache produced (different empty-string handling),
so any signature built by cron over bytes A and served as bytes B
from lazy-build would fail daemon verification.

refreshRegistryCache (~165-line Contents-API walker) deleted too —
it had been silently dropping half the plugins under Workers Free's
50-subrequest budget. Cron now re-runs the same forced-refresh path
(3 subrequests) as a daily backstop.

signWithRegistryKey + b64FromBytes + bytesFromB64 deleted — no
remaining caller. -169 net lines on the registry worker.

CRITICAL #2 — marketplace-worker handlePublishVersion now refuses
bundle_url not on an approved CDN prefix. Without this, an author
could publish bundle_url=https://attacker.example/payload.tgz with
a valid sha256 and get a registry-signed signature back. Also
tightens bundle_sha256 to ^[0-9a-f]{64}$.

MEDIUM #11 — constantTimeEqual padded to fixed length so timing
leaks at most "they are not the same", not "yours is shorter than
mine".

Key rotation: new keypair generated as part of decoupling the
worker from key custody. New pub:
  ClGa0Ucap8NdrKAy1rw9Tt6A9I8eg4zJ53+xIuKMuq0=
in lockstep across daemon EMBEDDED_REGISTRY_PUBKEY + 3 worker
locations. Private key now ONLY in librefang-registry's GitHub
Actions store. Old worker-side keys are unused but kept (rotation
one-way; deleting them is purely cleanup).

Cloudflare routing: stats.librefang.ai routes only /api/* to the
worker, so the daemon's HTTP rotation-probe path needed an
/api/registry/pubkey alias (the /.well-known/ form only resolves
on workers.dev). Daemon's OFFICIAL_PUBKEY_URL now points at the
routed alias.

29 plugin_manager tests pass; clippy clean; both worker JS files
node --check clean; end-to-end signature verifies against served
pubkey for all 11 plugins.

* fix(plugins): address PR re-review HIGH/MEDIUM/LOW defects

Round-2 review of PR librefang#4600 found 5 new HIGH defects introduced by the
round-1 fixes plus 2 MEDIUM and 1 LOW. Code-only follow-ups here;
the relocated CRITICAL oracle (workflow signing on push) is its own
follow-up commit.

HIGH-NEW-B — fetch_verified_index require_sig flag was keyed on
registry slug == OFFICIAL_REGISTRY_REPO. An attacker who could change
the configured registry (e.g. via a self-hosted-fork-style override)
flipped require_sig false; the daemon then verified against the
embedded official pubkey, fell through to the soft-warn branch when
verification failed, and silently accepted unsigned bytes. Now
require_sig is unconditional. Self-hosted forks must explicitly
opt out via LIBREFANG_REGISTRY_VERIFY=0; no implicit downgrade path.

HIGH-NEW-C — install_from_registry's index-membership check had an
"Err(e) if checksum_verified => warn" arm that downgraded on any
fetch error. Combined with the manifest-only SHA-256 check (which an
attacker who controls the GitHub repo can forge), DoS of
stats.librefang.ai bypassed verification entirely. Removed the
checksum-rescue arm; index-fetch errors now hard-fail after a single
500ms-backoff retry to absorb transient blips. checksum_verified
binding is gone (the SHA-256 step still runs and still hard-errors
on mismatch, but doesn't gate any other check).

HIGH-NEW-D — marketplace bundle_url allowlist matched on the raw
input string post-URL.parse, so WHATWG normalization tricks
(`https://github.com/../../attacker/x`) bypassed it. Reworked to
match on (parsed.host, parsed.pathname) tuples and tightened the
GitHub allowance to /releases/download/ only — the previous prefix
also matched .../raw/main/... which is a mutable branch HEAD and
defeats the immutability rationale. Also rejects URLs with hash
fragments.

HIGH-NEW-E — check-pubkey-lockstep.sh regex was loose enough to
match a renamed LEGACY_REGISTRY_PUBLIC_KEY or a base64-shaped
fragment in a comment. Anchored to start-of-line, length-limited
the captured value to exactly 44 chars (Ed25519 raw 32-byte b64),
forbade other quoted strings between the name and the value.

MEDIUM-NEW-F — plugins-index.json.sig length check was `>= 86`,
admitting arbitrarily long garbage. Tightened to exactly 86 or 88
chars and stricter regex.

MEDIUM-NEW-G — Cache-API purge failure was silently swallowed by
try/catch (_), so the worker reported success while the dashboard
served stale bytes for up to 1h. Now surfaces cache_purged: bool
(plus cache_purge_error when present) in the JSON response.

LOW — EMBEDDED_REGISTRY_PUBKEY was a single &str, leaving no
acceptance window for daemon ↔ worker rotations to overlap.
Promoted to EMBEDDED_REGISTRY_PUBKEYS: &[&str]. Added
verify_registry_index_multi which tries the resolved key first
then falls back to other embedded slice entries (with a warn so
ops sees they're on the prior key). The lockstep script extracts
the active slot 0 only.

29 plugin_manager tests + clippy clean; both worker JS files
node --check clean.

* fix(plugins): expire prior embedded pubkeys per round-3 MEDIUM

Round-3 PR re-review noted that EMBEDDED_REGISTRY_PUBKEYS as &[&str]
left rotation-window keys accepted indefinitely — a future leak of a
retired private key would still be exploitable against any daemon
binary still carrying it.

Promoted slice element to a struct:
  EmbeddedPubkey { b64, expires_at: Option<i64> }

Slot 0 (active) keeps expires_at: None. Rotation procedure (per the
updated doc-comment): when shipping a new active key, move the prior
slot-0 entry to slot 1 with expires_at: Some(now + ~4 weeks). After
the deprecation window passes, drop the prior entry in a follow-up
release.

verify_registry_index_multi now skips embedded entries whose expiry
is in the past, with a debug log so ops can tell when daemons stop
falling back. Active key path is unchanged.

Lockstep script extended to handle the struct-field syntax (matches
the first `b64: "..."` in the file, which is slot 0). Same 44-char
length validation as before.

29 plugin_manager tests + clippy clean.

* chore(codegen): auto-regenerate openapi.json + sdk + schema baselines [skip ci]

* fix(plugins): close round-4 PR re-review follow-ups

Round-4 reviewer flagged 4 hardening items (all MEDIUM/LOW; PR was
already MERGEABLE per the verdict). Closing the three code-side ones
here; the branch-protection MEDIUM is documented as an intentional
trade-off for the single-maintainer + auto-publish flow.

LOW round 4 — `EmbeddedPubkey.b64` field renamed to `pubkey_b64`. The
old generic name made the lockstep regex's "find any 44-char base64
in a `b64: \"...\"` field" rule fragile against unrelated future
fields with the same shape. The unique name is grep-anchored and
disambiguates intent. Lockstep script updated in lockstep.

LOW round 4 — added `embedded_pubkeys_slot0_has_no_expiry` test +
`debug_assert!` in resolve_registry_pubkey. A maintainer who absent-
mindedly sets `expires_at: Some(...)` on slot 0 during a rotation
edit would silently break installs the moment that timestamp passed.
The runtime assert catches it pre-ship; the test catches it in CI.

MEDIUM round 4 — `verify_registry_index_multi` dedup now compares
against `resolved_pubkey.trim()` rather than raw. All current call
sites pre-trim, but a future code path that forgets would otherwise
verify the same key twice (wasted CPU on one extra Ed25519 verify;
not unsafe). Cheap to harden, defensive against future drift.

MEDIUM round 4 — when slot-0 verification fails AND every prior
embedded pubkey is past expiry, the returned error now appends
"(N prior embedded pubkey(s) past expiry — this daemon binary is
past its rotation window; upgrade librefang to restore plugin
installs)". Gives ops an actionable next step instead of a bare
"Signature verification failed" mystery.

MEDIUM round 4 (branch protection) — kept as documented trade-off:
`bypass_pull_request_allowances.users:["houko"]` allows the single
registry maintainer to direct-push to main, defeating CODEOWNERS for
that one identity. `enforce_admins:false` similarly allows admin
direct-push. Removing the bypass would force the maintainer through
a self-approval PR cycle for every plugin commit — and the CI bot
(`github-actions[bot]`, identity `apps:[]` per the API since the
github-actions app slug isn't bypass-eligible) would also be blocked
from `git push`, breaking the auto-publish UX. Multi-maintainer
project would justify the strict stance; single-maintainer doesn't.

30 plugin_manager tests + clippy clean.

* style(plugins): apply cargo fmt to round-4 follow-up changes

The round-4 follow-up commit (9aca54a) shipped without running
`cargo fmt` locally — CI Quality job's `Check formatting` step
caught it. Reformatted plugin_manager.rs against the workspace
rustfmt config; no semantic change.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
neo-wanderer pushed a commit that referenced this pull request May 7, 2026
…ang#3566) (librefang#4726)

* refactor(kernel): scaffold KernelApi trait (librefang#3566 1/N)

Foundation for librefang#3566: define an empty 'pub trait KernelApi: Send + Sync'
in librefang-kernel and an 'impl KernelApi for LibreFangKernel {}'.
The trait is the single explicit contract between the API layer and the
kernel — subsequent commits widen it method-by-method until AppState's
'pub kernel: Arc<LibreFangKernel>' can be replaced with
'pub kernel: Arc<dyn KernelApi>'.

This is the API-side counterpart to the runtime-side 'KernelHandle'
role traits introduced in librefang#3746 and widened in librefang#3744.

* refactor(kernel): widen KernelApi trait to cover route-needed methods (librefang#3566 2/N)

Adds ~110 method signatures to KernelApi covering subsystem accessors
(audit, agent_registry, approvals, event_bus, hands, memory, MCP,
templates, supervisor, …), '_ref' accessors that expose internal locks,
config / lifecycle hooks (reload_config, validate_config_for_reload,
shutdown), vault, inbox, auto-dream, full agent lifecycle (spawn / kill /
suspend / resume / compact / sessions / manifest / skills / mcp servers /
tool filters), messaging (send_message, send_message_ephemeral), hands
(activate / deactivate / pause / resume / reload / persist), MCP
connection ops (connect / disconnect / retry / reload / reconnect),
triggers + workflows + events, agent bindings, model-catalog access, and
'start_background_for_agent'.

The 'Arc<Self>'-receiver methods (connect_mcp_servers, retry_mcp_connection,
reload_mcp_servers, reconnect_mcp_server, start_background_for_agent) take
'self: Arc<Self>' on the trait so 'Arc<dyn KernelApi>' can dispatch to
them — call sites consume one strong ref per call (cloning the AppState
arc costs nothing).

The two generic-closure inherent methods are exposed as dyn-safe trait
methods:
  - update_budget_config takes '&dyn Fn(&mut BudgetConfig)'
  - model_catalog_update takes '&mut dyn FnMut(&mut ModelCatalog)' and
    returns (); callers needing the closure's return value capture it
    via '&mut Option<R>' from the surrounding scope (the AppState swap
    commit migrates the affected providers / registry / budget routes).

Bumps 'librefang-kernel' recursion_limit to 256 because the
'#[async_trait]' expansion nests pinned futures past the default 128
when the trait is this wide.

* refactor(api): swap AppState to Arc<dyn KernelApi> + finish trait coverage (librefang#3566 3/N)

Final structural change for librefang#3566: 'AppState.kernel' is now
'Arc<dyn KernelApi>'. Routes interact with the kernel exclusively
through the trait; the concrete 'LibreFangKernel' is no longer reachable
from 'AppState'.

Trait widening
  - 'KernelApi' now extends 'KernelHandle' (the runtime-facing
    super-trait that bundles every role trait via librefang#3746/librefang#3744). This
    lets 'Arc<dyn KernelApi>' upcast to 'Arc<dyn KernelHandle>' (and to
    any individual role trait) for the runtime-call paths the dashboard
    uses (channel send, prompt store, task queue, approval gate, …) —
    no separate trait object needed.
  - Adds the kernel-inherent surface routes/WS/server/channel_bridge
    actually use that the role traits don't cover: subsystem accessors
    (audit, agent_registry, approvals, hands, memory, MCP, templates,
    supervisor, browser, media, tts, web_tools, …), '_ref' accessors,
    config/lifecycle ops (reload_config, validate_config_for_reload,
    shutdown), full agent lifecycle (spawn/kill/suspend/resume/compact/
    sessions/manifest/skills/MCP servers/tool filters), messaging
    (send_message + handle / blocks / sender_context / incognito /
    streaming variants), hand ops, MCP connect/disconnect/retry/reload/
    reconnect (Arc<Self> receivers), triggers + workflows + events,
    bindings, model-catalog access, vault, auto-dream, a2a tasks/agents,
    embedding/tts/web/browser/media engines, and 'data_dir' /
    'install_peer_registry_for_test' / 'set_self_handle' for
    integration tests.

Naming conflicts with role-trait methods are resolved by suffixing the
typed-arg variants on KernelApi:
  - 'spawn_agent_typed(AgentManifest) -> AgentId' (vs.
    'AgentControl::spawn_agent(&str, Option<&str>) -> (String, String)')
  - 'kill_agent_typed(AgentId)' (vs. 'AgentControl::kill_agent(&str)')
  - 'run_workflow_typed(WorkflowId, String) -> (WorkflowRunId, String)'
    (vs. 'WorkflowRunner::run_workflow(&str, &str) -> (String, String)')
  - 'publish_typed_event(Event) -> Vec<TriggerMatch>' (vs.
    'EventBus::publish_event(&str, Value) -> Result<(), _>')
Routes/tests are migrated to the typed names.

Generic-closure inherent methods become dyn-safe trait methods:
  - 'update_budget_config(&dyn Fn(&mut BudgetConfig))' (callers wrap as
    '&|b| { ... }')
  - 'model_catalog_update(&mut dyn FnMut(&mut ModelCatalog))' returning
    '()'; the providers/registry/server callers that needed the
    closure's return value now capture it via '&mut Option<R>' from the
    surrounding scope.

Receiver mechanics: the seven Arc<Self>-receiver methods
('connect_mcp_servers', 'disconnect_mcp_server' [&self], 'retry_/reconnect_/
reload_mcp_servers', 'send_message_streaming_with_*',
'start_background_for_agent', 'spawn_key_validation',
'auto_dream_trigger_manual', 'probe_local_provider', 'set_self_handle')
take 'self: Arc<Self>' on the trait. Call sites consume one strong ref
per call ('state.kernel.clone().method(...)') — Arc clones are cheap.

channel_bridge / server.rs migration:
  - 'KernelBridgeAdapter' / 'start_channel_bridge' /
    'start_channel_bridge_with_config' now take 'Arc<dyn KernelApi>'.
  - 'server::build_router' still takes 'Arc<LibreFangKernel>' (the
    daemon-side caller has the concrete kernel); it implicitly coerces
    to 'Arc<dyn KernelApi>' at the AppState struct literal.
  - 'as Arc<dyn KernelHandle>' explicit casts at 7 call sites are
    replaced by implicit upcast via the type annotation
    ('let kh: Arc<dyn KernelHandle> = state.kernel.clone();') — works
    because of the new supertrait.

Recursion limit on librefang-kernel is bumped to 256 ('lib.rs') because
the '#[async_trait]' expansion across this many methods pushes the
default 128-step layout pass over the edge.

Verification
  cargo check --workspace --all-targets  passes locally
  (next commits: clippy zero-warnings, then 'cargo test -p librefang-api').

* refactor(kernel): allow too_many_arguments on KernelApi (librefang#3566 4/N)

'register_trigger_with_target' (8 args) and 'send_message_with_incognito'
(8 args) mirror the kernel-inherent signatures verbatim. Splitting the
trait method into a builder would diverge the trait surface from the
inherent surface and break the 'trait is a thin facade' invariant. The
inherent kernel methods are themselves over the threshold; clippy is
silenced at the file level rather than per-method to keep the trait body
uncluttered.

* refactor(arch): self-review follow-ups for librefang#3566 (KernelApi widening)

Addresses every concrete item from the self-review pass:

P2 #2 — `build_router` now takes `Arc<dyn KernelApi>` instead of
`Arc<LibreFangKernel>`. The "stub the kernel for tests" goal of librefang#3566
required this final step; embedders that previously held the concrete
type get implicit upcasting via Rust's coercion. `set_self_handle`'s
`self: Arc<Self>` receiver forces a `kernel.clone().set_self_handle()`
at the boot site so the surrounding scope keeps its handle for
`start_background_agents` and friends.

P2 #3 — Removed dead `as_dyn` helper. With `build_router` accepting
`Arc<dyn KernelApi>` directly, callers either upcast at construction
(implicit coerce on `Arc::new(LibreFangKernel { .. })`) or use
`as Arc<dyn KernelApi>` explicitly; the helper was never wired.

P2 #4 — Confirmed by-suite test runs for the 5 highest-risk integration
suites the PR body's run skipped:
  - mcp_oauth_flow_test                        — 7 pass
  - auth_public_allowlist                      — 9 pass
  - pairing_test                               — 8 pass
  - tools_invoke_test                          — 8 pass
  - users_test                                 — 27 pass
mcp_oauth_flow specifically exercises the `Arc<Self>` MCP retry/reconnect
surface that the trait migration touched; clean.

P3 #1 — Module-level `#![allow(clippy::too_many_arguments)]` collapsed
to per-method `#[allow]` on the only two methods that need it
(`register_trigger_with_target` + `send_message_with_incognito`,
both in trait def and `LibreFangKernel` impl). Unrelated methods
that creep over 7 args will trip the lint instead of being silently
masked.

P3 #2 — Inherent `LibreFangKernel::run_workflow` now carries a
docstring pointing at the typed trait method
`KernelApi::run_workflow_typed` so readers crossing the seam don't
mistake the role-trait `String`-shape variant for the typed path.

Note on P2 #1 (RCU retry safety in `routes/providers.rs`): re-read
of the inherent `model_catalog_update` confirms each rcu attempt
clones `(**cat).clone()` into a fresh `next` and runs the closure
on that fork — closures never observe their own previous-attempt
writes. The two flagged callers (`set_model_overrides`,
`delete_custom_model`) are retry-safe; no change needed.

Verification

- `cargo check -p librefang-api -p librefang-kernel --all-targets`
  on host (docker image lacks libdbus / gdk for sibling crates;
  CI Linux runner covers them)
- 5 by-suite tests above
neo-wanderer pushed a commit that referenced this pull request May 7, 2026
…ng#3744 phases 1-3) (librefang#4713)

* refactor(kernel): extract 16 kernel_handle trait impls into kernel::handles

Phase 1 of the kernel/mod.rs split. Move every `impl kernel_handle::* for
LibreFangKernel` block out of the 20K-line god-file into per-trait files
under `kernel/handles/`. The submodules are descendants of `kernel`, so
they retain access to LibreFangKernel's private fields and inherent
methods without any visibility surgery.

Mechanical, behavior-preserving move — no method bodies were touched.
mod.rs drops from ~20.6k to ~18.8k lines (-2073). Three imports that
became unused after the move were removed (kernel_handle self-alias,
ToolApprovalSubmission, std::str::FromStr) and re-imported locally in
the consuming handle files.

Traits moved:
  AgentControl, MemoryAccess, TaskQueue, EventBus, KnowledgeGraph,
  CronControl, HandsControl, ApprovalGate, A2ARegistry, ChannelSender,
  PromptStore, WorkflowRunner, GoalControl, ToolPolicy, ApiAuth,
  SessionWriter

Verified: cargo check -p librefang-kernel --lib + cargo clippy
-p librefang-kernel --lib -- -D warnings both green.

* refactor(kernel): phase 2 — extract free-fn / cron-bridge / probe helpers

Move the cohesive bottom-of-file helpers out of kernel/mod.rs into
thematic sub-modules of `kernel`:

  mcp_summary.rs       – mcp_summary_cache_key, render_mcp_summary
  reviewer_sanitize.rs – sanitize_reviewer_line, sanitize_reviewer_block
  cron_script.rs       – cron_script_wake_gate, atomic_write_toml,
                         parse_wake_gate
  cron_bridge.rs       – `impl CronChannelSender for KernelCronBridge`,
                         CRON_EMPTY_OUTPUT_HEARTBEAT, cron_fan_out_targets,
                         cron_deliver_response
  provider_probe.rs    – probe_and_update_local_provider,
                         probe_all_local_providers_once

Mechanical, behavior-preserving move. mod.rs drops by ~636 lines (now
~18.2k). The struct `KernelCronBridge` itself stays in mod.rs; only its
trait impl moves. Free fns formerly visible only to mod.rs are now
`pub(super)` so the (still-resident) `LibreFangKernel` methods that call
them keep compiling. mod.rs adds a `use cron_bridge::{cron_deliver_response,
cron_fan_out_targets};` re-export so existing call sites in mod.rs and
`kernel::tests` resolve byte-for-byte.

Also restores `kernel_handle::self` alongside the prelude wildcard
import — phase 1 dropped the self alias as "newly unused", missing the
fact that `kernel::tests` uses it via `kernel_handle::ApprovalGate::...`.
Without this, `cargo test -p librefang-kernel --lib --no-run` fails.

* refactor(kernel): phase 3a — extract accessor / lifecycle impl block to kernel::accessors

Move the first `impl LibreFangKernel { ... }` block (formerly mod.rs
lines 1106..2598, ~1500 lines, ~85 methods — accessors like `home_dir`,
`config_ref`, `cron`, `pairing_ref`, plus operational helpers like
`spawn_key_validation`, `spawn_approval_sweep_task`, `record`,
`auto_dream_*`, etc.) into `crates/librefang-kernel/src/kernel/accessors.rs`.

Sibling submodule of `kernel::mod`, so retains private-field/method
access into `LibreFangKernel` without any visibility surgery (same
pattern as the Phase 1 trait-impl moves).

Mechanical, behavior-preserving move — method bodies untouched.
mod.rs drops from ~18.2k to ~16.7k lines (-1500). 4 inherent
`impl LibreFangKernel` blocks remain in mod.rs (down from 5);
phase 3b/3c will continue chipping.

Verified: cargo check / clippy -- -D warnings / cargo check --tests
/ rustfmt --check all green.

* refactor(kernel): phase 3b — extract cron tick loop closure to kernel::cron_tick

The cron scheduler tick loop was historically the longest closure in
mod.rs (~528 lines, the landing zone for librefang#4683 et al.). Lift the body
out of `spawn_logged("cron_scheduler", async move { … })` into a free
`pub(super) async fn run_cron_scheduler_loop(kernel: Arc<LibreFangKernel>)`
in `crates/librefang-kernel/src/kernel/cron_tick.rs`.

Behaviour-preserving — body moved byte-for-byte; only the outer wrapper
changed (closure → free fn). Captured state was just `kernel`, which
becomes the single fn parameter; per-tick `cron_sem` and per-job clones
are still constructed inside the loop body, unchanged.

mod.rs drops by ~528 lines (now ~16.2k). Three Phase 2 re-exports
(`cron_deliver_response`, `cron_fan_out_targets`, `cron_script_wake_gate`)
that mod.rs introduced for the inline closure are no longer needed —
the only call sites moved with the loop — so they're gone from mod.rs's
`use` block.

Verified: cargo check / clippy -- -D warnings / cargo check --tests
all green.

* fix(kernel): repair phase 1 regressions on prompt store / wiki / cron compaction

The Phase 1 mechanical-move subagent worked off a state that pre-dated
four recent main commits, silently reverting their changes during the
trait-impl extraction:

- librefang#4685 (prompt store r2d2 pool): mod.rs called `memory.usage_conn()` and
  passed 1 arg to `PromptStore::new_with_path`. Both APIs had been replaced
  with `memory.pool()` / 2-arg `new_with_path(&db_path, pool_size)`. Restored.
- librefang#3329 (memory_wiki vault): `kernel_handle::WikiAccess` is a `KernelHandle`
  super-trait but the `wiki_vault` field is not yet on `LibreFangKernel` in
  this branch. Replace the broken impl with an empty `impl WikiAccess for
  LibreFangKernel {}` so every method falls through to the trait default
  (`KernelOpError::unavailable("wiki_*")`) until the rebase-on-main work
  pulls in the full vault wiring.
- librefang#4683 (cron summarize-trim compaction): `kernel::tests` references
  `cron_compute_keep_count` / `cron_clamp_keep_recent` /
  `cron_resolve_compaction_mode` / `try_summarize_trim` that were dropped
  from mod.rs. Cherry-pick the four helpers into a new
  `kernel::cron_compaction` sub-module + re-export from mod.rs so tests
  resolve. The matching cron-tick body change still needs to be ported
  into `kernel::cron_tick` — that's a separate item.

Verified: cargo check -p librefang-kernel --lib + cargo clippy
-p librefang-kernel --lib -- -D warnings + cargo check -p librefang-kernel
--lib --tests + rustfmt --check all green.

Note for reviewers: this PR's branch is "behind 4" of origin/main. The
proper fix is a rebase, which will involve genuine merge work in the
giant impl block that the refactor sliced. This commit is the minimum
to keep HEAD compiling against the current workspace deps; it does NOT
restore librefang#4683's runtime changes (the SummarizeTrim path inside the cron
tick body) — only the helpers + their tests.

* fix(kernel): port librefang#4683 cron-tick SummarizeTrim path into kernel::cron_tick

Phase 3b's `cron_tick.rs` body was the pre-librefang#4683 closure (the version
my Phase 1 subagent saw); the SummarizeTrim runtime path was lost when
the Phase 1 work happened to omit librefang#4683's mod.rs hunk. The earlier
repair commit restored the helper fns into `kernel::cron_compaction`
but left their call site missing.

Replace the body of `run_cron_scheduler_loop` with the post-librefang#4683
version copied from origin/main's mod.rs (the `spawn_logged("cron_scheduler",
async move { … })` block at lines 14386..15064). Now the cron tick
body actually calls:

  - `cron_compute_keep_count` to size the keep window without mutating
  - `cron_resolve_compaction_mode` to fall back from SummarizeTrim →
    Prune when keep_count < 2
  - `cron_clamp_keep_recent` to enforce `[summary] + tail` cap
  - `try_summarize_trim` (aux LLM compaction) on the SummarizeTrim path
  - `apply_cron_prune` for both the configured Prune path and the
    SummarizeTrim → Prune fallback

`apply_cron_prune` was bumped to `pub(super)` so cron_tick.rs (sibling
submodule of cron_compaction) can call it.

Verified: cargo check / clippy -- -D warnings / cargo check --tests /
rustfmt --check all green.

* refactor(kernel): phase 3c — extract 5 thematic clusters from giant impl

Slice the still-resident giant `impl LibreFangKernel { ... }` body into
five cohesive sub-modules under `kernel/`:

  messaging.rs         — 26 send_message* / send_message_streaming_*
                         variants + send_message_full /
                         send_message_full_with_upstream +
                         resolve_agent_home_channel +
                         run_forked_agent_streaming
                         (~2 670 lines)
  assistant_routing.rs — 12 helpers driving the @-mention / specialist
                         routing path: notify_owner_bg,
                         llm_classify_intent, resolve_or_spawn_specialist,
                         send_message_streaming_resolved,
                         resolve_assistant_target,
                         route_assistant_by_metadata, etc.
                         (~460 lines)
  hands_lifecycle.rs   — 14 hand-management methods: activate_hand,
                         deactivate_hand, reload_hands,
                         apply_hand_agent_runtime_override_to_registry,
                         resolve_hand_agent_model_defaults, etc.
                         (~990 lines)
  mcp_setup.rs         — 6 MCP wiring fns: connect_mcp_servers,
                         reload_mcp_servers, retry_mcp_connection,
                         reconnect_mcp_server, run_mcp_health_loop,
                         disconnect_mcp_server (~610 lines)
  prompt_context.rs    — 7 cached-summary builders for prompt
                         assembly: cached_workspace_metadata,
                         cached_skill_metadata, active_goals_for_prompt,
                         build_skill_summary_from_skills,
                         build_mcp_summary, collect_prompt_context, etc.
                         (~340 lines)

Mechanical, behavior-preserving move — method bodies untouched.
mod.rs drops from ~16.2k to ~11.3k lines (-4 950). Combined with
phases 1+2+3a+3b that's a 9 350-line reduction (-45%) on the
original 20 609-line god-file.

Visibility surgery (only what the compiler demanded; private inherent
methods are not visible across sibling submodules without it):

  - messaging::send_message_full / send_message_streaming_with_sender_and_session
    bumped to pub(crate) — called from cron_tick + assistant_routing.
  - assistant_routing::{notify_owner_bg, send_message_streaming_resolved,
    resolve_assistant_target, assistant_route_key,
    should_skip_intent_classification} bumped to pub(crate) — called
    from messaging + tests.
  - mcp_setup::run_mcp_health_loop bumped to pub(crate) — kept-resident
    spawn_logged callsite in mod.rs needs it.
  - prompt_context::{cached_workspace_metadata, cached_skill_metadata,
    active_goals_for_prompt, build_mcp_summary} bumped to pub(crate)
    for cross-module consumers.
  - mod.rs structs CachedWorkspaceMetadata + CachedSkillMetadata
    bumped to pub(crate) struct (private fields kept) so the
    pub(crate) accessor signatures don't trip the private-interfaces
    lint.

Verified across all 5 cluster checkpoints + final pass: cargo check
-p librefang-kernel --lib + clippy -- -D warnings + check --tests +
rustfmt --check all green.

* refactor(kernel): phase 3d — extract 5 more clusters from giant impl

Continue slicing the still-resident `impl LibreFangKernel { ... }` body
into thematic sub-modules:

  boot.rs            — session_stream_hub, boot, boot_with_config
                       (~2 005 lines — `boot_with_config` is the
                       single largest method extracted so far)
  spawn.rs           — spawn_agent + 4 spawn_agent_with_*  +
                       spawn_agent_inner + verify_signed_manifest +
                       trusted_manifest_signer_keys (~410 lines)
  agent_execution.rs — execute_wasm_agent, execute_python_agent,
                       should_reuse_cached_route, is_brief_acknowledgement,
                       execute_llm_agent (~1 195 lines)
  session_ops.rs     — inject_message + inject_message_for_session +
                       setup/teardown_injection_channel + resolve_module_path
                       + reset/reboot_session + clear_agent_history +
                       list/create/switch/export/import_session +
                       inject_reset_prompt + evaluate_condition +
                       save_session_summary (~820 lines)
  agent_state.rs     — persist_manifest_to_disk, set_agent_model,
                       reload_agent_from_disk, update_manifest,
                       set_agent_skills, set_agent_mcp_servers,
                       set_agent_tool_filters (~530 lines)

Mechanical move — method bodies untouched. mod.rs drops from
~11.3k to ~6.4k lines (-4 870). Cumulative across phases 1+2+3a+3b+3c+3d
the original 20 609-line god-file is now ~6 385 lines (-69%).

Visibility surgery (only what the compiler demanded for cross-submodule
call sites):

  - spawn::spawn_agent_inner          → pub(crate) (called by hands_lifecycle)
  - agent_execution::execute_wasm_agent / execute_python_agent /
    execute_llm_agent / should_reuse_cached_route → pub(crate)
    (called by messaging + assistant_routing)
  - session_ops::{setup_injection_channel, teardown_injection_channel,
    resolve_module_path, inject_reset_prompt, evaluate_condition}
    → pub(crate) (called by messaging / spawn / tests)

No struct or enum visibility bumps were needed — `boot.rs` builds
the kernel via struct literal because descendant submodules retain
private-field access.

Verified: cargo check / clippy -- -D warnings / cargo check --tests /
rustfmt --check all green at every cluster checkpoint and the final
pass.

* refactor(kernel): phase 3e/1 — extract agent_runtime cluster

Move runtime-control methods (cost/stop/list/suspend/resume/compact/
context_report/kill + watchers) from the giant `impl LibreFangKernel`
into `kernel/agent_runtime.rs`. mod.rs drops from 6 385 to 5 825
(-560).

Methods: session_usage_cost, stop_agent_run, stop_session_run,
list_running_sessions, agent_has_active_session, running_session_ids,
suspend_agent, resume_agent, persist_agent_enabled,
compact_agent_session, compact_agent_session_with_id, context_report,
register_agent_watcher, abort_agent_watchers, kill_agent,
kill_agent_with_purge.

Verified: cargo check / clippy -- -D warnings / cargo check --tests
all green.

* refactor(kernel): phase 3e/2-7 — extract 6 remaining clusters from giant impl

Continue slicing the still-resident `impl LibreFangKernel { ... }` body
into thematic submodules. After this commit the giant first impl block
is fully drained.

  bindings_and_handle.rs  — set_log_reloader, set_self_handle,
                            kernel_handle, list_bindings, add_binding,
                            remove_binding (~95 lines)
  config_reload_ops.rs    — reload_config + apply_hot_actions_inner
                            (~480 lines)
  triggers_and_workflow.rs — spawn_session_label_generation,
                             one_shot_llm_call, publish_event +
                             publish_event_inner, register_trigger +
                             register_trigger_with_target +
                             remove_trigger / set_trigger_enabled /
                             list_triggers / get_trigger / update_trigger,
                             register_workflow, run_workflow,
                             dry_run_workflow (~640 lines)
  background_lifecycle.rs — start_background_agents, start_ofp_node,
                            self_arc, start_heartbeat_monitor,
                            start_background_for_agent, shutdown
                            (~1 270 lines — the longest single
                            extraction in this PR)
  llm_drivers.rs          — lookup_provider_url, resolve_driver
                            (~250 lines)
  tools_and_skills.rs     — available_tools, reload_skills,
                            try_claim_skill_review_slot,
                            summarize_traces_for_review,
                            background_skill_review,
                            is_transient_review_error,
                            extract_json_from_llm_response,
                            context_engine_for_agent (~1 080 lines)

Mechanical move — method bodies untouched. mod.rs drops from
~5.8k to ~2.0k lines (-3 800). Combined with phases 1+2+3a+3b+3c+3d
the original 20 609-line god-file is now at ~2 000 lines (-90%).

Visibility surgery (only what the compiler demanded):

  - tools_and_skills::{context_engine_for_agent, try_claim_skill_review_slot,
    summarize_traces_for_review, background_skill_review,
    extract_json_from_llm_response, is_transient_review_error,
    MAX_INFLIGHT_SKILL_REVIEWS} → pub(crate) (cross-submodule + tests)
  - mod.rs `enum ReviewError` → pub(crate) enum (private-interfaces lint
    once tools_and_skills::background_skill_review's signature went
    pub(crate))

Plus a cosmetic doc-paragraph rewrap in `llm_drivers.rs` to satisfy
`clippy::doc_lazy_continuation`.

Verified: cargo check / clippy -- -D warnings / cargo check --tests
all green.

* fix(kernel): repair four behavioural regressions from mod split (librefang#4713 review)

The kernel/mod split landed type-clean but silently dropped four
behavioural hooks that `cargo check` / `clippy` could not see. All
four fail open / fail-permissive (rather than fail-closed), which is
why CI green doesn't catch them — they're only visible by exercising
the boot path or the wiki tools at runtime.

Restored to parity with `origin/main` (pre-split):

- `boot.rs`: re-add `config.tool_exec.validate()` after the
  workspace-wide `config.validate()` warning loop. Without it, a
  deployment with `[tool_exec] kind = "ssh"` or `"daytona"` but
  missing the matching subtable boots silently and fails on first
  tool call instead of aborting at boot.
- `boot.rs`: `MemorySubstrate::open_with_chunking(...)` →
  `open_with_pool_size(..., config.memory.pool_size)`. The 3-arg form
  forces the r2d2 default pool size of 8 regardless of operator
  config, so `[memory] pool_size` (just landed in librefang#4685) was a no-op
  for the primary substrate; only the prompt store init below this
  line still honoured the setting, leaving the two SQLite pools
  inconsistently sized.
- `spawn.rs::spawn_agent_inner`: re-add the
  `manifest.tool_exec_backend` override validation block after
  `validate_manifest_module_path`. Without it, an agent whose
  manifest pins `tool_exec_backend = "ssh"` against a daemon lacking
  `[tool_exec.ssh]` enters the registry and fails every tool
  invocation thereafter, instead of being rejected pre-registry.
- `mod.rs` + `boot.rs` + `handles/wiki_access.rs`: re-add the
  `wiki_vault: Option<Arc<librefang_memory_wiki::WikiVault>>` field on
  `LibreFangKernel`, the boot-time `WikiVault::new` initializer with
  the same fail-open warning behaviour as main, the
  `wiki_vault: wiki_vault.clone()` in the struct construction, and
  the concrete `wiki_get` / `wiki_search` / `wiki_write` method
  bodies (ported verbatim from the pre-split mod.rs). Without these
  three pieces, every `tool_wiki_*` call on a deployment with
  `[memory_wiki] enabled = true` flowed to the trait default and
  returned `KernelOpError::unavailable("wiki_*")`, silently disabling
  the just-landed librefang#3329 feature. Phase 1 had left an empty
  `impl WikiAccess for LibreFangKernel {}` with a header comment
  claiming \"will arrive with the librefang#3329 main-side merge\" — but
  librefang#3329 was already on `origin/main` (librefang#4712) at the time of the
  branch.

Verified locally:
  cargo check  -p librefang-kernel --lib                       OK
  cargo clippy -p librefang-kernel --lib -- -D warnings        0 warnings
  rustfmt      --check --edition 2021 (4 modified files)       clean

Refs Codex review on commits 594aa42 / 0107d06 / 79eb683 of the
parent PR — findings #2-#5 (the four P2 items still open at HEAD)
addressed here. Codex P1 finding #1 (cron SummarizeTrim) was
addressed earlier in 0107d06 and is unaffected by this commit.

* fix(kernel): repair phase 3 regressions on reload_config + NO_REPLY allow-list (librefang#4713)

Phase 3e/3 moved Kernel::reload_config into kernel/config_reload_ops.rs
but the body that landed there was the pre-librefang#4664 tolerant version: it
called crate::config::load_config(...) (which silently returns
KernelConfig::default() on parse / migration / deserialize / include
failures) and wrapped validator errors as "Validation failed: ..."
without the "live config unchanged" reload-boundary pledge.

The strict-loader function crate::config::try_load_config and its 7
direct unit tests in config.rs are still present (orphaned), but the
4 api integration tests + 1 kernel integration test that drive
reload_config end-to-end all fail because the strict path was never
re-wired after the move. Restore parity with librefang#4664:

- swap load_config for try_load_config and propagate the Err through
  the same "Config reload failed; live config unchanged: {e}" wrapper
- wrap the validator branch with the same prefix so every
  reload-rejection path satisfies the "live config unchanged"
  invariant the integration helper relies on

Also: phase 3 split Kernel::cron_tick into a sibling kernel/cron_tick.rs
file that carries one inline comment mentioning the NO_REPLY literal.
The silent_response_single_source_of_truth grep-guard in
librefang-runtime allow-lists the literal in selected files; add
cron_tick.rs next to the existing mod.rs allow entry so the guard
stops flagging it.

Verified locally: kernel + api + runtime tests pass; clippy clean.
neo-wanderer pushed a commit that referenced this pull request May 7, 2026
…tems (librefang#3565) (librefang#4756)

* refactor(kernel): extract MeteringSubsystem from LibreFangKernel (librefang#3565 1/13)

Move audit_log + metering engine + budget_config off the LibreFangKernel
god struct into a new field-owning MeteringSubsystem under
kernel/subsystems/. Three slots become one. Method bodies stay on the
kernel and reach into the subsystem via self.metering.{audit_log,engine,budget_config}.

Refs librefang#3565.

* refactor(kernel): extract ProcessSubsystem from LibreFangKernel (librefang#3565 2/13)

Move process_manager + process_registry off LibreFangKernel into
ProcessSubsystem under kernel/subsystems/. Two slots become one;
references migrate from self.process_manager / self.process_registry
to self.processes.manager / self.processes.registry.

Refs librefang#3565.

* refactor(kernel): extract SecuritySubsystem from LibreFangKernel (librefang#3565 3/13)

Move auth + pairing + vault_cache + vault_recovery_codes_mutex off
LibreFangKernel into SecuritySubsystem under kernel/subsystems/. Four
slots become one. Field references migrate to self.security.{auth,pairing,
vault_cache,vault_recovery_codes_mutex}.

Refs librefang#3565.

* refactor(kernel): extract MediaSubsystem from LibreFangKernel (librefang#3565 4/13)

Move web_ctx + browser_ctx + media_engine + tts_engine + media_drivers
off LibreFangKernel into MediaSubsystem under kernel/subsystems/. Five
slots become one. References migrate to self.media.<field>.

Refs librefang#3565.

* refactor(kernel): extract LlmSubsystem from LibreFangKernel (librefang#3565 5/13)

Move default_driver + aux_client + embedding_driver + driver_cache +
model_catalog + default_model_override off LibreFangKernel into
LlmSubsystem under kernel/subsystems/. Six slots become one. References
migrate to self.llm.<field>.

Refs librefang#3565.

* refactor(kernel): extract WorkflowSubsystem from LibreFangKernel (librefang#3565 6/13)

Move workflow engine + template_registry + triggers + background +
cron_scheduler + command_queue off LibreFangKernel into
WorkflowSubsystem under kernel/subsystems/. Six slots become one. Inner
WorkflowEngine field renamed to engine to avoid the
self.workflows.workflows collision.

Refs librefang#3565.

* refactor(kernel): extract SkillsSubsystem from LibreFangKernel (librefang#3565 7/13)

Move skill_registry + hand_registry + skill_generation +
skill_review_cooldowns + skill_review_concurrency off LibreFangKernel
into SkillsSubsystem under kernel/subsystems/. Five slots become one.
References migrate to self.skills.<field>; the
CachedToolList.skill_generation field stays put because it's the
private cache-invalidation cursor, not the kernel one.

Refs librefang#3565.

* refactor(kernel): extract McpSubsystem from LibreFangKernel (librefang#3565 8/13)

Move mcp_connections + mcp_auth_states + mcp_oauth_provider + mcp_tools
+ mcp_summary_cache + mcp_catalog + mcp_health + effective_mcp_servers
+ mcp_generation off LibreFangKernel into McpSubsystem under
kernel/subsystems/. Nine slots become one. Inner field names keep their
mcp_ prefix so the migration is purely mechanical
(self.mcp_X to self.mcp.mcp_X).

Refs librefang#3565.

* refactor(kernel): extract MeshSubsystem from LibreFangKernel (librefang#3565 9/13)

Move a2a_task_store + a2a_external_agents + delivery_tracker + bindings
+ broadcast + peer_registry + peer_node + channel_adapters off
LibreFangKernel into MeshSubsystem under kernel/subsystems/. Eight
slots become one. References migrate to self.mesh.<field>.

Refs librefang#3565.

* refactor(kernel): extract GovernanceSubsystem from LibreFangKernel (librefang#3565 10/13)

Move approval_manager + hooks + external_hooks + approval_sweep_started
+ task_board_sweep_started off LibreFangKernel into GovernanceSubsystem
under kernel/subsystems/. Five slots become one. References migrate to
self.governance.<field>. session_stream_hub_gc_started stays put — it
will move with the session stream hub into EventSubsystem.

Refs librefang#3565.

* refactor(kernel): extract EventSubsystem from LibreFangKernel (librefang#3565 11/13)

Move event_bus + session_lifecycle_bus + session_stream_hub +
injection_senders + injection_receivers + assistant_routes +
route_divergence + session_stream_hub_gc_started off LibreFangKernel
into EventSubsystem under kernel/subsystems/. Eight slots become one.
References migrate to self.events.<field>. AssistantRouteTarget
visibility widened to pub(crate) so the subsystem can name it.

Refs librefang#3565.

* refactor(kernel): extract MemorySubsystem from LibreFangKernel (librefang#3565 12/13)

Move memory + wiki_vault + proactive_memory + proactive_memory_extractor
+ prompt_store off LibreFangKernel into MemorySubsystem under
kernel/subsystems/. Five slots become one. Inner MemorySubstrate field
renamed to substrate to avoid the self.memory.memory collision.

Refs librefang#3565.

* refactor(kernel): extract AgentSubsystem from LibreFangKernel (librefang#3565 13/13)

Move registry + agent_identities + capabilities + scheduler + supervisor
+ running_tasks + session_interrupts + agent_msg_locks + session_msg_locks
+ agent_concurrency + hand_runtime_override_locks + decision_traces +
agent_watchers off LibreFangKernel into AgentSubsystem under
kernel/subsystems/. Thirteen slots become one — the largest cluster.
References migrate to self.agents.<field>.

Final state: 13 subsystems own all of LibreFangKernel's previously-flat
field surface. The kernel struct itself is now a thin facade over
typed subsystem handles plus a small residual of cross-cutting state
(boot dirs, config, wasm sandbox, context engine, etc.).

Refs librefang#3565.

* refactor(kernel): migrate trivial accessor bodies into subsystems (librefang#3565 follow-up #1)

Each subsystem now owns its trivial accessor methods. The kernel
struct keeps the same public API but every accessor body becomes a
one-line delegate to the subsystem method that owns the underlying
field.

Subsystem methods added (all trivial, all tested via the existing
kernel-side delegates):

- AgentSubsystem: registry_ref, identities_ref, scheduler_ref,
  supervisor_ref, traces.
- EventSubsystem: event_bus_ref, lifecycle_bus, injection_senders_ref.
- GovernanceSubsystem: approvals, hook_registry.
- LlmSubsystem: catalog_swap, catalog_load, catalog_update,
  clear_driver_cache, embedding, default_model_override_ref.
- McpSubsystem: catalog_swap, catalog_load, health, connections_ref,
  auth_states_ref, oauth_provider_ref, tools_ref,
  effective_servers_ref.
- MediaSubsystem: web_tools, browser, engine, tts, drivers.
- MemorySubsystem: substrate_ref, proactive_store.
- MeshSubsystem: a2a_tasks, a2a_agents, channel_adapters_ref,
  bindings_ref, broadcast_ref, delivery, peer_registry_ref,
  peer_node_ref.
- MeteringSubsystem: audit_log, engine, current_budget, update_budget.
- ProcessSubsystem: manager, registry.
- SecuritySubsystem: auth_ref, pairing_ref.
- SkillsSubsystem: registry_ref, hand_registry_ref.
- WorkflowSubsystem: engine_ref, templates_ref, triggers_ref, cron_ref,
  command_queue_ref.

Non-trivial methods (vault_*, mcp_catalog_reload, spawn_*_sweep,
gc_sweep, etc.) keep their bodies on LibreFangKernel for now — those
involve cross-subsystem coordination and will move in a later follow-up
if they fit cleanly.

Refs librefang#3565.

* refactor(kernel): expose focused per-subsystem traits (librefang#3565 follow-up #2)

Each of the 13 subsystems now has its own focused trait —
AgentSubsystemApi, EventSubsystemApi, GovernanceSubsystemApi,
LlmSubsystemApi, McpSubsystemApi, MediaSubsystemApi,
MemorySubsystemApi, MeshSubsystemApi, MeteringSubsystemApi,
ProcessSubsystemApi, SecuritySubsystemApi, SkillsSubsystemApi,
WorkflowSubsystemApi.

Each trait pulls the trivial accessors that follow-up #1 added onto
the corresponding subsystem struct. Generic mutators (catalog_update,
update_budget) keep their inherent-method form because trait methods
cannot accept impl Fn / FnMut arguments without dyn boxing — and the
overhead would matter on the hot path.

Re-exports added in subsystems/mod.rs so external crates can name the
trait without referencing the per-subsystem submodule. accessors.rs
imports the bundle so the kernel-side delegate methods compile against
the trait surface rather than reaching into struct fields.

Refs librefang#3565.

* refactor(kernel): forward per-subsystem traits on LibreFangKernel (librefang#3565 follow-up #3)

Add a forwarding impl block per focused trait
(AgentSubsystemApi, EventSubsystemApi, GovernanceSubsystemApi, etc.)
for LibreFangKernel itself in a new kernel/subsystem_forwards.rs
module. Each method delegates to the matching subsystem instance
(self.agents.<method>(), self.metering.<method>(), …).

This lets new callers — and tests / mocks — bind against a focused
trait surface (Arc<dyn AgentSubsystemApi>, &dyn MeteringSubsystemApi)
without dragging in the entire 50+ method KernelApi trait. Existing
Arc<dyn KernelApi> consumers continue to work unchanged.

Together with follow-ups #1 and #2, this completes the issue's
'thin facade with subsystem handles, each owning its own state and
exposing a focused trait' goal:

- #1 made each subsystem own its trivial accessor bodies.
- #2 defined a focused per-subsystem trait and implemented it on the
  subsystem struct.
- #3 forwards those same traits onto LibreFangKernel itself, so the
  kernel can be used either through its fat KernelApi facade or
  through any subset of focused subsystem traits.

Refs librefang#3565.

* refactor(kernel): make focused-trait method names globally unambiguous on LibreFangKernel (librefang#3565 follow-up #4)

Code review on PR librefang#4756 flagged that several *SubsystemApi traits used
the same short method name, so once LibreFangKernel implements all 13
focused traits at once, kernel.catalog_swap() becomes ambiguous and
needs UFCS to disambiguate. The forwarding impls were already working
around this with McpSubsystemApi::catalog_swap(&self.mcp) etc., which
is a yellow flag, not a green one.

Rename trait methods so every kernel.<method>() call resolves to a
single trait without UFCS:

- LlmSubsystemApi::catalog_swap/load -> model_catalog_swap/load
- McpSubsystemApi::catalog_swap/load -> mcp_catalog_swap/load
- MeteringSubsystemApi::engine -> metering_engine
- MediaSubsystemApi::engine -> media_engine
- AgentSubsystemApi::registry_ref -> agent_registry_ref
- SkillsSubsystemApi::registry_ref -> skill_registry_ref
- ProcessSubsystemApi::manager/registry -> process_manager_ref/registry_ref

The new names also match the convention LibreFangKernel's existing
inherent accessors already follow (mcp_catalog, mcp_health,
event_bus_ref, session_lifecycle_bus, …), so binding through the
focused trait now reads exactly like binding through the kernel.

UFCS workarounds in subsystem_forwards.rs are dropped now that names
are unique. accessors.rs delegate calls updated to the new method
names. The kernel-public API (LibreFangKernel inherent methods like
audit(), metering_ref(), processes(), agent_registry()) is unchanged
— only the trait surface is renamed.

Refs librefang#3565.

* docs(kernel): preserve running_tasks rationale + document shutdown invariant (librefang#3565)

Two doc-only fixes after the subsystem extraction:

* Restore the `running_tasks` rationale comment (parallel `session_mode = "new"`
  triggers / `agent_send` fan-out / parallel channel chats; pre-librefang#3172 rekey
  history) on the field's new home in `AgentSubsystem` — the comment was
  truncated to a one-liner during the move.
* Drop the orphan `running_tasks` doc comment that was left dangling on
  `LibreFangKernel` after the field migrated to `AgentSubsystem`.
* Add an explicit "Status" + "Shutdown ordering invariant" section to
  `kernel::subsystems` — clarifies that focused traits and forward impls have
  already landed (follow-ups #2 + #3) while method-body migration is still
  pending, and documents that graceful shutdown is broadcast-based via
  `shutdown_tx` + `supervisor.shutdown()` + `workflows.engine.drain_on_shutdown()`,
  so reorganising fields between subsystem structs is safe with respect to
  Rust drop order.

No behavior change.

* test(kernel): boundary tests for ProcessSubsystemApi + MeteringSubsystemApi (librefang#3565)

The focused per-subsystem traits added in follow-up #2 had no consumers in
this PR (the API layer still binds against `Arc<dyn KernelApi>`). Add
`#[cfg(test)]` boundary tests next to the trait impls to:

1. **Pin the trait shape** — assert object-safety (`&dyn`) and `Send + Sync`
   bounds at compile time. Drift in lifetimes / bounds breaks here, not at
   the first real caller.
2. **Validate routing** — call methods through `&dyn ProcessSubsystemApi` /
   `&dyn MeteringSubsystemApi` and verify the returned `&Arc<...>` handles
   are pointer-equal to the ones the constructor stored, with no hidden
   clone or rewrapping.
3. **Demonstrate the mock pattern** — `ProcessSubsystemApi` ships with a
   minimal `StubProcesses` impl proving the trait is implementable
   independently of `LibreFangKernel` boot, so future tests can mock the
   subsystem without spinning up the full kernel.
4. **Cover both return shapes** — `MeteringSubsystemApi` returns both
   borrowed `&Arc<T>` (audit_log, metering_engine) and an owned snapshot
   (current_budget); the test exercises both.

Two subsystems are covered as exemplars; the same pattern applies to the
remaining 11 traits when consumers materialise.

  cargo test -p librefang-kernel --lib subsystems::
  → 4 passed; 0 failed

* test(kernel): snapshot post-boot default_model in reload-invalid-TOML test

The pre-existing sanity assertion expected `boot_with_config()` to leave
the baseline `default_model.model` untouched, but boot legitimately
rewrites it via the auto-detect fallback in `kernel/boot.rs` when the
requested provider has no usable credentials. On dev machines that have
OPENAI_API_KEY set or Claude Code / Copilot CLI logged in, the fallback
fires and the test panics deterministically with `gpt-5` /
`copilot/gpt-4o` instead of `user-picked-model`.

The regression actually being guarded here is "invalid TOML reload must
not mutate the live config" — it does not depend on the post-boot value
being exactly the baseline. Snapshot whatever `boot_with_config()`
settles on and assert that snapshot survives the failed reload, so the
test is deterministic regardless of ambient credentials.

Behaviour under test is unchanged; CI was already green because its
runners have none of those credentials configured.

* docs(changelog): note kernel subsystem decomposition under [Unreleased]

The refactor itself is internal — no user-visible behaviour change — but
the structural shift (~70 flat fields → 13 typed subsystem handles plus
focused-trait forwarding) is large enough to warrant a Maintenance-level
trace in the changelog so future reviewers walking `git log` for "why
did `LibreFangKernel` shape change?" find the rationale and the
remaining-follow-up notes (method-body migration + KernelApi trait
carving) without diving into 19 individual commits.
neo-wanderer pushed a commit that referenced this pull request May 16, 2026
…brefang#5074)

* fix(vault): unify init() key resolution with resolve_master_key() (librefang#5069)

The MCP OAuth auth_start handler stores PKCE state across three
sequential vault_set calls (pkce_verifier → pkce_state → redirect_uri).
The first call lazy-creates the vault via init() + set(); every
subsequent call walks unlock() + set() against a fresh CredentialVault
instance constructed inside KernelOAuthProvider::vault_set. Pre-fix the
second call's unlock() failed with aead::Error because init() duplicated
the env / keyring lookup code from resolve_master_key(). The two sites
could resolve different master keys on container hosts where DBus is
partially available or env mutation races with the vault unlock, leaving
a file the same process could not decrypt one call later.

Two changes:

1. init() now resolves the master key through resolve_master_key()
   directly. The random-key fallback only kicks in when
   resolve_master_key() returns VaultLocked (no env, no keyring), and
   that branch stores the generated key into the keyring so subsequent
   resolves on fresh instances find the same value. By construction the
   two code paths can no longer diverge.

2. init() now performs a post-write verification: it constructs a
   sibling CredentialVault::new instance and calls unlock() on the
   freshly-written file. If unlock fails, init() rolls back the file
   and returns an actionable Vault error pointing at the divergence
   rather than letting the next vault_set caller fail downstream with
   an opaque aead::Error. This also catches an AAD path-binding
   regression (issue hypothesis #2) at the source.

Regression coverage:

- vault::tests::init_then_set_then_reopen_unlock_via_env_key pins the
  init() + set() -> fresh-instance unlock() round trip via the
  env-resolved master key, mirroring the failing auth_start sequence.
- mcp_oauth_provider::tests::vault_set_twice_round_trips_via_env_key
  exercises the same flow through KernelOAuthProvider::vault_set --
  the exact API the auth_start handler calls -- and reads every entry
  back through a fresh provider instance to confirm no entry is lost
  across the re-open boundary.

Verification:
- cargo check --workspace --lib
- cargo clippy -p librefang-extensions -p librefang-kernel --lib --tests -- -D warnings
- cargo test -p librefang-extensions (87 passed)
- cargo test -p librefang-kernel --lib vault (10 passed, including
  cross-test interaction with vault_cache_reuses_unlocked_handle_across_calls)
- cargo test -p librefang-api --test mcp_auth_routes_extra_test (8 passed)

* fix(vault): pin env-mutation regression + align serial test groups

Addresses code review on librefang#5074.

- new vault::tests::init_env_mutation_during_save_rolls_back_and_surfaces_typed_error
  test: a worker thread mutates LIBREFANG_VAULT_KEY between init's
  resolve_master_key() and init's post-write verify-unlock, proving
  the verify-unlock catches the divergence, rolls back the corrupt
  vault.enc, and surfaces a typed ExtensionError::Vault(_) naming the
  divergence (not a bare aead::Error). Verified failing on origin/main
  (pre-fix init() returns Ok silently and the test's expect_err panics)
  and passing on this branch.
- align serial_test groups: vault_set_twice_round_trips_via_env_key
  (mcp_oauth_provider), vault_cache_reuses_unlocked_handle_across_calls
  and install_integration_writes_through_cached_vault_handle
  (kernel::tests) all move to #[serial_test::serial(librefang_vault_key)]
  so every test in librefang-kernel that mutates the process-global
  LIBREFANG_VAULT_KEY sits in one named group. Two disjoint serial
  groups in the same crate could otherwise race the env var.
- tone down init() doc-comment speculation about whitespace tolerance
  and Zeroizing-vs-String temporary lifetimes — neither path strips
  whitespace and neither is optimizer-sensitive at env-read. Replace
  with the concrete failure mode: env mutation between init's save
  and the next unlock's read.
- log on rollback unlink failure with tracing::warn (path, error).
  EACCES would otherwise silently leave a corrupt vault.enc that the
  next init() rejects with "Vault already exists"; operators need
  visibility into the residue.
- drop redundant drop(verify) — the block-scope drop runs anyway.
neo-wanderer pushed a commit that referenced this pull request May 23, 2026
…fang#5454)

Surfaces from the post-librefang#5445 audit (6th in the dingtalk/qq/omnibus
review chain — first verdict that wasn't BROKEN/NEEDS_HOTFIX). All
three nits are doc/UX cleanups, none are runtime regressions:

1. **Docstring claim #3 was wrong** — said "429 Retry-After honoured
   on every outbound POST", but only Cloud API uses
   `_cloud_post_with_retry`; the gateway path calls `_http_request`
   directly and raises on any non-2xx. Soften the claim to
   "Cloud-API outbound POSTs only" + explain when gateway-side
   retry would matter (operators proxying the local Baileys
   gateway behind a rate-limiting reverse-proxy).

2. **WHATSAPP_VERIFY_TOKEN unset failed silently** — Meta's
   subscription handshake returned 403 with no log line pointing
   back to the missing env var; operators saw "subscription failed"
   in the Meta dashboard with nothing in their daemon logs. Now
   warns at __init__, matching the WHATSAPP_APP_SECRET pattern
   already there.

3. **WHATSAPP_GROUP_POLICY is dead config** — Cloud API webhook
   payloads don't surface a group/conversation distinction (we
   hardcode `is_group=False` at `_handle_post_webhook`), and gateway
   mode delegates inbound entirely to the Node Baileys gateway which
   never calls back into the sidecar's `should_handle_message`
   filter. So `WHATSAPP_GROUP_POLICY` has zero effect today. Mark
   the schema field with an explicit "(currently inert)" note in the
   label so operators don't waste time setting it. Kept the field
   itself for forward-compat — Meta has been rolling out group-chat
   support to the Cloud API gradually and we want the schema to be
   ready when it lands.

Drive-by: dropped the misleading send-voice docstring claim (lines
22-23 + 36-39 promised a `{gateway}/message/send-voice` multipart
upload route that doesn't exist in code; rationale comment about
the daemon's ChannelContent::Voice always carrying a URL at the
dispatch boundary explains why we don't need it).

Test:
- New test_get_verify_empty_self_token_rejects asserts BOTH the
  __init__ warn fires AND the handshake fails closed. Regression
  guard for #2 — without this an attacker who guesses the empty
  hub.verify_token could subscribe their own callback URL.
- cd sdk/python && pytest tests/test_whatsapp_adapter.py — 79 passed
  (was 78; +1 regression guard).

This closes the audit chain. WhatsApp itself is structurally clean
— natural routing key (phone) is preserved as channel_id both
directions, the bug family from librefang#5417librefang#5449 doesn't apply.
neo-wanderer pushed a commit that referenced this pull request Jun 5, 2026
…cay, dedup, prompt budget, async consolidate) (librefang#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 (librefang#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>
neo-wanderer pushed a commit that referenced this pull request Jun 5, 2026
…CH, multi-keyword search, configurable UPDATE thresholds (librefang#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 librefang#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 librefang#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 librefang#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 librefang#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]>
neo-wanderer pushed a commit that referenced this pull request Jun 5, 2026
…ibrefang#5862)

Plugin hook subprocesses previously ran with network and filesystem
access wide open, and the Linux seccomp / Landlock backends were not in
the default feature set — so the shipped binary applied no syscall or LSM
filtering at all.

Secure-by-default changes (breaking):

- Flip `allow_network` / `allow_filesystem` defaults from `true` to
  `false` in every default source: `HookConfig::default()` in
  plugin_runtime.rs and the serde `#[serde(default)]` for
  `ContextEngineHooks` in librefang-types (was `default_true_bool`, now
  the bool default `false`, which also makes the derived `Default` and
  the serde-omitted default agree — they previously disagreed).
- Enable `seccomp-sandbox` and `landlock-sandbox` in the runtime's
  default feature set. Their Linux-only deps (`seccompiler`, `landlock`)
  are moved under `[target.'cfg(target_os = "linux")'.dependencies]` so
  the features can ship in `default` without breaking macOS / Windows
  builds (the `dep:` activations are no-ops where the dep is absent, and
  all code touching them is already `cfg(all(target_os = "linux", feature))`).

Two latent bugs surfaced by turning the sandbox on by default (in-scope,
same file / feature):

- The seccomp allowlist was missing glibc / language-runtime start-up
  syscalls (rseq, set_robust_list, rt_sigtimedwait, statx,
  sched_getaffinity, …). With seccomp off by default the gap was never
  exercised; on by default it SIGSYS-killed every hook child before it
  read stdin (observed as "Broken pipe"). Added the universal start-up
  set (present on both x86_64 and aarch64).
- The `unshare` namespace probe checked only `unshare --help`, not
  whether the kernel grants the namespace. In unprivileged containers /
  hardened CI the binary exists but CLONE_NEWNET / CLONE_NEWNS returns
  EPERM, so wrapping a deny-network / deny-filesystem hook with `unshare`
  killed the child instead of failing open to env-var / Landlock
  isolation. The probe now runs `unshare --<ns> -- true` and only wraps
  when the namespace can actually be created.

Tests:

- librefang-types: omitted `[hooks]` flags deserialize to deny; explicit
  opt-in honoured; derived Default matches serde default.
- librefang-runtime: `HookConfig::default()` denies both;
  `locked_down_default_hook_completes_round_trip` runs a hook under the
  full deny-by-default sandbox (seccomp + unshare-or-fallback) and
  asserts a clean JSON round trip — the regression guard for the seccomp
  allowlist and the unshare probe.

Verified in the Linux dev container: `cargo check -p librefang-runtime
--lib`, `cargo clippy -p librefang-runtime --lib -D warnings`,
`cargo test -p librefang-runtime --lib plugin` (58 pass),
`cargo test -p librefang-types context_engine_hooks` (2 pass).

Co-authored-by: Evan <[email protected]>
neo-wanderer pushed a commit that referenced this pull request Jun 5, 2026
…log gen (librefang#5924)

extract_pr_numbers grepped every #N on a oneline subject, so in-title
cross-references (issue refs like 'fixes librefang#5740', prior-PR refs like
'post-librefang#5053', and 'part N of M' markers like '(#2)') were treated as
their own PRs and fed to 'gh pr view', pulling unrelated ancient or
unmerged PRs into the generated release notes.

A squash merge always appends the PR reference as the trailing (#N) of
the subject, so parse only the last match per line. Split the parsing
into a pure parse_pr_numbers helper with unit coverage.

Co-authored-by: Evan <[email protected]>
neo-wanderer pushed a commit that referenced this pull request Jun 27, 2026
…ang#6256)

The webhook store guarded its in-memory state with `std::sync::RwLock`
and exposed synchronous `list`/`get`/`create`/`update`/`delete` methods
called directly from async axum handlers. Holding a blocking lock (and
running synchronous `std::fs` persistence) on the runtime threads can
stall the executor under contention.

Switch the guard to `tokio::sync::RwLock` and make the accessor methods
`async`. Persistence now runs the file I/O on `spawn_blocking` so the
atomic write never blocks the async runtime. `load` stays synchronous:
it runs once at daemon boot / test setup, off the request path.

Handlers are updated to `.await` the store calls. Where a handler held
the `!Send` `ErrorTranslator` across the new await point, the translated
messages are resolved up front and the translator dropped before the
await (per CLAUDE.md), keeping the axum `Handler` bound satisfied.

Closes #2

Co-authored-by: Evan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/channels Messaging channel adapters area/ci CI/CD and build tooling area/docs Documentation and guides area/kernel Core kernel (scheduling, RBAC, workflows) has-conflicts PR has merge conflicts that need resolution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants