From e74214dce8fe6013b8a9a8dd02fd13cacf263131 Mon Sep 17 00:00:00 2001 From: Reid <61492567+reidliu41@users.noreply.github.com> Date: Sun, 15 Mar 2026 13:59:08 +0800 Subject: [PATCH] fix(config): unify ChannelsConfig resolution to env > settings > default (#1124) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChannelsConfig::resolve() ignored most ChannelSettings fields, reading exclusively from env vars. This made `config set` ineffective for gateway, HTTP, CLI, and WASM channel settings — a prerequisite blocker for #86 (hot-reload) and CLI management commands. - Add gateway and CLI fields to ChannelSettings with correct defaults - Rewrite resolve() to fall back to settings when env var is unset - Keep strict boolean validation via parse_bool_env for all bool fields - Fix GATEWAY_PORT default divergence (3001 -> 3000) in extension manager - Export DEFAULT_GATEWAY_PORT constant as single source of truth - Add 8 tests: settings fallback, env override, DB roundtrip, invalid bool rejection Part of #1119 (Phase 1: Channels pilot) [skip-regression-check] --- src/config/channels.rs | 342 ++++++++++++++++++++++++++++++++++---- src/config/mod.rs | 4 +- src/extensions/manager.rs | 3 +- src/settings.rs | 54 +++++- 4 files changed, 367 insertions(+), 36 deletions(-) diff --git a/src/config/channels.rs b/src/config/channels.rs index 90635c22..981b0170 100644 --- a/src/config/channels.rs +++ b/src/config/channels.rs @@ -91,11 +91,20 @@ pub struct SignalConfig { } impl ChannelsConfig { + /// Resolve channels config following `env > settings > default` for every field. pub(crate) fn resolve(settings: &Settings) -> Result { - let http = if optional_env("HTTP_PORT")?.is_some() || optional_env("HTTP_HOST")?.is_some() { + let cs = &settings.channels; + + // --- HTTP webhook --- + // HTTP is enabled when env vars are set OR settings has it enabled. + let http_enabled_by_env = + optional_env("HTTP_PORT")?.is_some() || optional_env("HTTP_HOST")?.is_some(); + let http = if http_enabled_by_env || cs.http_enabled { Some(HttpConfig { - host: optional_env("HTTP_HOST")?.unwrap_or_else(|| "0.0.0.0".to_string()), - port: parse_optional_env("HTTP_PORT", 8080)?, + host: optional_env("HTTP_HOST")? + .or_else(|| cs.http_host.clone()) + .unwrap_or_else(|| "0.0.0.0".to_string()), + port: parse_optional_env("HTTP_PORT", cs.http_port.unwrap_or(8080))?, webhook_secret: optional_env("HTTP_WEBHOOK_SECRET")?.map(SecretString::from), user_id: optional_env("HTTP_USER_ID")?.unwrap_or_else(|| "http".to_string()), }) @@ -103,42 +112,58 @@ impl ChannelsConfig { None }; - let gateway_enabled = parse_bool_env("GATEWAY_ENABLED", true)?; + // --- Web gateway --- + let gateway_enabled = parse_bool_env("GATEWAY_ENABLED", cs.gateway_enabled)?; let gateway = if gateway_enabled { Some(GatewayConfig { - host: optional_env("GATEWAY_HOST")?.unwrap_or_else(|| "127.0.0.1".to_string()), - port: parse_optional_env("GATEWAY_PORT", 3000)?, - auth_token: optional_env("GATEWAY_AUTH_TOKEN")?, - user_id: optional_env("GATEWAY_USER_ID")?.unwrap_or_else(|| "default".to_string()), + host: optional_env("GATEWAY_HOST")? + .or_else(|| cs.gateway_host.clone()) + .unwrap_or_else(|| "127.0.0.1".to_string()), + port: parse_optional_env( + "GATEWAY_PORT", + cs.gateway_port.unwrap_or(DEFAULT_GATEWAY_PORT), + )?, + auth_token: optional_env("GATEWAY_AUTH_TOKEN")? + .or_else(|| cs.gateway_auth_token.clone()), + user_id: optional_env("GATEWAY_USER_ID")? + .or_else(|| cs.gateway_user_id.clone()) + .unwrap_or_else(|| "default".to_string()), }) } else { None }; - let signal = if let Some(http_url) = optional_env("SIGNAL_HTTP_URL")? { - let account = optional_env("SIGNAL_ACCOUNT")?.ok_or(ConfigError::InvalidValue { - key: "SIGNAL_ACCOUNT".to_string(), - message: "SIGNAL_ACCOUNT is required when SIGNAL_HTTP_URL is set".to_string(), - })?; - let allow_from = match std::env::var_os("SIGNAL_ALLOW_FROM") { + // --- Signal --- + let signal_url = optional_env("SIGNAL_HTTP_URL")?.or_else(|| cs.signal_http_url.clone()); + let signal = if let Some(http_url) = signal_url { + let account = optional_env("SIGNAL_ACCOUNT")? + .or_else(|| cs.signal_account.clone()) + .ok_or(ConfigError::InvalidValue { + key: "SIGNAL_ACCOUNT".to_string(), + message: "SIGNAL_ACCOUNT is required when Signal is enabled".to_string(), + })?; + let allow_from_str = + optional_env("SIGNAL_ALLOW_FROM")?.or_else(|| cs.signal_allow_from.clone()); + let allow_from = match allow_from_str { None => vec![account.clone()], - Some(val) => { - let s = val.to_string_lossy(); - s.split(',') - .map(|e| e.trim().to_string()) - .filter(|s| !s.is_empty()) - .collect() - } + Some(s) => s + .split(',') + .map(|e| e.trim().to_string()) + .filter(|s| !s.is_empty()) + .collect(), }; - let dm_policy = - optional_env("SIGNAL_DM_POLICY")?.unwrap_or_else(|| "pairing".to_string()); - let group_policy = - optional_env("SIGNAL_GROUP_POLICY")?.unwrap_or_else(|| "allowlist".to_string()); + let dm_policy = optional_env("SIGNAL_DM_POLICY")? + .or_else(|| cs.signal_dm_policy.clone()) + .unwrap_or_else(|| "pairing".to_string()); + let group_policy = optional_env("SIGNAL_GROUP_POLICY")? + .or_else(|| cs.signal_group_policy.clone()) + .unwrap_or_else(|| "allowlist".to_string()); Some(SignalConfig { http_url, account, allow_from, allow_from_groups: optional_env("SIGNAL_ALLOW_FROM_GROUPS")? + .or_else(|| cs.signal_allow_from_groups.clone()) .map(|s| { s.split(',') .map(|e| e.trim().to_string()) @@ -149,6 +174,7 @@ impl ChannelsConfig { dm_policy, group_policy, group_allow_from: optional_env("SIGNAL_GROUP_ALLOW_FROM")? + .or_else(|| cs.signal_group_allow_from.clone()) .map(|s| { s.split(',') .map(|e| e.trim().to_string()) @@ -167,9 +193,17 @@ impl ChannelsConfig { None }; - let cli_enabled = optional_env("CLI_ENABLED")? - .map(|s| s.to_lowercase() != "false" && s != "0") - .unwrap_or(true); + // --- CLI --- + let cli_enabled = parse_bool_env("CLI_ENABLED", cs.cli_enabled)?; + + // --- WASM channels --- + let wasm_channels_dir = optional_env("WASM_CHANNELS_DIR")? + .map(PathBuf::from) + .or_else(|| cs.wasm_channels_dir.clone()) + .unwrap_or_else(default_channels_dir); + + let wasm_channels_enabled = + parse_bool_env("WASM_CHANNELS_ENABLED", cs.wasm_channels_enabled)?; Ok(Self { cli: CliConfig { @@ -178,12 +212,10 @@ impl ChannelsConfig { http, gateway, signal, - wasm_channels_dir: optional_env("WASM_CHANNELS_DIR")? - .map(PathBuf::from) - .unwrap_or_else(default_channels_dir), - wasm_channels_enabled: parse_bool_env("WASM_CHANNELS_ENABLED", true)?, + wasm_channels_dir, + wasm_channels_enabled, wasm_channel_owner_ids: { - let mut ids = settings.channels.wasm_channel_owner_ids.clone(); + let mut ids = cs.wasm_channel_owner_ids.clone(); // Backwards compat: TELEGRAM_OWNER_ID env var if let Some(id_str) = optional_env("TELEGRAM_OWNER_ID")? { let id: i64 = id_str.parse().map_err(|e: std::num::ParseIntError| { @@ -200,6 +232,10 @@ impl ChannelsConfig { } } +/// Default gateway port — used both in `resolve()` and as the fallback in +/// other modules that need to construct a gateway URL. +pub const DEFAULT_GATEWAY_PORT: u16 = 3000; + /// Get the default channels directory (~/.ironclaw/channels/). fn default_channels_dir() -> PathBuf { ironclaw_base_dir().join("channels") @@ -362,4 +398,244 @@ mod tests { "expected path ending in 'channels', got: {dir:?}" ); } + + #[test] + fn default_gateway_port_constant() { + assert_eq!(DEFAULT_GATEWAY_PORT, 3000); + } + + /// With default settings and no env vars, gateway should use defaults. + #[test] + fn resolve_gateway_defaults_from_settings() { + let _lock = crate::config::helpers::ENV_MUTEX.lock(); + // Clear env vars that would interfere + unsafe { + std::env::remove_var("GATEWAY_ENABLED"); + std::env::remove_var("GATEWAY_HOST"); + std::env::remove_var("GATEWAY_PORT"); + std::env::remove_var("GATEWAY_AUTH_TOKEN"); + std::env::remove_var("GATEWAY_USER_ID"); + std::env::remove_var("HTTP_PORT"); + std::env::remove_var("HTTP_HOST"); + std::env::remove_var("SIGNAL_HTTP_URL"); + std::env::remove_var("CLI_ENABLED"); + std::env::remove_var("WASM_CHANNELS_DIR"); + std::env::remove_var("WASM_CHANNELS_ENABLED"); + std::env::remove_var("TELEGRAM_OWNER_ID"); + } + + let settings = crate::settings::Settings::default(); + let cfg = ChannelsConfig::resolve(&settings).unwrap(); + + let gw = cfg.gateway.expect("gateway should be enabled by default"); + assert_eq!(gw.host, "127.0.0.1"); + assert_eq!(gw.port, DEFAULT_GATEWAY_PORT); + assert!(gw.auth_token.is_none()); + assert_eq!(gw.user_id, "default"); + } + + /// Settings values should be used when no env vars are set. + #[test] + fn resolve_gateway_from_settings() { + let _lock = crate::config::helpers::ENV_MUTEX.lock(); + unsafe { + std::env::remove_var("GATEWAY_ENABLED"); + std::env::remove_var("GATEWAY_HOST"); + std::env::remove_var("GATEWAY_PORT"); + std::env::remove_var("GATEWAY_AUTH_TOKEN"); + std::env::remove_var("GATEWAY_USER_ID"); + std::env::remove_var("HTTP_PORT"); + std::env::remove_var("HTTP_HOST"); + std::env::remove_var("SIGNAL_HTTP_URL"); + std::env::remove_var("CLI_ENABLED"); + std::env::remove_var("WASM_CHANNELS_DIR"); + std::env::remove_var("WASM_CHANNELS_ENABLED"); + std::env::remove_var("TELEGRAM_OWNER_ID"); + } + + let mut settings = crate::settings::Settings::default(); + settings.channels.gateway_port = Some(4000); + settings.channels.gateway_host = Some("0.0.0.0".to_string()); + settings.channels.gateway_auth_token = Some("db-token-123".to_string()); + settings.channels.gateway_user_id = Some("myuser".to_string()); + + let cfg = ChannelsConfig::resolve(&settings).unwrap(); + let gw = cfg.gateway.expect("gateway should be enabled"); + assert_eq!(gw.port, 4000); + assert_eq!(gw.host, "0.0.0.0"); + assert_eq!(gw.auth_token.as_deref(), Some("db-token-123")); + assert_eq!(gw.user_id, "myuser"); + } + + /// Env vars should override settings values. + #[test] + fn resolve_env_overrides_settings() { + let _lock = crate::config::helpers::ENV_MUTEX.lock(); + unsafe { + std::env::set_var("GATEWAY_PORT", "5000"); + std::env::set_var("GATEWAY_HOST", "10.0.0.1"); + std::env::set_var("GATEWAY_AUTH_TOKEN", "env-token"); + std::env::remove_var("GATEWAY_ENABLED"); + std::env::remove_var("GATEWAY_USER_ID"); + std::env::remove_var("HTTP_PORT"); + std::env::remove_var("HTTP_HOST"); + std::env::remove_var("SIGNAL_HTTP_URL"); + std::env::remove_var("CLI_ENABLED"); + std::env::remove_var("WASM_CHANNELS_DIR"); + std::env::remove_var("WASM_CHANNELS_ENABLED"); + std::env::remove_var("TELEGRAM_OWNER_ID"); + } + + let mut settings = crate::settings::Settings::default(); + settings.channels.gateway_port = Some(4000); + settings.channels.gateway_host = Some("0.0.0.0".to_string()); + settings.channels.gateway_auth_token = Some("db-token".to_string()); + + let cfg = ChannelsConfig::resolve(&settings).unwrap(); + let gw = cfg.gateway.expect("gateway should be enabled"); + assert_eq!(gw.port, 5000, "env should override settings"); + assert_eq!(gw.host, "10.0.0.1", "env should override settings"); + assert_eq!( + gw.auth_token.as_deref(), + Some("env-token"), + "env should override settings" + ); + + // Cleanup + unsafe { + std::env::remove_var("GATEWAY_PORT"); + std::env::remove_var("GATEWAY_HOST"); + std::env::remove_var("GATEWAY_AUTH_TOKEN"); + } + } + + /// CLI enabled should fall back to settings. + #[test] + fn resolve_cli_enabled_from_settings() { + let _lock = crate::config::helpers::ENV_MUTEX.lock(); + unsafe { + std::env::remove_var("CLI_ENABLED"); + std::env::remove_var("GATEWAY_ENABLED"); + std::env::remove_var("GATEWAY_HOST"); + std::env::remove_var("GATEWAY_PORT"); + std::env::remove_var("GATEWAY_AUTH_TOKEN"); + std::env::remove_var("GATEWAY_USER_ID"); + std::env::remove_var("HTTP_PORT"); + std::env::remove_var("HTTP_HOST"); + std::env::remove_var("SIGNAL_HTTP_URL"); + std::env::remove_var("WASM_CHANNELS_DIR"); + std::env::remove_var("WASM_CHANNELS_ENABLED"); + std::env::remove_var("TELEGRAM_OWNER_ID"); + } + + let mut settings = crate::settings::Settings::default(); + settings.channels.cli_enabled = false; + + let cfg = ChannelsConfig::resolve(&settings).unwrap(); + assert!(!cfg.cli.enabled, "settings should disable CLI"); + } + + /// HTTP channel should activate when settings has it enabled. + #[test] + fn resolve_http_from_settings() { + let _lock = crate::config::helpers::ENV_MUTEX.lock(); + unsafe { + std::env::remove_var("HTTP_PORT"); + std::env::remove_var("HTTP_HOST"); + std::env::remove_var("HTTP_WEBHOOK_SECRET"); + std::env::remove_var("HTTP_USER_ID"); + std::env::remove_var("GATEWAY_ENABLED"); + std::env::remove_var("GATEWAY_HOST"); + std::env::remove_var("GATEWAY_PORT"); + std::env::remove_var("GATEWAY_AUTH_TOKEN"); + std::env::remove_var("GATEWAY_USER_ID"); + std::env::remove_var("SIGNAL_HTTP_URL"); + std::env::remove_var("CLI_ENABLED"); + std::env::remove_var("WASM_CHANNELS_DIR"); + std::env::remove_var("WASM_CHANNELS_ENABLED"); + std::env::remove_var("TELEGRAM_OWNER_ID"); + } + + let mut settings = crate::settings::Settings::default(); + settings.channels.http_enabled = true; + settings.channels.http_port = Some(9090); + settings.channels.http_host = Some("10.0.0.1".to_string()); + + let cfg = ChannelsConfig::resolve(&settings).unwrap(); + let http = cfg.http.expect("HTTP should be enabled from settings"); + assert_eq!(http.port, 9090); + assert_eq!(http.host, "10.0.0.1"); + } + + /// Settings round-trip through DB map for new gateway fields. + #[test] + fn settings_gateway_fields_db_roundtrip() { + let mut settings = crate::settings::Settings::default(); + settings.channels.gateway_port = Some(4000); + settings.channels.gateway_host = Some("0.0.0.0".to_string()); + settings.channels.gateway_auth_token = Some("tok-abc".to_string()); + settings.channels.gateway_user_id = Some("myuser".to_string()); + settings.channels.cli_enabled = false; + + let map = settings.to_db_map(); + let restored = crate::settings::Settings::from_db_map(&map); + + assert_eq!(restored.channels.gateway_port, Some(4000)); + assert_eq!(restored.channels.gateway_host.as_deref(), Some("0.0.0.0")); + assert_eq!( + restored.channels.gateway_auth_token.as_deref(), + Some("tok-abc") + ); + assert_eq!(restored.channels.gateway_user_id.as_deref(), Some("myuser")); + assert!(!restored.channels.cli_enabled); + } + + /// Invalid boolean env values must produce errors, not silently degrade. + #[test] + fn resolve_rejects_invalid_bool_env() { + let _lock = crate::config::helpers::ENV_MUTEX.lock(); + let settings = crate::settings::Settings::default(); + + // GATEWAY_ENABLED=maybe should error + unsafe { + std::env::set_var("GATEWAY_ENABLED", "maybe"); + std::env::remove_var("HTTP_PORT"); + std::env::remove_var("HTTP_HOST"); + std::env::remove_var("SIGNAL_HTTP_URL"); + std::env::remove_var("CLI_ENABLED"); + std::env::remove_var("WASM_CHANNELS_ENABLED"); + std::env::remove_var("GATEWAY_PORT"); + std::env::remove_var("GATEWAY_HOST"); + std::env::remove_var("GATEWAY_AUTH_TOKEN"); + std::env::remove_var("GATEWAY_USER_ID"); + std::env::remove_var("WASM_CHANNELS_DIR"); + std::env::remove_var("TELEGRAM_OWNER_ID"); + } + let result = ChannelsConfig::resolve(&settings); + assert!(result.is_err(), "GATEWAY_ENABLED=maybe should be rejected"); + + // CLI_ENABLED=on should error + unsafe { + std::env::remove_var("GATEWAY_ENABLED"); + std::env::set_var("CLI_ENABLED", "on"); + } + let result = ChannelsConfig::resolve(&settings); + assert!(result.is_err(), "CLI_ENABLED=on should be rejected"); + + // WASM_CHANNELS_ENABLED=yes should error + unsafe { + std::env::remove_var("CLI_ENABLED"); + std::env::set_var("WASM_CHANNELS_ENABLED", "yes"); + } + let result = ChannelsConfig::resolve(&settings); + assert!( + result.is_err(), + "WASM_CHANNELS_ENABLED=yes should be rejected" + ); + + // Cleanup + unsafe { + std::env::remove_var("WASM_CHANNELS_ENABLED"); + } + } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 34c34423..0ce8dfec 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -34,7 +34,9 @@ use crate::settings::Settings; // Re-export all public types so `crate::config::FooConfig` continues to work. pub use self::agent::AgentConfig; pub use self::builder::BuilderModeConfig; -pub use self::channels::{ChannelsConfig, CliConfig, GatewayConfig, HttpConfig, SignalConfig}; +pub use self::channels::{ + ChannelsConfig, CliConfig, DEFAULT_GATEWAY_PORT, GatewayConfig, HttpConfig, SignalConfig, +}; pub use self::database::{DatabaseBackend, DatabaseConfig, SslMode, default_libsql_path}; pub use self::embeddings::EmbeddingsConfig; pub use self::heartbeat::HeartbeatConfig; diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index f3358f34..e057e2ac 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -3451,7 +3451,8 @@ impl ExtensionManager { .or_else(|| relay_config.callback_url.clone()) .unwrap_or_else(|| { let host = std::env::var("GATEWAY_HOST").unwrap_or_else(|_| "127.0.0.1".into()); - let port = std::env::var("GATEWAY_PORT").unwrap_or_else(|_| "3001".into()); + let port = std::env::var("GATEWAY_PORT") + .unwrap_or_else(|_| crate::config::DEFAULT_GATEWAY_PORT.to_string()); format!("http://{}:{}", host, port) }); diff --git a/src/settings.rs b/src/settings.rs index 482291b6..29bfbae1 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -220,7 +220,7 @@ pub struct TunnelSettings { } /// Channel-specific settings. -#[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct ChannelSettings { /// Whether HTTP webhook channel is enabled. #[serde(default)] @@ -234,6 +234,30 @@ pub struct ChannelSettings { #[serde(default)] pub http_host: Option, + /// Whether the web gateway is enabled. + #[serde(default = "default_true")] + pub gateway_enabled: bool, + + /// Web gateway listen host. + #[serde(default)] + pub gateway_host: Option, + + /// Web gateway listen port. + #[serde(default)] + pub gateway_port: Option, + + /// Web gateway bearer auth token. Auto-generated at gateway startup if unset. + #[serde(default)] + pub gateway_auth_token: Option, + + /// Web gateway user ID. + #[serde(default)] + pub gateway_user_id: Option, + + /// Whether the CLI channel is enabled. + #[serde(default = "default_true")] + pub cli_enabled: bool, + /// Whether Signal channel is enabled. #[serde(default)] pub signal_enabled: bool, @@ -289,6 +313,34 @@ pub struct ChannelSettings { pub wasm_channels_dir: Option, } +impl Default for ChannelSettings { + fn default() -> Self { + Self { + http_enabled: false, + http_port: None, + http_host: None, + gateway_enabled: true, + gateway_host: None, + gateway_port: None, + gateway_auth_token: None, + gateway_user_id: None, + cli_enabled: true, + signal_enabled: false, + signal_http_url: None, + signal_account: None, + signal_allow_from: None, + signal_allow_from_groups: None, + signal_dm_policy: None, + signal_group_policy: None, + signal_group_allow_from: None, + wasm_channel_owner_ids: std::collections::HashMap::new(), + wasm_channels: Vec::new(), + wasm_channels_enabled: true, + wasm_channels_dir: None, + } + } +} + /// Heartbeat configuration. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct HeartbeatSettings {