From 54c9924b0372c4a786d0a3bccfaca06a238eba57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20LIARD?= Date: Sun, 26 Apr 2026 18:23:06 +0200 Subject: [PATCH] feat(rpc): impl config/set with in-memory mutation (#228) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the `TODO(#228)` stub in `server::rpc::config_ns::set` with a working in-memory mutation, mirroring the validated MCP path (`server::mcp_handlers::config::apply_config_update`) but without disk persistence — that remains explicitly out of scope per #228 ("Non-goal: persistence in config.toml"). Behaviour: - Clone the active config snapshot so validation failures leave the running registry untouched. - Reject denied sections/keys (`providers`, `dlp`, any `api_key`) via the existing `config_guard::is_section_or_key_denied`. - Validate dotted keys (`section.field`) and value types against the target field; surface failures as `INVALID_PARAMS_CODE`. - On success, build a new `Router` + `ProviderRegistry` from the mutated config and atomic-swap the `ReloadableState` (same primitive used by `config_guard::reload_state`). In-flight requests finish on the old snapshot. # Errors - `ERR_FORBIDDEN` when the caller is below `Admin`. - `INVALID_PARAMS_CODE` for malformed keys, denied targets, or wrong value types. - `ERR_INTERNAL` when the registry rebuild or atomic swap fails. Tests: 15/15 unit tests pass (success paths for router/cache, denial paths for providers/dlp/api_key, malformed-key rejection, unknown section/key, role denial for Observer). Implements P1 of the cli-forge-chef brigade plan for issue #228. The remaining stubs (tools_ns enable/disable, pledge_ns set/clear, hit_ns set_policy/resolve) follow the same pattern in P2/P3/P4. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/server/rpc/config_ns.rs | 322 +++++++++++++++++++++++++++++++++++- 1 file changed, 314 insertions(+), 8 deletions(-) diff --git a/src/server/rpc/config_ns.rs b/src/server/rpc/config_ns.rs index d57dd228..af385810 100644 --- a/src/server/rpc/config_ns.rs +++ b/src/server/rpc/config_ns.rs @@ -2,7 +2,12 @@ use super::auth::{require_role, CallerIdentity}; use super::types::{rpc_err, Role, StatusResponse, ERR_INTERNAL}; -use crate::server::AppState; +use crate::models::config::AppConfig; +use crate::providers::ProviderRegistry; +use crate::routing::classify::Router; +use crate::server::config_guard::is_section_or_key_denied; +use crate::server::{AppState, ReloadableState}; +use jsonrpsee::types::error::INVALID_PARAMS_CODE; use jsonrpsee::types::ErrorObjectOwned; use std::sync::Arc; @@ -30,22 +35,59 @@ pub async fn get( } } -/// Sets a configuration key (in-memory only, does not persist to disk). +/// Applies a key/value mutation to the running config (in-memory only). +/// +/// The change survives until the next config reload from disk. +/// Persistence is explicitly out of scope (#228). +/// +/// # Errors +/// +/// Returns `ERR_FORBIDDEN` (insufficient role) when the caller is below `Admin`. +/// Returns `INVALID_PARAMS_CODE` when `key` is malformed (no dot separator), +/// targets a denied section/key (`providers`, `dlp`, any `api_key`), or the +/// value type does not match the field. +/// Returns `ERR_INTERNAL` when the atomic config swap fails. pub async fn set( state: &Arc, caller: &CallerIdentity, - _key: &str, - _value: &serde_json::Value, + key: &str, + value: &serde_json::Value, ) -> Result { require_role(caller, Role::Admin)?; - // TODO(#228): Implement in-memory config mutation with validation. - // Phase 2 will add persistence and diff tracking. - let _ = state; + // Clone the active config so a validation failure leaves the snapshot intact. + let mut new_config = state.snapshot().config.clone(); + apply_runtime_update(&mut new_config, key, value)?; + + // Rebuild the reloadable state from the mutated config and swap atomically. + // Mirrors the pattern in `server_ns::reload_config` and + // `config_guard::reload_state`. We deliberately do NOT call + // `config_guard::persist_and_reload`: persistence to disk is a non-goal + // for #228 (in-memory mutation only). + let new_router = Router::new(new_config.clone()); + let new_registry = ProviderRegistry::from_configs_with_models( + &new_config.providers, + Some(state.token_store.clone()), + &new_config.models, + &new_config.server.timeouts, + ) + .map(Arc::new) + .map_err(|e| rpc_err(ERR_INTERNAL, format!("Failed to rebuild providers: {e}")))?; + + let new_inner = Arc::new(ReloadableState::new(new_config, new_router, new_registry)); + *state.inner.write().unwrap_or_else(|e| e.into_inner()) = new_inner; + + tracing::info!( + caller_ip = %caller.ip, + key = key, + "RPC config/set applied (in-memory only)" + ); Ok(StatusResponse { status: "ok".into(), - message: Some("Config set (in-memory only — use reload to persist)".into()), + message: Some(format!( + "Set {key} (in-memory only — change reverts on next disk reload)" + )), }) } @@ -110,9 +152,151 @@ fn resolve_dotted_path<'a>( Some(current) } +/// Applies an in-memory mutation to `config` according to a dotted `key`. +/// +/// Splits `key` at the first `.` into `(section, sub_key)`, validates the +/// pair against [`is_section_or_key_denied`], then writes the JSON `value` +/// into the matching field. The mutation contract mirrors +/// [`crate::server::mcp_handlers::config::apply_config_update`] so that the +/// MCP and JSON-RPC self-tuning surfaces stay aligned (per #228 plan). +/// +/// Supported sections: `router`, `budget`, `cache`, `classifier`. The +/// `providers` and `dlp` sections, plus any `api_key` field, are rejected +/// up-front by the deny-list. +/// +/// # Errors +/// +/// Returns an `ErrorObjectOwned` with code `INVALID_PARAMS_CODE` when: +/// - `key` lacks a `.` separator, +/// - the resolved `(section, sub_key)` pair is on the deny-list, +/// - the section name is unknown, +/// - the sub-key is unknown for that section, +/// - the JSON `value` type does not match the destination field. +fn apply_runtime_update( + config: &mut AppConfig, + key: &str, + value: &serde_json::Value, +) -> Result<(), ErrorObjectOwned> { + let invalid = |msg: String| rpc_err(INVALID_PARAMS_CODE, msg); + + let (section, sub_key) = key.split_once('.').ok_or_else(|| { + invalid(format!( + "key must be in dotted form (e.g. 'router.default'); got '{key}'" + )) + })?; + + if is_section_or_key_denied(section, sub_key) { + return Err(invalid(format!( + "section/key '{section}.{sub_key}' is on the deny-list (security policy)" + ))); + } + + match section { + "router" => match sub_key { + "default" => { + config.router.default = value + .as_str() + .ok_or_else(|| invalid("expected string for router.default".into()))? + .to_string(); + } + "background" => config.router.background = value.as_str().map(String::from), + "think" => config.router.think = value.as_str().map(String::from), + "websearch" => config.router.websearch = value.as_str().map(String::from), + "auto_map_regex" => config.router.auto_map_regex = value.as_str().map(String::from), + "background_regex" => config.router.background_regex = value.as_str().map(String::from), + "gdpr" => { + config.router.gdpr = value + .as_bool() + .ok_or_else(|| invalid("expected bool for router.gdpr".into()))?; + } + "region" => config.router.region = value.as_str().map(String::from), + other => return Err(invalid(format!("unknown router key: {other}"))), + }, + "budget" => match sub_key { + "monthly_limit_usd" => { + let v = value.as_f64().ok_or_else(|| { + invalid("expected number for budget.monthly_limit_usd".into()) + })?; + config.budget.monthly_limit_usd = crate::cli::BudgetUsd::new(v) + .map_err(|e| invalid(format!("invalid budget: {e}")))?; + } + "warn_at_percent" => { + let v = value + .as_u64() + .ok_or_else(|| invalid("expected integer for budget.warn_at_percent".into()))?; + if v > 100 { + return Err(invalid("warn_at_percent must be 0-100".into())); + } + config.budget.warn_at_percent = v as u32; + } + other => return Err(invalid(format!("unknown budget key: {other}"))), + }, + "cache" => match sub_key { + "enabled" => { + config.cache.enabled = value + .as_bool() + .ok_or_else(|| invalid("expected bool for cache.enabled".into()))?; + } + "max_capacity" => { + config.cache.max_capacity = value + .as_u64() + .ok_or_else(|| invalid("expected integer for cache.max_capacity".into()))?; + } + "ttl_secs" => { + config.cache.ttl_secs = value + .as_u64() + .ok_or_else(|| invalid("expected integer for cache.ttl_secs".into()))?; + } + "max_entry_bytes" => { + let v = value + .as_u64() + .ok_or_else(|| invalid("expected integer for cache.max_entry_bytes".into()))?; + config.cache.max_entry_bytes = v as usize; + } + other => return Err(invalid(format!("unknown cache key: {other}"))), + }, + "classifier" => { + let cfg = config.classifier.get_or_insert_with(Default::default); + let v = value + .as_f64() + .ok_or_else(|| invalid(format!("expected number for classifier.{sub_key}")))? + as f32; + match sub_key { + "weights.max_tokens" => cfg.weights.max_tokens = v, + "weights.tools" => cfg.weights.tools = v, + "weights.context_size" => cfg.weights.context_size = v, + "weights.keywords" => cfg.weights.keywords = v, + "weights.system_prompt" => cfg.weights.system_prompt = v, + "thresholds.medium_threshold" => cfg.thresholds.medium_threshold = v, + "thresholds.complex_threshold" => cfg.thresholds.complex_threshold = v, + other => return Err(invalid(format!("unknown classifier key: {other}"))), + } + } + other => return Err(invalid(format!("unknown config section: {other}"))), + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; + use crate::server::rpc::types::ERR_FORBIDDEN; + + /// Parses a minimal AppConfig from a TOML snippet (test fixture). + fn fixture_config() -> AppConfig { + let toml = r#" +[router] +default = "claude-sonnet-4-6" + +[cache] +enabled = false +max_capacity = 100 +ttl_secs = 60 +max_entry_bytes = 8192 +"#; + toml::from_str(toml).expect("valid test TOML") + } #[test] fn resolve_dotted_path_simple() { @@ -145,4 +329,126 @@ mod tests { assert!(result.is_some()); assert!(result.unwrap().is_array()); } + + // ── apply_runtime_update — mutation path ───────────────────────── + + #[test] + fn set_router_default_succeeds_for_admin() { + // Mirrors the chef-spec test: an Admin caller setting `router.default` + // results in the config field being mutated. The role check is verified + // separately in `set_denies_for_observer`; here we exercise the mutation. + let mut config = fixture_config(); + apply_runtime_update( + &mut config, + "router.default", + &serde_json::json!("new-default"), + ) + .expect("router.default update should succeed"); + assert_eq!(config.router.default, "new-default"); + } + + #[test] + fn set_cache_ttl_succeeds() { + let mut config = fixture_config(); + apply_runtime_update(&mut config, "cache.ttl_secs", &serde_json::json!(900)) + .expect("cache.ttl_secs update should succeed"); + assert_eq!(config.cache.ttl_secs, 900); + } + + #[test] + fn set_rejects_dlp_section() { + // Bonus chef-spec test: dlp section is on the deny-list, even for Admin. + let mut config = fixture_config(); + let err = apply_runtime_update(&mut config, "dlp.enabled", &serde_json::json!(true)) + .expect_err("dlp section is denied"); + assert_eq!(err.code(), INVALID_PARAMS_CODE); + } + + #[test] + fn set_rejects_providers_section() { + let mut config = fixture_config(); + let err = apply_runtime_update( + &mut config, + "providers.name", + &serde_json::json!("anthropic"), + ) + .expect_err("providers section is denied"); + assert_eq!(err.code(), INVALID_PARAMS_CODE); + } + + #[test] + fn set_rejects_api_key_field() { + let mut config = fixture_config(); + let err = apply_runtime_update(&mut config, "router.api_key", &serde_json::json!("secret")) + .expect_err("api_key fields are denied"); + assert_eq!(err.code(), INVALID_PARAMS_CODE); + } + + #[test] + fn set_rejects_malformed_key() { + let mut config = fixture_config(); + let err = apply_runtime_update(&mut config, "noseparator", &serde_json::json!("x")) + .expect_err("missing '.' separator should fail"); + assert_eq!(err.code(), INVALID_PARAMS_CODE); + } + + #[test] + fn set_rejects_unknown_section() { + let mut config = fixture_config(); + let err = apply_runtime_update(&mut config, "bogus.field", &serde_json::json!("x")) + .expect_err("unknown section should fail"); + assert_eq!(err.code(), INVALID_PARAMS_CODE); + } + + #[test] + fn set_rejects_unknown_router_key() { + let mut config = fixture_config(); + let err = apply_runtime_update(&mut config, "router.bogus", &serde_json::json!("x")) + .expect_err("unknown router key should fail"); + assert_eq!(err.code(), INVALID_PARAMS_CODE); + } + + #[test] + fn set_rejects_wrong_value_type() { + let mut config = fixture_config(); + let err = apply_runtime_update(&mut config, "router.default", &serde_json::json!(42)) + .expect_err("router.default expects a string"); + assert_eq!(err.code(), INVALID_PARAMS_CODE); + } + + // ── role enforcement (the entry point of `set`) ────────────────── + + #[test] + fn set_denies_for_observer() { + // `set` calls `require_role(caller, Role::Admin)` before any state + // access — so an Observer caller is rejected with `ERR_FORBIDDEN` + // (the canonical "insufficient role" code in this repo) regardless of + // the key/value. We assert the role-check contract directly to avoid + // pulling a full `AppState` into a unit test. + let observer = CallerIdentity { + role: Role::Observer, + ip: "10.0.0.1".into(), + tenant_id: String::new(), + }; + let err = require_role(&observer, Role::Admin).expect_err("Observer < Admin"); + assert_eq!(err.code(), ERR_FORBIDDEN); + + // Sanity: Operator is also below Admin. + let operator = CallerIdentity { + role: Role::Operator, + ip: "10.0.0.1".into(), + tenant_id: String::new(), + }; + assert!(require_role(&operator, Role::Admin).is_err()); + } + + #[test] + fn set_admin_passes_role_check() { + let admin = CallerIdentity { + role: Role::Admin, + ip: "10.0.0.1".into(), + tenant_id: String::new(), + }; + assert!(require_role(&admin, Role::Admin).is_ok()); + } }