feat(types): KernelConfig.parallel_tools section (PR-3/6)#3144
Merged
Conversation
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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Apr 25, 2026
Merged
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
4 tasks
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
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
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.
2 tasks
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).
3 tasks
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.
This was referenced Apr 25, 2026
Merged
4 tasks
This was referenced May 14, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the
[parallel_tools]configuration section toKernelConfiginlibrefang-types. Schema only — runtime behaviour is unchanged becausenothing in the agent loop reads the new field yet (PR-4 wires the
dispatcher; PR-5 flips the default to
enabled = true).Changes
ParallelToolsConfigstruct with four fields:enabled: bool— master switch, defaultfalse.max_concurrent: u32— bucket-level concurrency cap, default4.0means uncapped.mcp_default_safety: String—"read_only"|"write_shared"fallback for unannotated MCP tools, default
"write_shared"(safeserialised behaviour). Kept as
Stringdeliberately so PR-4 canpromote it to a typed enum in lockstep with the dispatcher.
mcp_readonly_allowlist: Vec<String>— explicit overrides keyedby fully-namespaced MCP tool name (
mcp__server__name).KernelConfig.parallel_tools: ParallelToolsConfigfield (with#[serde(default)]) plus matching entry in theKernelConfig::default()impl, per CLAUDE.md "Config fields added to
KernelConfigstructMUST also be added to the Default impl".
config::types::tests:parallel_tools_default_is_disabled— defaults match the spec.parallel_tools_serde_round_trip— TOML + JSON round-trip with afully-populated struct.
parallel_tools_missing_in_kernel_config_uses_default— oldpre-PR-3
config.tomlfiles (no[parallel_tools]section) hydratethe field from
Default.parallel_tools_partial_section_fills_remaining_with_default—enabled = truealone deserialises with the rest fromDefault,verifying
#[serde(default)]on the struct itself.Non-goals
parallel_dispatch/tool_classifiercodetouched. Runtime behaviour is byte-for-byte identical.
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
partial section.
KernelConfig::default()includes the new field (assertion inparallel_tools_default_is_disabled).site (per CLAUDE.md guidance: integration tests at injection
site, not just implementation site).