diff --git a/api/connector_contracts.py b/api/connector_contracts.py index 99c4d13..f934a25 100644 --- a/api/connector_contracts.py +++ b/api/connector_contracts.py @@ -56,10 +56,14 @@ logger = logging.getLogger("ComfyUI-OpenClaw.api.connector_contracts") def _require_admin(request) -> Optional[web.Response]: if not check_rate_limit(request, "admin"): - return web.json_response({"ok": False, "error": "Rate limit exceeded"}, status=429) + return web.json_response( + {"ok": False, "error": "Rate limit exceeded"}, status=429 + ) allowed, err = require_admin_token(request) if not allowed: - return web.json_response({"ok": False, "error": err or "Unauthorized"}, status=403) + return web.json_response( + {"ok": False, "error": err or "Unauthorized"}, status=403 + ) return None @@ -106,7 +110,9 @@ async def connector_installation_get_handler(request): installation = registry.get_installation(installation_id) if installation is None: return web.json_response({"ok": False, "error": "not_found"}, status=404) - return web.json_response({"ok": True, "installation": installation.to_public_dict()}) + return web.json_response( + {"ok": True, "installation": installation.to_public_dict()} + ) @endpoint_metadata( @@ -130,7 +136,10 @@ async def connector_installation_resolve_handler(request): registry = get_connector_installation_registry() resolution = registry.resolve_installation(platform, workspace_id) status_code = 200 if resolution.ok else 409 - return web.json_response({"ok": resolution.ok, "resolution": resolution.to_public_dict()}, status=status_code) + return web.json_response( + {"ok": resolution.ok, "resolution": resolution.to_public_dict()}, + status=status_code, + ) @endpoint_metadata( diff --git a/config.py b/config.py index b6d77d4..c1d0d44 100644 --- a/config.py +++ b/config.py @@ -5,6 +5,33 @@ import time from logging.handlers import RotatingFileHandler from typing import Optional +# R139: centralized env-alias helpers for config surface compatibility. +try: + from .services.config_layers import ( + GENERIC_LLM_API_KEY_ENV_KEYS, + get_first_present_env, + ) +except Exception: + try: + from services.config_layers import ( # type: ignore + GENERIC_LLM_API_KEY_ENV_KEYS, + get_first_present_env, + ) + except Exception: + GENERIC_LLM_API_KEY_ENV_KEYS = ( + "OPENCLAW_LLM_API_KEY", + "MOLTBOT_LLM_API_KEY", + "CLAWDBOT_LLM_API_KEY", + ) + + def get_first_present_env(keys, *, env=None): # type: ignore + env_map = env or os.environ + for key in keys: + if key in env_map: + return env_map.get(key) + return None + + # Pack metadata PACK_NAME = "ComfyUI-OpenClaw" PACK_START_TIME = time.time() @@ -59,9 +86,9 @@ def _read_pyproject_version() -> Optional[str]: PACK_VERSION = _read_pyproject_version() or "0.1.0" # Environment variable for the API key -ENV_API_KEY = "OPENCLAW_LLM_API_KEY" -LEGACY_ENV_API_KEY = "MOLTBOT_LLM_API_KEY" -LEGACY2_ENV_API_KEY = "CLAWDBOT_LLM_API_KEY" +ENV_API_KEY = GENERIC_LLM_API_KEY_ENV_KEYS[0] +LEGACY_ENV_API_KEY = GENERIC_LLM_API_KEY_ENV_KEYS[1] +LEGACY2_ENV_API_KEY = GENERIC_LLM_API_KEY_ENV_KEYS[2] # Data directory (R11: use portable state directory) try: @@ -145,14 +172,8 @@ def get_api_key() -> Optional[str]: 2) (legacy) MOLTBOT_LLM_API_KEY 3) (legacy) CLAWDBOT_LLM_API_KEY """ - # Respect explicit empty string overrides by checking env var presence. - if ENV_API_KEY in os.environ: - return os.environ.get(ENV_API_KEY) or None - if LEGACY_ENV_API_KEY in os.environ: - return os.environ.get(LEGACY_ENV_API_KEY) or None - if LEGACY2_ENV_API_KEY in os.environ: - return os.environ.get(LEGACY2_ENV_API_KEY) or None - return None + value = get_first_present_env(GENERIC_LLM_API_KEY_ENV_KEYS) + return value or None def setup_logger(name: str = "ComfyUI-OpenClaw") -> logging.Logger: diff --git a/docs/adr/ADR-0001-config-surface-unification.md b/docs/adr/ADR-0001-config-surface-unification.md new file mode 100644 index 0000000..b995d05 --- /dev/null +++ b/docs/adr/ADR-0001-config-surface-unification.md @@ -0,0 +1,58 @@ +# ADR-0001: Configuration Surface Unification (R139) + +- Status: Accepted +- Date: 2026-03-07 +- Owners: OpenClaw maintainers +- Related roadmap item: `R139` + +## Context + +OpenClaw currently has distributed configuration logic across `config.py`, `services/runtime_config.py`, and selected call sites that still read env vars directly. This increases precedence drift risk and makes behavior harder to reason about. + +`R139` requires a phased, backward-compatible unification, not a single destructive rewrite. + +## Decision + +Adopt one authoritative layered model for runtime LLM config resolution, exposed through a unified resolver and consumed by `services/runtime_config.py` compatibility APIs. + +Layer precedence (highest to lowest): +1. `env` (`OPENCLAW_*` first, `MOLTBOT_*` fallback) +2. `runtime_override` (in-memory only; process-local) +3. `persisted` (`OPENCLAW_STATE_DIR/config.json`) +4. `default` + +Key points: +- Keep env-first semantics for operational safety and backward compatibility. +- Preserve legacy key support with explicit warning behavior. +- Keep runtime overrides non-persisted and source-attributed. + +## Consequences + +Positive: +- Deterministic precedence and source attribution. +- Reduced duplicated merge logic in primary runtime paths. +- Safer phased migration with compatibility facade intact. + +Trade-offs: +- Temporary coexistence of migrated and non-migrated call sites during phased rollout. +- Additional adapter code until follow-up phases complete. + +## Rollout Plan + +Phase 1 (`R139`): +- Introduce unified resolver + runtime override registry. +- Refactor `services/runtime_config.py` effective-read path to resolver-backed flow. +- Migrate core LLM call sites to stop duplicating env precedence. +- Add precedence/compatibility regression tests. + +Phase 2+ (future follow-ups): +- Continue migrating remaining direct env readers where they overlap with runtime config contract. +- Remove obsolete adapter/shim code once migration reaches stable completion. + +## Rejected Alternatives + +1. Big-bang rewrite of all config readers: + - Rejected due to blast radius and rollback difficulty. +2. Keep dual systems and patch ad hoc: + - Rejected due to ongoing precedence drift and maintenance cost. + diff --git a/docs/release/config_secrets_contract.md b/docs/release/config_secrets_contract.md index 2336302..a62b7f7 100644 --- a/docs/release/config_secrets_contract.md +++ b/docs/release/config_secrets_contract.md @@ -10,7 +10,7 @@ This document defines the authoritative configuration contract for OpenClaw. It ## 1. Configuration Principles -1. **Environment First**: Environment variables (`OPENCLAW_*`) always take precedence over file-based config or defaults. +1. **Environment First**: Environment variables (`OPENCLAW_*`) always take precedence over runtime/file-based config layers. 2. **Secure by Default**: Missing optional secrets result in disabled features (fail-closed), not insecure open access. 3. **No Plaintext Storage**: Secrets MUST NOT be stored in plaintext config files committed to version control. They should be injected via environment variables or a secure secrets manager. 4. **Legacy Compatibility**: `MOLTBOT_*` keys are supported for backward compatibility but are deprecated. `OPENCLAW_*` keys are preferred. @@ -146,15 +146,17 @@ To rotate a secret (e.g., `OPENCLAW_ADMIN_TOKEN` or `OPENCLAW_LLM_API_KEY`): ### 3.2 Key Precedence -If multiple keys are configured for the same purpose, the following order applies: +If multiple layers are configured for the same purpose, the following order applies: 1. `OPENCLAW_` (Highest priority) 2. `MOLTBOT_` (Legacy fallback) -3. File-based config / Defaults (Lowest priority) +3. Runtime override (process-local, non-persisted) +4. File-based config (`OPENCLAW_STATE_DIR/config.json`) +5. Defaults (Lowest priority) ### 3.3 Persistence -Non-secret configuration (such as enabled/disabled flags, feature toggles) may be persisted in the `OPENCLAW_STATE_DIR/config.json` via the Settings API. However, **environment variables always override persisted settings**. +Non-secret configuration (such as enabled/disabled flags, feature toggles) may be persisted in the `OPENCLAW_STATE_DIR/config.json` via the Settings API. Runtime overrides (if enabled by internal callers) are process-local and non-persisted. **Environment variables always override runtime and persisted settings**. Persistence guardrails: - Runtime-only guardrail fields (for example `runtime_guardrails` and legacy guardrail aliases) are stripped/ignored when loading persisted config and rejected on `/config` write requests. diff --git a/services/config_layers.py b/services/config_layers.py new file mode 100644 index 0000000..acd74d1 --- /dev/null +++ b/services/config_layers.py @@ -0,0 +1,157 @@ +""" +R139 layered configuration primitives. + +This module provides a small, dependency-light resolver used by runtime config +to unify precedence handling without forcing a big-bang migration. +""" + +from __future__ import annotations + +import os +from threading import Lock +from typing import Any, Callable, Dict, Iterable, Mapping, Optional, Tuple + +SOURCE_ENV = "env" +SOURCE_RUNTIME_OVERRIDE = "runtime_override" +SOURCE_PERSISTED = "persisted" +SOURCE_DEFAULT = "default" + +LLM_ENV_MAPPINGS: Dict[str, Tuple[str, str]] = { + "provider": ("OPENCLAW_LLM_PROVIDER", "MOLTBOT_LLM_PROVIDER"), + "model": ("OPENCLAW_LLM_MODEL", "MOLTBOT_LLM_MODEL"), + "base_url": ("OPENCLAW_LLM_BASE_URL", "MOLTBOT_LLM_BASE_URL"), + "timeout_sec": ("OPENCLAW_LLM_TIMEOUT", "MOLTBOT_LLM_TIMEOUT"), + "max_retries": ("OPENCLAW_LLM_MAX_RETRIES", "MOLTBOT_LLM_MAX_RETRIES"), + "fallback_models": ("OPENCLAW_FALLBACK_MODELS", "MOLTBOT_FALLBACK_MODELS"), + "fallback_providers": ( + "OPENCLAW_FALLBACK_PROVIDERS", + "MOLTBOT_FALLBACK_PROVIDERS", + ), + "max_failover_candidates": ( + "OPENCLAW_MAX_FAILOVER_CANDIDATES", + "MOLTBOT_MAX_FAILOVER_CANDIDATES", + ), +} + +GENERIC_LLM_API_KEY_ENV_KEYS = ( + "OPENCLAW_LLM_API_KEY", + "MOLTBOT_LLM_API_KEY", + "CLAWDBOT_LLM_API_KEY", +) +ADMIN_TOKEN_ENV_KEYS = ("OPENCLAW_ADMIN_TOKEN", "MOLTBOT_ADMIN_TOKEN") +OBS_TOKEN_ENV_KEYS = ("OPENCLAW_OBSERVABILITY_TOKEN", "MOLTBOT_OBSERVABILITY_TOKEN") + +_RUNTIME_OVERRIDE_LOCK = Lock() +_RUNTIME_OVERRIDES: Dict[str, Dict[str, Any]] = {} + + +def get_first_present_env( + keys: Iterable[str], *, env: Optional[Mapping[str, str]] = None +) -> Optional[str]: + """Return the first env value by presence (not truthiness).""" + env_map = env or os.environ + for key in keys: + if key in env_map: + return env_map.get(key) + return None + + +def get_preferred_env_value( + primary: str, legacy: str, *, env: Optional[Mapping[str, str]] = None +) -> Tuple[Optional[str], bool]: + """ + Return value by primary->legacy precedence and whether legacy path was used. + + Presence-based semantics are intentional so explicit empty-string values still + count as a deliberate override. + """ + env_map = env or os.environ + if primary in env_map: + return env_map.get(primary), False + if legacy and legacy in env_map: + return env_map.get(legacy), True + return None, False + + +def get_runtime_overrides(section: str) -> Dict[str, Any]: + """Get a shallow copy of runtime overrides for a section.""" + with _RUNTIME_OVERRIDE_LOCK: + return dict(_RUNTIME_OVERRIDES.get(section, {})) + + +def set_runtime_overrides(section: str, updates: Mapping[str, Any]) -> Dict[str, Any]: + """Merge runtime overrides for a section. `None` value removes the key.""" + with _RUNTIME_OVERRIDE_LOCK: + current = dict(_RUNTIME_OVERRIDES.get(section, {})) + for key, value in updates.items(): + if value is None: + current.pop(key, None) + else: + current[key] = value + if current: + _RUNTIME_OVERRIDES[section] = current + else: + _RUNTIME_OVERRIDES.pop(section, None) + return dict(current) + + +def clear_runtime_overrides(section: str, keys: Optional[Iterable[str]] = None) -> None: + """Clear runtime overrides for a section or specific keys in the section.""" + with _RUNTIME_OVERRIDE_LOCK: + if keys is None: + _RUNTIME_OVERRIDES.pop(section, None) + return + current = _RUNTIME_OVERRIDES.get(section) + if not current: + return + for key in keys: + current.pop(key, None) + if not current: + _RUNTIME_OVERRIDES.pop(section, None) + + +def resolve_layered_config( + *, + ordered_keys: Iterable[str], + defaults: Mapping[str, Any], + persisted: Optional[Mapping[str, Any]] = None, + runtime_overrides: Optional[Mapping[str, Any]] = None, + env_getter: Optional[Callable[[str], Optional[Any]]] = None, + normalize_value: Optional[Callable[[str, Any, str], Any]] = None, +) -> Tuple[Dict[str, Any], Dict[str, str]]: + """ + Resolve layered values with deterministic precedence. + + Precedence (highest to lowest): env > runtime_override > persisted > default. + """ + persisted_map = dict(persisted or {}) + runtime_map = dict(runtime_overrides or {}) + effective: Dict[str, Any] = {} + sources: Dict[str, str] = {} + + for key in ordered_keys: + value = defaults.get(key) + source = SOURCE_DEFAULT + + if key in persisted_map: + value = persisted_map.get(key) + source = SOURCE_PERSISTED + + if key in runtime_map: + value = runtime_map.get(key) + source = SOURCE_RUNTIME_OVERRIDE + + if env_getter is not None: + env_value = env_getter(key) + if env_value is not None: + value = env_value + source = SOURCE_ENV + + if normalize_value is not None: + value = normalize_value(key, value, source) + + effective[key] = value + sources[key] = source + + return effective, sources + diff --git a/services/connector_callback_contract.py b/services/connector_callback_contract.py index 994b423..bdf6bc3 100644 --- a/services/connector_callback_contract.py +++ b/services/connector_callback_contract.py @@ -132,7 +132,9 @@ class ConnectorCallbackContract: self._installation_registry = ( installation_registry or get_connector_installation_registry() ) - self._replay_guard = replay_guard or ReplayGuard(window_sec=300, max_entries=5000) + self._replay_guard = replay_guard or ReplayGuard( + window_sec=300, max_entries=5000 + ) self._callback_contract = callback_contract or CallbackContract() self._action_policy_map = dict(action_policy_map or {}) self._timestamp_drift_sec = max(1, int(timestamp_drift_sec)) @@ -256,7 +258,9 @@ class ConnectorCallbackContract: decision_code=CallbackDecisionCode.REJECT_TIMESTAMP.value, message="timestamp_out_of_window", ) - self._audit_decision(platform=platform, envelope=envelope, decision=decision) + self._audit_decision( + platform=platform, envelope=envelope, decision=decision + ) return decision expected_hash = self.compute_payload_hash(payload) @@ -266,7 +270,9 @@ class ConnectorCallbackContract: decision_code=CallbackDecisionCode.REJECT_PAYLOAD_HASH.value, message="payload_hash_mismatch", ) - self._audit_decision(platform=platform, envelope=envelope, decision=decision) + self._audit_decision( + platform=platform, envelope=envelope, decision=decision + ) return decision # CRITICAL: interactive callback signatures must remain canonical and constant-time. @@ -279,7 +285,9 @@ class ConnectorCallbackContract: decision_code=CallbackDecisionCode.REJECT_SIGNATURE.value, message="signature_mismatch", ) - self._audit_decision(platform=platform, envelope=envelope, decision=decision) + self._audit_decision( + platform=platform, envelope=envelope, decision=decision + ) return decision if self._replay_guard.is_duplicate(envelope.request_id): @@ -288,7 +296,9 @@ class ConnectorCallbackContract: decision_code=CallbackDecisionCode.REJECT_REPLAY.value, message="request_id_replay", ) - self._audit_decision(platform=platform, envelope=envelope, decision=decision) + self._audit_decision( + platform=platform, envelope=envelope, decision=decision + ) return decision resolution = self._installation_registry.resolve_installation( @@ -296,7 +306,9 @@ class ConnectorCallbackContract: ) if not resolution.ok or resolution.installation is None: decision = self._map_installation_reject(resolution) - self._audit_decision(platform=platform, envelope=envelope, decision=decision) + self._audit_decision( + platform=platform, envelope=envelope, decision=decision + ) return decision command_class = self._resolve_action_policy(envelope.action_type) @@ -307,7 +319,9 @@ class ConnectorCallbackContract: installation_id=resolution.installation.installation_id, message="unknown_action_type", ) - self._audit_decision(platform=platform, envelope=envelope, decision=decision) + self._audit_decision( + platform=platform, envelope=envelope, decision=decision + ) return decision record = self._callback_contract.create( diff --git a/services/connector_installation_registry.py b/services/connector_installation_registry.py index ce893d3..983b477 100644 --- a/services/connector_installation_registry.py +++ b/services/connector_installation_registry.py @@ -19,9 +19,7 @@ except ImportError: from services.secret_store import SecretStore, get_secret_store # type: ignore from services.state_dir import get_state_dir # type: ignore -logger = logging.getLogger( - "ComfyUI-OpenClaw.services.connector_installation_registry" -) +logger = logging.getLogger("ComfyUI-OpenClaw.services.connector_installation_registry") INSTALLATION_STORE_FILE = "connector_installations.json" MAX_INSTALLATION_AUDIT = 500 @@ -249,7 +247,9 @@ class ConnectorInstallationRegistry: now = time.time() refs = dict(token_refs or {}) if token_values: - refs.update(self._store_token_refs(normalized_installation, token_values)) + refs.update( + self._store_token_refs(normalized_installation, token_values) + ) existing = self._installations.get(normalized_installation) if existing is None: created_at = now @@ -288,12 +288,22 @@ class ConnectorInstallationRegistry: with self._lock: items = list(self._installations.values()) if platform: - items = [i for i in items if i.platform == self._normalize_platform(platform)] + items = [ + i for i in items if i.platform == self._normalize_platform(platform) + ] if workspace_id: - items = [i for i in items if i.workspace_id == str(workspace_id).strip()] + items = [ + i for i in items if i.workspace_id == str(workspace_id).strip() + ] if status: items = [i for i in items if i.status == str(status).strip()] - items.sort(key=lambda inst: (inst.platform, inst.workspace_id, inst.installation_id)) + items.sort( + key=lambda inst: ( + inst.platform, + inst.workspace_id, + inst.installation_id, + ) + ) return [ConnectorInstallation(**asdict(inst)) for inst in items] def _transition( @@ -317,7 +327,9 @@ class ConnectorInstallationRegistry: self._save() return ConnectorInstallation(**asdict(inst)) - def activate_installation(self, installation_id: str, reason: str = "") -> ConnectorInstallation: + def activate_installation( + self, installation_id: str, reason: str = "" + ) -> ConnectorInstallation: return self._transition( installation_id, InstallationStatus.ACTIVE.value, @@ -352,7 +364,9 @@ class ConnectorInstallationRegistry: self._save() return ConnectorInstallation(**asdict(inst)) - def revoke_installation(self, installation_id: str, reason: str = "") -> ConnectorInstallation: + def revoke_installation( + self, installation_id: str, reason: str = "" + ) -> ConnectorInstallation: return self._transition( installation_id, InstallationStatus.REVOKED.value, @@ -360,7 +374,9 @@ class ConnectorInstallationRegistry: action="revoke", ) - def deactivate_installation(self, installation_id: str, reason: str = "") -> ConnectorInstallation: + def deactivate_installation( + self, installation_id: str, reason: str = "" + ) -> ConnectorInstallation: return self._transition( installation_id, InstallationStatus.DEACTIVATED.value, @@ -368,7 +384,9 @@ class ConnectorInstallationRegistry: action="deactivate", ) - def uninstall_installation(self, installation_id: str, reason: str = "") -> ConnectorInstallation: + def uninstall_installation( + self, installation_id: str, reason: str = "" + ) -> ConnectorInstallation: with self._lock: inst = self._installations.get(str(installation_id).strip()) if inst is None: @@ -383,7 +401,9 @@ class ConnectorInstallationRegistry: self._save() return ConnectorInstallation(**asdict(inst)) - def resolve_installation(self, platform: str, workspace_id: str) -> InstallationResolution: + def resolve_installation( + self, platform: str, workspace_id: str + ) -> InstallationResolution: with self._lock: normalized_platform = self._normalize_platform(platform) normalized_workspace = self._normalize_identifier( @@ -395,9 +415,7 @@ class ConnectorInstallationRegistry: if inst.platform == normalized_platform and inst.workspace_id == normalized_workspace ] - eligible = [ - inst for inst in matches if inst.status in _RESOLVABLE_STATUSES - ] + eligible = [inst for inst in matches if inst.status in _RESOLVABLE_STATUSES] if len(eligible) > 1: return InstallationResolution( ok=False, diff --git a/services/llm_client.py b/services/llm_client.py index a4e350e..18a5ceb 100644 --- a/services/llm_client.py +++ b/services/llm_client.py @@ -45,31 +45,43 @@ except ImportError: def get_configured_provider() -> str: - """Get the configured provider from environment or default.""" - return ( - os.environ.get("OPENCLAW_LLM_PROVIDER") - or os.environ.get("MOLTBOT_LLM_PROVIDER") - or DEFAULT_PROVIDER - ).lower() + """Get configured provider from the unified runtime-config surface.""" + try: + from ..services.runtime_config import get_effective_config + except ImportError: + from services.runtime_config import get_effective_config + + effective, _sources = get_effective_config() + return str(effective.get("provider") or DEFAULT_PROVIDER).lower() def get_configured_model(provider: str) -> str: - """Get the configured model for a provider.""" - env_model = os.environ.get("OPENCLAW_LLM_MODEL") or os.environ.get( - "MOLTBOT_LLM_MODEL" - ) - if env_model: - return env_model + """Get configured model for a provider via unified runtime-config snapshot.""" + try: + from ..services.runtime_config import get_effective_config + except ImportError: + from services.runtime_config import get_effective_config + + effective, _sources = get_effective_config() + current_provider = str(effective.get("provider") or "").lower() + configured_model = effective.get("model") + if configured_model and str(provider).lower() == current_provider: + return str(configured_model) return DEFAULT_MODEL_BY_PROVIDER.get(provider, "default") def get_configured_base_url(provider: str) -> str: - """Get the configured base URL for a provider.""" - env_url = os.environ.get("OPENCLAW_LLM_BASE_URL") or os.environ.get( - "MOLTBOT_LLM_BASE_URL" - ) - if env_url: - return env_url + """Get configured base URL via unified runtime-config snapshot.""" + try: + from ..services.runtime_config import get_effective_config + except ImportError: + from services.runtime_config import get_effective_config + + effective, _sources = get_effective_config() + current_provider = str(effective.get("provider") or "").lower() + configured_base_url = str(effective.get("base_url") or "").strip() + if configured_base_url and str(provider).lower() == current_provider: + return configured_base_url info = get_provider_info(provider) if info: diff --git a/services/runtime_config.py b/services/runtime_config.py index 37b31a7..1c9a88f 100644 --- a/services/runtime_config.py +++ b/services/runtime_config.py @@ -87,6 +87,148 @@ except ImportError: return config_blob, [] +# R139: Layered config resolver + compatibility env alias helpers. +try: + from .config_layers import ( + ADMIN_TOKEN_ENV_KEYS, + LLM_ENV_MAPPINGS, + SOURCE_ENV, + SOURCE_PERSISTED, + SOURCE_RUNTIME_OVERRIDE, + ) + from .config_layers import clear_runtime_overrides as _clear_runtime_overrides + from .config_layers import get_first_present_env, get_preferred_env_value + from .config_layers import get_runtime_overrides as _get_runtime_overrides + from .config_layers import resolve_layered_config + from .config_layers import set_runtime_overrides as _set_runtime_overrides +except ImportError: + try: + from services.config_layers import ( + ADMIN_TOKEN_ENV_KEYS, + LLM_ENV_MAPPINGS, + SOURCE_ENV, + SOURCE_PERSISTED, + SOURCE_RUNTIME_OVERRIDE, + ) + from services.config_layers import ( + clear_runtime_overrides as _clear_runtime_overrides, # type: ignore + ) + from services.config_layers import ( + get_first_present_env, + get_preferred_env_value, + ) + from services.config_layers import ( + get_runtime_overrides as _get_runtime_overrides, + ) + from services.config_layers import resolve_layered_config + from services.config_layers import ( + set_runtime_overrides as _set_runtime_overrides, + ) + except ImportError: + # Compatibility fallback for constrained test environments. + ADMIN_TOKEN_ENV_KEYS = ("OPENCLAW_ADMIN_TOKEN", "MOLTBOT_ADMIN_TOKEN") + SOURCE_ENV = "env" + SOURCE_PERSISTED = "persisted" + SOURCE_RUNTIME_OVERRIDE = "runtime_override" + LLM_ENV_MAPPINGS = { + "provider": ("OPENCLAW_LLM_PROVIDER", "MOLTBOT_LLM_PROVIDER"), + "model": ("OPENCLAW_LLM_MODEL", "MOLTBOT_LLM_MODEL"), + "base_url": ("OPENCLAW_LLM_BASE_URL", "MOLTBOT_LLM_BASE_URL"), + "timeout_sec": ("OPENCLAW_LLM_TIMEOUT", "MOLTBOT_LLM_TIMEOUT"), + "max_retries": ("OPENCLAW_LLM_MAX_RETRIES", "MOLTBOT_LLM_MAX_RETRIES"), + "fallback_models": ( + "OPENCLAW_FALLBACK_MODELS", + "MOLTBOT_FALLBACK_MODELS", + ), + "fallback_providers": ( + "OPENCLAW_FALLBACK_PROVIDERS", + "MOLTBOT_FALLBACK_PROVIDERS", + ), + "max_failover_candidates": ( + "OPENCLAW_MAX_FAILOVER_CANDIDATES", + "MOLTBOT_MAX_FAILOVER_CANDIDATES", + ), + } + + def get_first_present_env(keys, *, env=None): # type: ignore + env_map = env or os.environ + for key in keys: + if key in env_map: + return env_map.get(key) + return None + + def get_preferred_env_value(primary, legacy, *, env=None): # type: ignore + env_map = env or os.environ + if primary in env_map: + return env_map.get(primary), False + if legacy and legacy in env_map: + return env_map.get(legacy), True + return None, False + + _RUNTIME_OVERRIDES: Dict[str, Dict[str, Any]] = {} + + def _get_runtime_overrides(section): # type: ignore + return dict(_RUNTIME_OVERRIDES.get(section, {})) + + def _set_runtime_overrides(section, updates): # type: ignore + current = dict(_RUNTIME_OVERRIDES.get(section, {})) + for key, value in updates.items(): + if value is None: + current.pop(key, None) + else: + current[key] = value + if current: + _RUNTIME_OVERRIDES[section] = current + else: + _RUNTIME_OVERRIDES.pop(section, None) + return dict(current) + + def _clear_runtime_overrides(section, keys=None): # type: ignore + if keys is None: + _RUNTIME_OVERRIDES.pop(section, None) + return + current = _RUNTIME_OVERRIDES.get(section) + if not current: + return + for key in keys: + current.pop(key, None) + if not current: + _RUNTIME_OVERRIDES.pop(section, None) + + def resolve_layered_config( # type: ignore + *, + ordered_keys, + defaults, + persisted=None, + runtime_overrides=None, + env_getter=None, + normalize_value=None, + ): + persisted = dict(persisted or {}) + runtime_overrides = dict(runtime_overrides or {}) + effective = {} + sources = {} + for key in ordered_keys: + value = defaults.get(key) + source = "default" + if key in persisted: + value = persisted.get(key) + source = SOURCE_PERSISTED + if key in runtime_overrides: + value = runtime_overrides.get(key) + source = SOURCE_RUNTIME_OVERRIDE + if env_getter is not None: + env_value = env_getter(key) + if env_value is not None: + value = env_value + source = SOURCE_ENV + if normalize_value is not None: + value = normalize_value(key, value, source) + effective[key] = value + sources[key] = source + return effective, sources + + # Config file location (under state dir) try: # Prefer package-relative imports when running as a ComfyUI custom node pack. @@ -194,20 +336,8 @@ SCHEDULER_CONSTRAINTS = { } # Environment variable mappings (new, legacy) -ENV_MAPPINGS = { - "provider": ("OPENCLAW_LLM_PROVIDER", "MOLTBOT_LLM_PROVIDER"), - "model": ("OPENCLAW_LLM_MODEL", "MOLTBOT_LLM_MODEL"), - "base_url": ("OPENCLAW_LLM_BASE_URL", "MOLTBOT_LLM_BASE_URL"), - "timeout_sec": ("OPENCLAW_LLM_TIMEOUT", "MOLTBOT_LLM_TIMEOUT"), - "max_retries": ("OPENCLAW_LLM_MAX_RETRIES", "MOLTBOT_LLM_MAX_RETRIES"), - # R14: Failover env vars - "fallback_models": ("OPENCLAW_FALLBACK_MODELS", "MOLTBOT_FALLBACK_MODELS"), - "fallback_providers": ("OPENCLAW_FALLBACK_PROVIDERS", "MOLTBOT_FALLBACK_PROVIDERS"), - "max_failover_candidates": ( - "OPENCLAW_MAX_FAILOVER_CANDIDATES", - "MOLTBOT_MAX_FAILOVER_CANDIDATES", - ), -} +# R139: defined in services.config_layers as the single source-of-truth. +ENV_MAPPINGS = dict(LLM_ENV_MAPPINGS) SCHEDULER_ENV_MAPPINGS = { "startup_jitter_sec": ("OPENCLAW_SCHEDULER_STARTUP_JITTER_SEC", ""), @@ -307,25 +437,21 @@ def _get_env_value(key: str) -> Optional[str]: if not env_vars: return None primary, legacy = env_vars + value, used_legacy = get_preferred_env_value(primary, legacy) + if not used_legacy: + return value - # Respect explicit empty-string overrides: treat "present in env" as an override. - if primary in os.environ: - return os.environ.get(primary) + # Check if we've already warned for this key to avoid spam. + if not getattr(_get_env_value, "_warned_legacy", None): + _get_env_value._warned_legacy = set() - if legacy in os.environ: - # Check if we've already warned for this key to avoid spam - if not getattr(_get_env_value, "_warned_legacy", None): - _get_env_value._warned_legacy = set() - - if legacy not in _get_env_value._warned_legacy: - logger.warning( - f"Config: Using legacy environment variable {legacy}. " - f"Please update to {primary} in future versions." - ) - _get_env_value._warned_legacy.add(legacy) - - return os.environ.get(legacy) - return None + if legacy not in _get_env_value._warned_legacy: + logger.warning( + f"Config: Using legacy environment variable {legacy}. " + f"Please update to {primary} in future versions." + ) + _get_env_value._warned_legacy.add(legacy) + return value def _env_flag(primary: str, legacy: str, default: bool = False) -> bool: @@ -430,66 +556,79 @@ def get_scheduler_config() -> Dict[str, Any]: return effective +def _normalize_llm_layer_value(key: str, value: Any, source: str) -> Any: + """Normalize/clamp per-key values while preserving compatibility semantics.""" + if source == SOURCE_ENV: + if key in ("fallback_models", "fallback_providers"): + if isinstance(value, str): + return [item.strip() for item in value.split(",") if item.strip()] + if isinstance(value, list): + return [str(item).strip() for item in value if str(item).strip()] + return [] + + if key in CONSTRAINTS: + try: + value_int = int(value) + except (TypeError, ValueError): + return DEFAULTS["llm"].get(key) + min_val, max_val = _get_constraint_range(key) + return _clamp(value_int, min_val, max_val) + + return value + + # Persisted/runtime/default values keep historical compatibility: + # clamp numeric constraint keys only when the value is already numeric. + if key in CONSTRAINTS and isinstance(value, (int, float)): + min_val, max_val = _get_constraint_range(key) + return _clamp(int(value), min_val, max_val) + return value + + +def get_runtime_overrides() -> Dict[str, Any]: + """Get current in-memory runtime overrides for the LLM section.""" + return _get_runtime_overrides("llm") + + +def set_runtime_overrides(updates: Dict[str, Any]) -> Tuple[bool, list]: + """ + Set in-memory runtime overrides for LLM config (non-persisted). + + Uses the same schema validation path as persisted config updates. + """ + sanitized, errors = validate_config_update(updates) + if errors: + return False, errors + _set_runtime_overrides("llm", sanitized) + return True, [] + + +def clear_runtime_overrides(keys: Optional[List[str]] = None) -> None: + """Clear all runtime overrides (or only selected keys) for LLM config.""" + _clear_runtime_overrides("llm", keys=keys) + + def get_effective_config() -> Tuple[Dict[str, Any], Dict[str, str]]: """ - Get effective LLM config with precedence: ENV > file > defaults. + Get effective LLM config with precedence: + ENV > runtime_override > persisted file > defaults. Returns: Tuple of (effective_config, sources) where sources maps each key to its origin. """ file_config = _load_file_config().get("llm", {}) - - effective = {} - sources = {} + runtime_overrides = get_runtime_overrides() ordered_keys = list(LLM_KEY_ORDER) + [ k for k in sorted(ALLOWED_LLM_KEYS) if k not in ENV_MAPPINGS ] - - timeout_cap, retry_cap = _s66_timeout_retry_caps() - - for key in ordered_keys: - # 1. Check ENV override - env_val = _get_env_value(key) - if env_val is not None: - # R14: Parse list env vars (comma-separated) - if key in ("fallback_models", "fallback_providers"): - env_val = [item.strip() for item in env_val.split(",") if item.strip()] - # Parse numeric env vars - if key in CONSTRAINTS: - try: - env_val = int(env_val) - if key == "timeout_sec": - env_val = _clamp(env_val, CONSTRAINTS[key][0], timeout_cap) - elif key == "max_retries": - env_val = _clamp(env_val, CONSTRAINTS[key][0], retry_cap) - else: - env_val = _clamp(env_val, *CONSTRAINTS[key]) - except ValueError: - env_val = DEFAULTS["llm"].get(key) - effective[key] = env_val - sources[key] = "env" - continue - - # 2. Check file config - if key in file_config: - val = file_config[key] - if key in CONSTRAINTS and isinstance(val, (int, float)): - if key == "timeout_sec": - val = _clamp(int(val), CONSTRAINTS[key][0], timeout_cap) - elif key == "max_retries": - val = _clamp(int(val), CONSTRAINTS[key][0], retry_cap) - else: - val = _clamp(int(val), *CONSTRAINTS[key]) - effective[key] = val - sources[key] = "file" - continue - - # 3. Use default - effective[key] = DEFAULTS["llm"].get(key, "") - sources[key] = "default" - - return effective, sources + return resolve_layered_config( + ordered_keys=ordered_keys, + defaults=DEFAULTS["llm"], + persisted=file_config, + runtime_overrides=runtime_overrides, + env_getter=_get_env_value, + normalize_value=_normalize_llm_layer_value, + ) def get_settings_schema() -> dict: @@ -784,9 +923,7 @@ def is_config_write_enabled() -> bool: def validate_admin_token(token: str) -> bool: """Validate admin token for config writes (S13).""" - expected = os.environ.get("OPENCLAW_ADMIN_TOKEN") or os.environ.get( - "MOLTBOT_ADMIN_TOKEN", "" - ) + expected = get_first_present_env(ADMIN_TOKEN_ENV_KEYS) or "" if not expected: return True # No token configured = convenience mode; caller must still enforce loopback-only. return token == expected @@ -799,11 +936,7 @@ def get_admin_token() -> str: This is for internal policy decisions only (e.g., "is a token configured?"). Never return this value to UI callers and never log it. """ - return ( - os.environ.get("OPENCLAW_ADMIN_TOKEN") - or os.environ.get("MOLTBOT_ADMIN_TOKEN") - or "" - ) + return get_first_present_env(ADMIN_TOKEN_ENV_KEYS) or "" def is_loopback_client(remote_addr: str) -> bool: @@ -814,7 +947,7 @@ def is_loopback_client(remote_addr: str) -> bool: class RuntimeConfig: """ Typed configuration snapshot. - Aggregates effective settings from Env and File. + Aggregates effective settings from layered config sources. """ def __init__(self): diff --git a/tests/contract/test_config_contract.py b/tests/contract/test_config_contract.py index b166f72..69d470d 100644 --- a/tests/contract/test_config_contract.py +++ b/tests/contract/test_config_contract.py @@ -8,7 +8,7 @@ import pytest # Contract: Config Precedence def test_config_precedence(): """ - Contract: OPENCLAW_* env vars > MOLTBOT_* env vars > file config > defaults. + Contract: OPENCLAW_* env vars > MOLTBOT_* env vars > runtime override > file config > defaults. """ # Mocking json load to avoid file I/O dependencies mock_json_load = MagicMock(return_value={}) @@ -33,6 +33,28 @@ def test_config_precedence(): assert sources["provider"] == "env" +def test_runtime_override_precedence_without_env(): + """ + Contract: runtime override wins over persisted/default when env overrides are absent. + """ + from services.runtime_config import ( + clear_runtime_overrides, + get_effective_config, + set_runtime_overrides, + ) + + clear_runtime_overrides() + ok, errors = set_runtime_overrides({"provider": "openrouter"}) + assert ok is True + assert errors == [] + try: + config, sources = get_effective_config() + assert config["provider"] == "openrouter" + assert sources["provider"] == "runtime_override" + finally: + clear_runtime_overrides() + + # Contract: Secret Safety def test_secrets_never_exposed(): """ diff --git a/tests/test_api_connector_contracts.py b/tests/test_api_connector_contracts.py index 7347e50..218990c 100644 --- a/tests/test_api_connector_contracts.py +++ b/tests/test_api_connector_contracts.py @@ -29,8 +29,13 @@ class TestAPIConnectorContracts(unittest.IsolatedAsyncioTestCase): with ( patch("api.connector_contracts.check_rate_limit", return_value=True), - patch("api.connector_contracts.require_admin_token", return_value=(True, None)), - patch("api.connector_contracts.get_connector_installation_registry", return_value=registry), + patch( + "api.connector_contracts.require_admin_token", return_value=(True, None) + ), + patch( + "api.connector_contracts.get_connector_installation_registry", + return_value=registry, + ), ): resp = await mod.connector_installations_list_handler(request) @@ -50,8 +55,13 @@ class TestAPIConnectorContracts(unittest.IsolatedAsyncioTestCase): with ( patch("api.connector_contracts.check_rate_limit", return_value=True), - patch("api.connector_contracts.require_admin_token", return_value=(True, None)), - patch("api.connector_contracts.get_connector_installation_registry", return_value=registry), + patch( + "api.connector_contracts.require_admin_token", return_value=(True, None) + ), + patch( + "api.connector_contracts.get_connector_installation_registry", + return_value=registry, + ), ): resp = await mod.connector_installation_get_handler(request) @@ -74,8 +84,13 @@ class TestAPIConnectorContracts(unittest.IsolatedAsyncioTestCase): with ( patch("api.connector_contracts.check_rate_limit", return_value=True), - patch("api.connector_contracts.require_admin_token", return_value=(True, None)), - patch("api.connector_contracts.get_connector_installation_registry", return_value=registry), + patch( + "api.connector_contracts.require_admin_token", return_value=(True, None) + ), + patch( + "api.connector_contracts.get_connector_installation_registry", + return_value=registry, + ), ): resp = await mod.connector_installation_resolve_handler(request) @@ -89,7 +104,10 @@ class TestAPIConnectorContracts(unittest.IsolatedAsyncioTestCase): with ( patch("api.connector_contracts.check_rate_limit", return_value=True), - patch("api.connector_contracts.require_admin_token", return_value=(False, "Unauthorized")), + patch( + "api.connector_contracts.require_admin_token", + return_value=(False, "Unauthorized"), + ), ): resp = await mod.connector_installation_audit_handler(request) diff --git a/tests/test_connector_callback_contract.py b/tests/test_connector_callback_contract.py index b065b44..f7c1ddc 100644 --- a/tests/test_connector_callback_contract.py +++ b/tests/test_connector_callback_contract.py @@ -3,7 +3,6 @@ import time import unittest from connector.config import CommandClass - from services.connector_callback_contract import ( CallbackActorContext, CallbackDecisionCode, @@ -63,7 +62,9 @@ class TestConnectorCallbackContract(unittest.TestCase): actor=CallbackActorContext(is_admin=False, is_trusted=False), ) self.assertTrue(decision.ok) - self.assertEqual(decision.decision_code, CallbackDecisionCode.ACCEPT_PUBLIC.value) + self.assertEqual( + decision.decision_code, CallbackDecisionCode.ACCEPT_PUBLIC.value + ) self.assertTrue(decision.callback_id) def test_tampered_signature_rejected(self): @@ -82,7 +83,9 @@ class TestConnectorCallbackContract(unittest.TestCase): actor=CallbackActorContext(), ) self.assertFalse(decision.ok) - self.assertEqual(decision.decision_code, CallbackDecisionCode.REJECT_SIGNATURE.value) + self.assertEqual( + decision.decision_code, CallbackDecisionCode.REJECT_SIGNATURE.value + ) def test_stale_timestamp_rejected(self): payload = self._payload() @@ -100,7 +103,9 @@ class TestConnectorCallbackContract(unittest.TestCase): actor=CallbackActorContext(), ) self.assertFalse(decision.ok) - self.assertEqual(decision.decision_code, CallbackDecisionCode.REJECT_TIMESTAMP.value) + self.assertEqual( + decision.decision_code, CallbackDecisionCode.REJECT_TIMESTAMP.value + ) def test_replay_request_id_rejected(self): payload = self._payload() @@ -236,7 +241,9 @@ class TestConnectorCallbackContract(unittest.TestCase): actor=CallbackActorContext(is_admin=True), ) self.assertTrue(decision.ok) - self.assertEqual(decision.decision_code, CallbackDecisionCode.ACCEPT_ADMIN.value) + self.assertEqual( + decision.decision_code, CallbackDecisionCode.ACCEPT_ADMIN.value + ) def test_missing_installation_rejected(self): payload = self._payload() diff --git a/tests/test_r139_config_layers.py b/tests/test_r139_config_layers.py new file mode 100644 index 0000000..fda17fa --- /dev/null +++ b/tests/test_r139_config_layers.py @@ -0,0 +1,109 @@ +import unittest + +from services.config_layers import ( + SOURCE_DEFAULT, + SOURCE_ENV, + SOURCE_PERSISTED, + SOURCE_RUNTIME_OVERRIDE, + clear_runtime_overrides, + get_preferred_env_value, + get_runtime_overrides, + resolve_layered_config, + set_runtime_overrides, +) + + +class TestR139ConfigLayers(unittest.TestCase): + def setUp(self): + clear_runtime_overrides("llm") + + def tearDown(self): + clear_runtime_overrides("llm") + + def test_get_preferred_env_value_prefers_primary(self): + value, used_legacy = get_preferred_env_value( + "OPENCLAW_LLM_PROVIDER", + "MOLTBOT_LLM_PROVIDER", + env={"OPENCLAW_LLM_PROVIDER": "openai", "MOLTBOT_LLM_PROVIDER": "anthropic"}, + ) + self.assertEqual(value, "openai") + self.assertFalse(used_legacy) + + def test_get_preferred_env_value_uses_legacy_when_primary_missing(self): + value, used_legacy = get_preferred_env_value( + "OPENCLAW_LLM_PROVIDER", + "MOLTBOT_LLM_PROVIDER", + env={"MOLTBOT_LLM_PROVIDER": "anthropic"}, + ) + self.assertEqual(value, "anthropic") + self.assertTrue(used_legacy) + + def test_resolve_layered_config_precedence(self): + # defaults < persisted < runtime_override < env + effective, sources = resolve_layered_config( + ordered_keys=["provider"], + defaults={"provider": "openai"}, + persisted={"provider": "gemini"}, + runtime_overrides={"provider": "openrouter"}, + env_getter=lambda _k: "anthropic", + ) + self.assertEqual(effective["provider"], "anthropic") + self.assertEqual(sources["provider"], SOURCE_ENV) + + def test_resolve_layered_config_without_env(self): + effective, sources = resolve_layered_config( + ordered_keys=["provider"], + defaults={"provider": "openai"}, + persisted={"provider": "gemini"}, + runtime_overrides={"provider": "openrouter"}, + env_getter=lambda _k: None, + ) + self.assertEqual(effective["provider"], "openrouter") + self.assertEqual(sources["provider"], SOURCE_RUNTIME_OVERRIDE) + + effective, sources = resolve_layered_config( + ordered_keys=["provider"], + defaults={"provider": "openai"}, + persisted={"provider": "gemini"}, + runtime_overrides={}, + env_getter=lambda _k: None, + ) + self.assertEqual(effective["provider"], "gemini") + self.assertEqual(sources["provider"], SOURCE_PERSISTED) + + effective, sources = resolve_layered_config( + ordered_keys=["provider"], + defaults={"provider": "openai"}, + persisted={}, + runtime_overrides={}, + env_getter=lambda _k: None, + ) + self.assertEqual(effective["provider"], "openai") + self.assertEqual(sources["provider"], SOURCE_DEFAULT) + + def test_runtime_override_registry_merge_and_clear(self): + current = set_runtime_overrides("llm", {"provider": "openai", "model": "x"}) + self.assertEqual(current["provider"], "openai") + self.assertEqual(current["model"], "x") + + current = set_runtime_overrides("llm", {"model": None, "timeout_sec": 120}) + self.assertEqual(current["provider"], "openai") + self.assertNotIn("model", current) + self.assertEqual(current["timeout_sec"], 120) + + snapshot = get_runtime_overrides("llm") + self.assertEqual(snapshot["provider"], "openai") + self.assertEqual(snapshot["timeout_sec"], 120) + + clear_runtime_overrides("llm", keys=["provider"]) + snapshot = get_runtime_overrides("llm") + self.assertNotIn("provider", snapshot) + self.assertIn("timeout_sec", snapshot) + + clear_runtime_overrides("llm") + self.assertEqual(get_runtime_overrides("llm"), {}) + + +if __name__ == "__main__": + unittest.main() + diff --git a/tests/test_runtime_config.py b/tests/test_runtime_config.py index 0c4f31f..04e7a9b 100644 --- a/tests/test_runtime_config.py +++ b/tests/test_runtime_config.py @@ -74,6 +74,14 @@ class TestRuntimeConfig(unittest.TestCase): ]: os.environ.pop(key, None) + # R139: runtime overrides are process-local; clear before each case. + try: + from services.runtime_config import clear_runtime_overrides + + clear_runtime_overrides() + except Exception: + pass + def test_defaults(self): """Should use defaults when no env or file config.""" from services.runtime_config import DEFAULTS, get_effective_config @@ -135,6 +143,32 @@ class TestRuntimeConfig(unittest.TestCase): self.assertEqual(effective["provider"], "openclaw-provider") # Should NOT log warning if primary is found (legacy is ignored) + def test_runtime_override_applies_without_env(self): + """R139: runtime override should win over persisted/default when env is absent.""" + from services.runtime_config import get_effective_config, set_runtime_overrides + + ok, errors = set_runtime_overrides({"provider": "openrouter"}) + self.assertTrue(ok) + self.assertEqual(errors, []) + + effective, sources = get_effective_config() + self.assertEqual(effective["provider"], "openrouter") + self.assertEqual(sources["provider"], "runtime_override") + + def test_env_beats_runtime_override(self): + """R139: env remains highest precedence over runtime override.""" + from services.runtime_config import get_effective_config, set_runtime_overrides + + ok, errors = set_runtime_overrides({"provider": "openrouter"}) + self.assertTrue(ok) + self.assertEqual(errors, []) + + with patch.dict(os.environ, {"OPENCLAW_LLM_PROVIDER": "anthropic"}): + effective, sources = get_effective_config() + + self.assertEqual(effective["provider"], "anthropic") + self.assertEqual(sources["provider"], "env") + def test_validate_provider(self): """Should reject unknown providers.""" from services.runtime_config import validate_config_update