Skip to content

feat(types): KernelConfig.parallel_tools section (PR-3/6)#3144

Merged
houko merged 2 commits into
mainfrom
feat/parallel-tool-calls-m3
Apr 25, 2026
Merged

feat(types): KernelConfig.parallel_tools section (PR-3/6)#3144
houko merged 2 commits into
mainfrom
feat/parallel-tool-calls-m3

Conversation

@houko

@houko houko commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds the [parallel_tools] configuration section to KernelConfig in
librefang-types. Schema only — runtime behaviour is unchanged because
nothing in the agent loop reads the new field yet (PR-4 wires the
dispatcher; PR-5 flips the default to enabled = true).

Changes

  • New ParallelToolsConfig struct with four fields:
    • enabled: bool — master switch, default false.
    • max_concurrent: u32 — bucket-level concurrency cap, default 4.
      0 means uncapped.
    • mcp_default_safety: String"read_only" | "write_shared"
      fallback for unannotated MCP tools, default "write_shared" (safe
      serialised behaviour). Kept as String deliberately so PR-4 can
      promote it to a typed enum in lockstep with the dispatcher.
    • mcp_readonly_allowlist: Vec<String> — explicit overrides keyed
      by fully-namespaced MCP tool name (mcp__server__name).
  • KernelConfig.parallel_tools: ParallelToolsConfig field (with
    #[serde(default)]) plus matching entry in the KernelConfig::default()
    impl, per CLAUDE.md "Config fields added to KernelConfig struct
    MUST also be added to the Default impl".
  • Four unit tests in config::types::tests:
    • parallel_tools_default_is_disabled — defaults match the spec.
    • parallel_tools_serde_round_trip — TOML + JSON round-trip with a
      fully-populated struct.
    • parallel_tools_missing_in_kernel_config_uses_default — old
      pre-PR-3 config.toml files (no [parallel_tools] section) hydrate
      the field from Default.
    • parallel_tools_partial_section_fills_remaining_with_default
      enabled = true alone deserialises with the rest from Default,
      verifying #[serde(default)] on the struct itself.

Non-goals

  • No agent-loop / parallel_dispatch / tool_classifier code
    touched. Runtime behaviour is byte-for-byte identical.
  • No new dependencies introduced.

Verification

  • cargo test -p librefang-types --lib — 624 passed (4 new).
  • cargo check --workspace --lib — clean.
  • cargo clippy -p librefang-types --all-targets -- -D warnings — clean.

Test plan

  • Unit tests cover defaults, full round-trip, missing section, and
    partial section.
  • KernelConfig::default() includes the new field (assertion in
    parallel_tools_default_is_disabled).
  • PR-4 will add live integration tests at the dispatcher injection
    site (per CLAUDE.md guidance: integration tests at injection
    site, not just implementation site).

Adds the configuration schema the upcoming agent_loop dispatcher
will consult before launching tools concurrently. Pure scaffolding —
no business code reads `parallel_tools` yet, so runtime behaviour is
unchanged.

`ParallelToolsConfig` (in `librefang-types::config::types`):
- enabled: bool, default `false`. Master switch for the parallel
  dispatcher. PR-5 will flip the default to `true` after the
  streaming integration lands.
- max_concurrent: u32, default 4. Cap on concurrent calls within a
  single bucket (`0` = uncapped).
- mcp_default_safety: String, default "write_shared". Default
  ParallelSafety class for unannotated MCP tools. Conservative —
  unannotated MCP tools serialise instead of optimistically
  parallelising. Accepted: "read_only" | "write_shared" (string,
  not enum, until PR-4 wires the dispatcher).
- mcp_readonly_allowlist: Vec<String>, default empty. Explicit MCP
  tool names (`mcp__server__name` form) to treat as ReadOnly
  regardless of `mcp_default_safety`.

`KernelConfig` grows a `parallel_tools` field
(`#[serde(default)]`); `Default` impl populated per CLAUDE.md
"Config fields added to KernelConfig struct MUST also be added to
the Default impl".

Companion: also fixes a baseline `clippy::manual_pattern_char_comparison`
in `scheduler.rs:555` (`s.find(|c: char| c == '/' || c == '?' || c == '#')`
→ `s.find(['/', '?', '#'])`) — surfaced when running
`cargo clippy -p librefang-types --all-targets -- -D warnings` and
unrelated to PR-3 itself, but blocks this PR's verification gate.

Tests (4 new):
- parallel_tools_default_is_disabled
- parallel_tools_serde_round_trip (toml + json double round-trip)
- parallel_tools_missing_in_kernel_config_uses_default (legacy
  config.toml without the section deserialises cleanly)
- parallel_tools_partial_section_fills_remaining_with_default

Verification:
- cargo test -p librefang-types --lib: 624 ok
- cargo clippy -p librefang-types --all-targets -- -D warnings: clean

Stack: independent, base main. PR-4 will read this config to gate
the non-streaming dispatcher integration.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

houko added a commit that referenced this pull request Apr 25, 2026
… fix)

Closes a high-severity gap from PR #3145 review: `PreScript.env` was
unvalidated, so an attacker-controlled cron config could set
`LD_PRELOAD=/tmp/x.so` (or the Darwin equivalent
`DYLD_INSERT_LIBRARIES`, or `PATH=/tmp/evil:$PATH`, or `IFS=...`)
and fully bypass the path allowlist on `argv[0]`. The "scripts under
~/.librefang/scripts only" promise is moot if the spawned binary
gets hijacked at link / shell-parse time.

Changes:
- New `DANGEROUS_ENV_KEYS` const listing 8 dynamic-linker / PATH /
  IFS knobs that we refuse to honor in `PreScript.env`. Match is
  case-insensitive (Windows env keys are case-insensitive natively;
  defence-in-depth elsewhere).
- New `PreScriptValidationError::DangerousEnvKey { key }` variant.
- `validate_pre_script` rejects before any file IO so the cheap
  check fails first.
- Hooked into the `CronJob::validate_action` path so config-load
  rejects bad pre_scripts even before the dispatcher runs them
  (review medium issue: `validate_pre_script` was previously
  dead code outside its own unit tests).

Companion: same `scheduler.rs` host-parse `clippy::manual_pattern_char_comparison`
fix as in #3144 / #3147 / #3149 (the line moved from :555 to :785
once the denylist + tests landed; identical 3-char fix). Whichever
PR lands first the others rebase cleanly.

Tests (5 new in `scheduler::tests`):
- pre_script_validation_rejects_ld_preload
- pre_script_validation_rejects_path_override
- pre_script_validation_rejects_dyld_insert
- pre_script_validation_rejects_dangerous_env_case_insensitive
- pre_script_validation_accepts_safe_env_keys

Verification:
- cargo test -p librefang-types --lib scheduler: 81 ok
- cargo clippy -p librefang-types -p librefang-kernel
  --all-targets -- -D warnings: clean
@houko
houko enabled auto-merge (squash) April 25, 2026 15:04
houko added a commit that referenced this pull request Apr 25, 2026
…ification (PR-6/6) (#3147)

* fix(runtime-mcp): preserve tool annotations for parallel safety classification (PR-6/6)

LibreFang's MCP `tools/list` parsing was discarding the `annotations`
field across all three tool-discovery paths (SSE, Streamable HTTP,
stdio). MCP spec 0.5+ uses these annotations
(`readOnlyHint` / `destructiveHint` / `idempotentHint`) to tell
clients which tools are safe to fan out and which need to serialise.
Without preserving them, every MCP tool ended up at
`ToolApprovalClass::Unknown` → `ParallelSafety::WriteShared` —
defeating the whole point of parallel-safe MCP tools.

This PR adds a single helper, `inject_annotation_class`, that maps
the MCP annotations onto a `metadata.tool_class` field in the tool's
input schema. The existing `runtime/tool_classifier.rs::classify_tool`
already reads `metadata.tool_class` (its primary escape hatch) so
the projection lights up automatically once the annotations land in
the schema.

Mapping rule:
- `readOnlyHint=true && destructiveHint=false`  → `"readonly_search"`
- otherwise                                     → `"mutating"`

Per spec, `destructiveHint` defaults to `true` when missing, so an
absent annotation block is treated as "destructive" — exactly the
existing pre-fix behaviour, just made explicit.

Three call sites updated:
- SSE: `lib.rs:1050` (raw JSON `tools/list` response loop)
- Streamable HTTP: same parse path, around line 1232
- stdio (rmcp SDK): SDK's `Tool` struct exposes `annotations` —
  serialise it with the tool's input_schema before injection so
  the same helper handles all three branches uniformly.

Companion fix: `scheduler.rs:555` baseline
`clippy::manual_pattern_char_comparison` lint
(`s.find(|c: char| c == '/' || c == '?' || c == '#')` →
`s.find(['/', '?', '#'])`). The lint surfaces when running clippy
on librefang-types via librefang-runtime-mcp's transitive
compilation; identical fix is also queued in PR #3144 (parallel-tool-calls
PR-3) so whichever lands first the other rebases cleanly.

Tests (6 new in `runtime_mcp::tests`):
- inject_annotation_readonly_sets_readonly_search
- inject_annotation_destructive_sets_mutating
- inject_annotation_default_destructive_when_missing
- inject_annotation_no_annotations_preserves_schema
- inject_annotation_preserves_existing_metadata
- inject_annotation_existing_tool_class_overwritten

Verification:
- cargo test -p librefang-runtime-mcp --lib: 76 ok
- cargo clippy -p librefang-runtime-mcp --all-targets -- -D warnings: clean

Stack: independent, base main. Once this lands the parallel
dispatcher (PR-4/5 of the parallel-tool-calls series) gets accurate
ParallelSafety classification for every annotated MCP tool.

* style: cargo fmt

* test(runtime-mcp): pin producer/consumer string contract for tool_class

Self-review caught that the existing tests verify
`inject_annotation_class` in isolation but never check that the literals
it emits ("readonly_search", "mutating") actually parse via
`ToolApprovalClass::from_snake_case` on the consumer side. A typo or a
future rename in either crate would silently fall back to Unknown →
WriteShared and the whole MCP-tool parallelisation fix becomes a no-op.

Add `injected_class_strings_parse_into_approval_class` to round-trip
both literals through `ToolApprovalClass::from_snake_case` so the
contract is enforced in CI.

Also extend the `inject_annotation_class` doc comment to record why
`idempotentHint` / `openWorldHint` are intentionally dropped at this
layer (no matching ParallelSafety variant today) so a future maintainer
doesn't think it's an oversight.
@houko
houko merged commit 5436e91 into main Apr 25, 2026
20 checks passed
@houko
houko deleted the feat/parallel-tool-calls-m3 branch April 25, 2026 15:20
houko added a commit that referenced this pull request Apr 25, 2026
… fix)

Closes a high-severity gap from PR #3145 review: `PreScript.env` was
unvalidated, so an attacker-controlled cron config could set
`LD_PRELOAD=/tmp/x.so` (or the Darwin equivalent
`DYLD_INSERT_LIBRARIES`, or `PATH=/tmp/evil:$PATH`, or `IFS=...`)
and fully bypass the path allowlist on `argv[0]`. The "scripts under
~/.librefang/scripts only" promise is moot if the spawned binary
gets hijacked at link / shell-parse time.

Changes:
- New `DANGEROUS_ENV_KEYS` const listing 8 dynamic-linker / PATH /
  IFS knobs that we refuse to honor in `PreScript.env`. Match is
  case-insensitive (Windows env keys are case-insensitive natively;
  defence-in-depth elsewhere).
- New `PreScriptValidationError::DangerousEnvKey { key }` variant.
- `validate_pre_script` rejects before any file IO so the cheap
  check fails first.
- Hooked into the `CronJob::validate_action` path so config-load
  rejects bad pre_scripts even before the dispatcher runs them
  (review medium issue: `validate_pre_script` was previously
  dead code outside its own unit tests).

Companion: same `scheduler.rs` host-parse `clippy::manual_pattern_char_comparison`
fix as in #3144 / #3147 / #3149 (the line moved from :555 to :785
once the denylist + tests landed; identical 3-char fix). Whichever
PR lands first the others rebase cleanly.

Tests (5 new in `scheduler::tests`):
- pre_script_validation_rejects_ld_preload
- pre_script_validation_rejects_path_override
- pre_script_validation_rejects_dyld_insert
- pre_script_validation_rejects_dangerous_env_case_insensitive
- pre_script_validation_accepts_safe_env_keys

Verification:
- cargo test -p librefang-types --lib scheduler: 81 ok
- cargo clippy -p librefang-types -p librefang-kernel
  --all-targets -- -D warnings: clean
@github-actions github-actions Bot removed the ready-for-review PR is ready for maintainer review label Apr 25, 2026
houko added a commit that referenced this pull request Apr 25, 2026
* feat(types): cron pre_script + silent_marker schema (PR-1/3)

Adds the configuration schema for cron pre-script injection +
silent-response markers. Pure schema work — no scheduler dispatch
code reads these fields yet (M2 will wire execution; M3 will add
the dashboard editor + optional TOTP approval).

`CronAction::AgentTurn` grows two optional fields:
- `pre_script: Option<PreScript>` — when set, the M2 dispatcher
  will run the argv before the scheduled prompt fires, capture
  stdout, and inject it into the LLM context. Distinct from the
  existing `pre_check_script` (which gates whether the agent runs
  at all via `{"wakeAgent": false}`); both can coexist.
- `silent_marker: Option<String>` — when present at the end of
  the response, the dispatcher suppresses delivery. Match is
  "last non-empty trimmed line == marker" — strict, doesn't
  trigger on mid-response mentions of `[SILENT]`. None means
  delivery always fires.

`PreScript` (new struct in `librefang-types::scheduler`):
- argv: Vec<String> — split form, no shell parsing.
- cwd: Option<String> — defaults to workspace root.
- env: HashMap<String, String> — added on top of daemon env;
  per-job secrets should still go through LIBREFANG_VAULT_KEY.
- `#[serde(deny_unknown_fields)]` so typos surface instead of
  being silently ignored.

`validate_pre_script(script, home_dir)` enforces a path allowlist:
argv[0] must canonicalize to a file under `<home_dir>/scripts/`.
Component-level prefix comparison via `Path::starts_with` (so
`/home/X-other` doesn't match `/home/X`). `..` escapes are
rejected after canonicalize. Errors typed via
`PreScriptValidationError` (thiserror): EmptyArgv,
OutsideAllowlist { path, home_dir }, NotFound { path }.

Companion changes (mechanical, fields propagated to existing
`CronAction::AgentTurn` construction sites):
- librefang-api/src/channel_bridge.rs: 2 lines (pre_script +
  silent_marker = None).
- librefang-api/src/routes/workflows.rs: 2 lines (same).
- librefang-types/Cargo.toml: 1 line (test-only addition).

Tests (8 new in `scheduler::tests`):
- pre_script_validation_accepts_path_under_scripts_dir
- pre_script_validation_rejects_outside_allowlist
- pre_script_validation_rejects_dot_dot_escape
- pre_script_validation_rejects_empty_argv
- pre_script_validation_rejects_nonexistent_path
- cron_action_agent_turn_serde_round_trip_with_pre_script
- cron_action_agent_turn_serde_compat_no_pre_script (legacy
  config without the field deserialises to None)
- silent_marker_default_via_serde_skip (None doesn't serialise)

Verification:
- cargo test -p librefang-types --lib: 628 ok
- cargo check --workspace --lib: clean
- cargo clippy -p librefang-types -p librefang-api --all-targets
  -- -D warnings: clean

Stack: independent, base main. PR-2 (next) will wire kernel
dispatcher: argv exec + stdout capture + prompt injection +
silent_marker check + wake-gate path-whitelist hardening.

* style: cargo fmt for scheduler.rs

* fix(types): reject dangerous env keys in PreScript validation (review fix)

Closes a high-severity gap from PR #3145 review: `PreScript.env` was
unvalidated, so an attacker-controlled cron config could set
`LD_PRELOAD=/tmp/x.so` (or the Darwin equivalent
`DYLD_INSERT_LIBRARIES`, or `PATH=/tmp/evil:$PATH`, or `IFS=...`)
and fully bypass the path allowlist on `argv[0]`. The "scripts under
~/.librefang/scripts only" promise is moot if the spawned binary
gets hijacked at link / shell-parse time.

Changes:
- New `DANGEROUS_ENV_KEYS` const listing 8 dynamic-linker / PATH /
  IFS knobs that we refuse to honor in `PreScript.env`. Match is
  case-insensitive (Windows env keys are case-insensitive natively;
  defence-in-depth elsewhere).
- New `PreScriptValidationError::DangerousEnvKey { key }` variant.
- `validate_pre_script` rejects before any file IO so the cheap
  check fails first.
- Hooked into the `CronJob::validate_action` path so config-load
  rejects bad pre_scripts even before the dispatcher runs them
  (review medium issue: `validate_pre_script` was previously
  dead code outside its own unit tests).

Companion: same `scheduler.rs` host-parse `clippy::manual_pattern_char_comparison`
fix as in #3144 / #3147 / #3149 (the line moved from :555 to :785
once the denylist + tests landed; identical 3-char fix). Whichever
PR lands first the others rebase cleanly.

Tests (5 new in `scheduler::tests`):
- pre_script_validation_rejects_ld_preload
- pre_script_validation_rejects_path_override
- pre_script_validation_rejects_dyld_insert
- pre_script_validation_rejects_dangerous_env_case_insensitive
- pre_script_validation_accepts_safe_env_keys

Verification:
- cargo test -p librefang-types --lib scheduler: 81 ok
- cargo clippy -p librefang-types -p librefang-kernel
  --all-targets -- -D warnings: clean

* style: post-rebase fmt drift
houko added a commit that referenced this pull request Apr 25, 2026
* feat(runtime): cache_hit_ratio metric + trajectory field (M2/2)

Builds on PR-1 (#3126, the system_and_3 stamping). Adds the
single-value observability metric the dashboard surfaces so users
can see at a glance whether prompt caching is paying off for their
workload. Pure observability — no behaviour change.

`cache_hit_ratio = cache_read / (cache_read + cache_creation)`,
in `[0.0, 1.0]`. Returns `0.0` when both numerators are zero
(caching not active for this turn / model doesn't support it).

Wire-up:
- `crates/librefang-runtime/src/agent_loop.rs`: new `cache_hit_ratio`
  helper alongside `accumulate_token_usage`. Per-turn `tracing::info!`
  on `librefang::cache` target with agent / hit_ratio / creation /
  read so existing log pipelines (Tempo, Grafana) pick it up
  without dashboard changes.
- `crates/librefang-kernel/src/trajectory/mod.rs`:
  `TrajectoryMetadata` grows `cache_hit_ratio: Option<f32>`
  (`#[serde(default, skip_serializing_if = "Option::is_none")]`)
  so legacy trajectories deserialise to None and replays don't
  need to know about caching to round-trip.

Tests (6 new):
- cache_hit_ratio_zero_when_no_caching (both 0)
- cache_hit_ratio_full_hit (read=100, creation=0 → 1.0)
- cache_hit_ratio_partial (read=70, creation=30 → 0.7)
- cache_hit_ratio_cold_start (read=0, creation=100 → 0.0)
- trajectory_metadata_cache_hit_ratio_serde_round_trip
- trajectory_metadata_cache_hit_ratio_legacy_compat (legacy JSON
  without the field deserialises to None, no panic)

Companion fix: `scheduler.rs:555` baseline
`clippy::manual_pattern_char_comparison` (`s.find(|c: char| c == '/'
|| c == '?' || c == '#')` → `s.find(['/', '?', '#'])`). Same one-line
fix is also queued in PR #3144 / #3147; whichever lands first the
others rebase cleanly (identical content).

Verification:
- cargo test -p librefang-runtime --lib agent_loop: 167 ok
- cargo test -p librefang-kernel --lib trajectory: 14 ok
- cargo clippy -p librefang-runtime -p librefang-kernel
  --all-targets -- -D warnings: clean

Stack: independent, base main. Closes the prompt-caching-system-and-3
plan — system_and_3 stamping (PR-1) plus this PR cover the full
"borrow hermes-agent's cache strategy" scope.

* refactor(types): consolidate cache_hit_ratio onto TokenUsage

Two parallel implementations of the same cache-ratio math (one in
agent_loop with f32/0.0-on-empty, one in trajectory with Option<f32>)
were drifting apart in semantics. Move the authoritative implementation
to TokenUsage::cache_hit_ratio() in librefang-types and have both
callers delegate:

- runtime: tracing log uses .unwrap_or(0.0) for the f32 field, with a
  comment explaining how to disambiguate "no caching" from "0% hit"
  via the creation/read totals.
- kernel: trajectory::compute_cache_hit_ratio() becomes a thin
  re-export; with_cache_hit_ratio builder doc clarified that the
  exporter never sees TokenUsage and call-site wiring is a follow-up.

Test coverage for the math is now centralized in librefang-types
(4 tests), runtime drops its 4 redundant tests, kernel keeps a smoke
test for the re-export plus the builder/serde/legacy-compat tests.
houko added a commit that referenced this pull request Apr 25, 2026
…on (#3167)

#3144 added KernelConfig.parallel_tools but did not refresh the
schemars-derived golden fixture. The PR's selective CI lane only
ran librefang-types so the drift slipped through; on push to main
the full-run lane has been failing on
kernel_config_schema_matches_golden_fixture for every commit since.
Any subsequent PR that touches librefang-api (e.g. #3162's dashboard
edits) gets blocked by the same panic.

Regenerated via:

    cargo test -p librefang-api --test config_schema_golden \
        -- --ignored regenerate_golden --nocapture

Diff is exactly the new ParallelToolsConfig definition + the
parallel_tools field reference — verified against the current main
HEAD (53dde25).
houko added a commit that referenced this pull request Apr 25, 2026
…#3171)

The schemars-derived KernelConfig schema lives in librefang-types,
but the golden-file regression guard
(`kernel_config_schema_matches_golden_fixture`) lives in
librefang-api. Selective PR-lane CI fans out by directly-touched
crates only — so a types-only PR that adds a new schema field never
runs librefang-api's tests, and the drift goes undetected until the
push-to-main full-run lane explodes. By then main is already poisoned
and every subsequent PR whose lane includes librefang-api gets blocked
on the same panic.

Incident chain that motivated this:
  - #3144 (KernelConfig.parallel_tools) merged with stale golden;
    PR-lane only ran librefang-types.
  - main full-run on the merge commit failed
    `kernel_config_schema_matches_golden_fixture` and stayed broken.
  - #3162 (dashboard-only) tripped the same panic two days later
    purely because its selective lane happened to include
    librefang-api.
  - #3167 regenerated the fixture as a one-shot fix.

This guard makes the same mistake fail at PR review time instead of
poisoning main. Implementation is one shell stanza in the existing
"Detect Changes" job, no new actions or jobs.
houko added a commit that referenced this pull request May 18, 2026
…g (P0–P3) (#5219)

* feat(channels): add sidecar protocol skeleton variants (P0)

Add placeholder protocol variants to SidecarEvent/SidecarCommand so
external adapters can be built against the final wire shape and later
phases land incrementally:

- SidecarEvent::Typing (inbound; wired to typing_events in P2)
- SidecarCommand::{ReadyAck, Typing, Reaction, Interactive,
  StreamStart, StreamDelta, StreamEnd, Heartbeat}

New variants are inert: not wired into `impl ChannelAdapter`. Adding
SidecarEvent::Typing made the stdout-reader match non-exhaustive, so a
behaviourally inert no-op catch-all arm was added; existing
Ready/Message/Error handling is byte-for-byte unchanged.

Verification:
- cargo check --workspace --lib: clean
- cargo clippy -p librefang-channels --all-targets -- -D warnings: clean
- cargo test -p librefang-channels (sidecar): 11 passed
  (7 original + echo integration + 3 new roundtrip/regression)

* feat(channels): sidecar structured content I/O (P1)

Carry the full ChannelContent enum + group/thread/metadata both ways
over the sidecar protocol while keeping legacy text-only adapters
working byte-for-byte.

- SidecarMessageParams: optional content/username/librefang_user/
  is_group/thread_id/group_members/group_participants/metadata
  (all #[serde(default)]). `content` supersedes `text`; absent ->
  ChannelContent::Text(text) fallback.
- SidecarSendParams: content (skip-if-none), thread_id (skip-if-none),
  full ChannelUser; text kept as best-effort flatten for legacy.
- Inbound reader folds username/group_members/group_participants into
  ChannelMessage.metadata; wires librefang_user/is_group/thread_id.

Deviations from the phase plan, evidence-backed:
- Dropped the speculative `target_agent` wire field: every in-process
  adapter sets ChannelMessage.target_agent = None; routing is the
  bridge AgentRouter's job (see email.rs:830). Adding it would
  contradict the established architecture.
- Boxed SidecarEvent::Message { params } — SidecarMessageParams now
  dwarfs the other variants (clippy::large_enum_variant). Box is
  transparent to serde; consumers unbox once for field moves.

Verification:
- cargo check --workspace --lib: clean
- cargo clippy -p librefang-channels --all-targets -- -D warnings: clean
- cargo test -p librefang-channels (sidecar): 15 passed
  (incl. 18-variant ChannelContent inbound roundtrip, structured-field
  parse, legacy text fallback, outbound send serialization)

* feat(channels): sidecar capability negotiation + method wiring (P2a)

Ready event now carries a declared capability/identity payload; the 12
optional ChannelAdapter methods are wired through the protocol, cap-gated,
degrading to the exact pre-P2 behaviour when a capability is absent.

- SidecarEvent::Ready { params: SidecarReadyParams } — capabilities,
  account_id, suppress_error_responses, notification_recipients,
  header_rules, protocol_version. Bare {"method":"ready"} still parses
  (params defaults). Reader populates Caps, sets account_id once, and
  emits ReadyAck.
- Inbound SidecarEvent::Typing -> TypingEvent fed to typing_events()
  (best-effort try_send); Ok(other) catch-all removed (all variants
  now explicit).
- Wired: send_typing/reaction/interactive/in_thread/streaming(+
  supports_streaming)/fetch_headers_for/typing_events/
  notification_recipients/account_id/suppress_error_responses.
  create_webhook_routes intentionally stays the trait default None
  (axum Router can't cross stdio; HTTP sidecars POST back via stdout).
- send_command refactored onto a shared free fn write_command so the
  reader can emit ReadyAck without &self.
- fetch_headers_for matches the URL host exactly against declared
  header_rules before emitting auth (token-exfil guard per trait doc).
- SidecarInteractiveParams now carries the full InteractiveMessage
  (P0 placeholder had text-only).
- SidecarEvent::Message left boxed (P1); supervision loop is P2b.

Verification:
- cargo check --workspace --lib: clean
- cargo clippy -p librefang-channels --all-targets -- -D warnings: clean
- cargo test -p librefang-channels (sidecar): 18 passed
  (ready w/ caps + bare-ready default, url_host, cap-gated defaults,
  echo integration still green => ReadyAck ignored by legacy adapter)

* feat(channels): sidecar subprocess supervision loop (P2b)

start() no longer spawns the child inline; it builds a SpawnCtx and a
supervisor task that owns a (re)spawn loop feeding a single long-lived
mpsc the returned stream wraps — so the stream survives child restarts.

- spawn_once(): one child spawn + stdin/stdout/stderr wiring + reader
  task returning ReaderExit (ChildClosed / Shutdown / ReceiverGone)
  and a oneshot that fires on the first `ready`.
- Supervisor: exponential backoff + dependency-free wall-clock jitter
  (<=20%), attempt reset after RESTART_RESET_AFTER_SECS stable uptime,
  circuit-break after RESTART_MAX_RETRIES (single error log, then the
  supervisor exits — no crash-loop spam), ready/ack bounded by
  READY_TIMEOUT_SECS (a child that never announces counts as a failed
  try and is killed). Never restarts on clean Shutdown or once the
  bridge dropped the stream (tx closed). Backoff sleeps are
  shutdown-interruptible.
- Backpressure unchanged in shape: bounded mpsc(MSG_BUFFER) + the
  reader's tx.send().await applies end-to-end backpressure.
- stop() graceful-wait is now SHUTDOWN_GRACE_SECS (was a hardcoded 2s);
  kill_on_drop stays the backstop.
- All tunables are compile-time consts here; P3 promotes them to
  per-adapter SidecarChannelConfig fields.

Verification:
- cargo check --workspace --lib: clean
- cargo clippy -p librefang-channels --all-targets -- -D warnings: clean
- cargo test -p librefang-channels (sidecar): 19 passed, incl. new
  test_supervisor_restarts_crashing_child (a child that exits after
  one message is auto-restarted; the same returned stream keeps
  yielding) and the echo integration test now exercising the full
  supervised path.

* feat(channels,types): per-adapter sidecar supervision config (P3)

Promote the P2b compile-time supervision constants to per-adapter
SidecarChannelConfig fields, snapshotted into a SupCfg at construction.

librefang-types:
- SidecarChannelConfig gains restart, restart_initial_backoff_ms,
  restart_max_backoff_ms, restart_max_retries, restart_reset_after_secs,
  ready_timeout_secs, shutdown_grace_secs, message_buffer, overflow.
  Each uses #[serde(default = "fn")] module-level default fns — the
  struct has no Default impl and name/command are required, so
  #[derive(Default)] is not viable (config-field ritual).
- New SidecarOverflowPolicy enum (snake_case, #[default] Block).

librefang-channels:
- SupCfg::from_config snapshots the values (message_buffer clamped to
  >=1 so mpsc::channel never panics on 0). Supervisor/spawn_once/stop
  read SupCfg instead of consts; consts removed.
- overflow wired: Block = backpressure (tx.send().await, unchanged);
  DropNewest = try_send, drop just-arrived msg on Full with a
  rate-limited warn (first + every 100th). Named DropNewest, not
  DropOldest as the phase doc speculated: a tokio mpsc cannot evict
  the oldest entry from the producer side — surfaced rather than
  mis-implemented.
- restart=false now short-circuits every restart path (ready-timeout,
  ChildClosed, spawn error) straight to a clean supervisor exit.

Schema-mirror invariant (#3144): kernel_config_schema.golden.json
regenerated (+420/-6; the -6 is pre-existing unrelated main drift the
header comment already tracks under #4084 — the #[ignore] flip stays
that issue's call, not bundled here). librefang.toml.example documents
the new [[sidecar_channels]] fields with defaults.

Verification:
- cargo check --workspace --lib: clean
- cargo clippy -p librefang-channels -p librefang-types --all-targets
  -- -D warnings: clean
- cargo test -p librefang-channels (sidecar): 20 passed (incl. new
  test_supcfg_defaults_and_overflow_parsing covering serde defaults,
  snake_case overflow, restart=false, buffer>=1 clamp)
- cargo test -p librefang-types --lib: 847 passed
- golden fixture regenerated via the documented --ignored path

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

* fix(types,channels): restore misattributed SidecarChannelConfig doc; clarify url_host/account_id semantics

The P3 commit inserted `SidecarOverflowPolicy` between the
`SidecarChannelConfig` doc comment and its struct, so the config doc
(incl. the toml example) attached to the enum and `SidecarChannelConfig`
lost its description entirely — visible in the regenerated golden
fixture. Move the doc back onto the struct and give the enum its own.

Also documents two intentional-but-non-obvious behaviours surfaced in
review: `url_host` does not parse IPv6 literals (fail-closed in
`fetch_headers_for`, never a credential leak) and `account_id_cell` is
`OnceLock` so a post-restart `ready` cannot change the account id.
`librefang.toml.example` now notes `message_buffer = 0` is clamped to 1.

Golden fixture regenerated via `--ignored regenerate_golden`; the only
delta is the two corrected descriptions.

* fix(channels): address sidecar reaction-id, typing-events race, streaming thread_id (codex review)

Three P2 correctness issues from automated review:

- Inbound `SidecarMessageParams` gains `message_id`; the reader uses it
  as `platform_message_id` instead of always fabricating a UUID, so
  `send_reaction`/edits target the real platform message. Absent => UUID
  (legacy behaviour).
- `typing_events()` is no longer gated on `has_cap("typing_events")`.
  The bridge calls it synchronously right after `start()`, before the
  async `ready` populates caps, so the gate made it return `None`
  permanently whenever `message_debounce_ms` was set. The receiver is
  now handed out unconditionally; the reader only forwards `Typing`
  events the sidecar actually emits.
- `send_streaming` no longer discards its `thread_id`: it is threaded
  into `SidecarStreamStartParams` (skipped when absent) and the
  no-streaming-cap fallback routes through `send_in_thread` so threaded
  replies keep thread context.

Tests: +message_id parse, +stream_start thread_id (de)serialization;
cap-gated test updated to assert the receiver is now exposed regardless
of caps. 22 sidecar tests pass; clippy clean; workspace lib check clean.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M 50-249 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant