feat(rpc): impl config/set with in-memory mutation (#228)

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<AppState>,
     caller: &CallerIdentity,
-    _key: &str,
-    _value: &serde_json::Value,
+    key: &str,
+    value: &serde_json::Value,
 ) -> Result<StatusResponse, ErrorObjectOwned> {
     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());
+    }
 }