From 618e3c3fca1aea480942fe1fb1a312a0cd83953a Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Mon, 16 Mar 2026 23:06:51 +0100 Subject: [PATCH 1/8] feat: migrate config consumers to read through SettingsService MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrate API controller config reads from direct RootConfig attribute access to SettingsService resolution (DB > env > YAML > code defaults), enabling runtime config overrides without YAML edits or restarts. Changes: - Create ConfigResolver bridge (settings/resolver.py) with typed scalar accessors and composed-read methods for BudgetConfig and CoordinationConfig - Migrate controllers: autonomy, company (company_name), budget, coordination to use ConfigResolver - Add 7 new settings definitions (3 coordination, 4 budget) - Fix autonomy_level definition bugs: wrong enum_values and yaml_path - Fix coordination yaml_paths: default_topology → topology, max_wave_size → max_concurrency_per_wave - Wire ConfigResolver into AppState via property accessor - 47 new unit tests for ConfigResolver + property-based roundtrips Closes #497 --- src/synthorg/api/controllers/autonomy.py | 6 +- src/synthorg/api/controllers/budget.py | 3 +- src/synthorg/api/controllers/company.py | 5 +- src/synthorg/api/controllers/coordination.py | 6 +- src/synthorg/api/state.py | 14 + src/synthorg/settings/__init__.py | 2 + src/synthorg/settings/definitions/budget.py | 60 ++ src/synthorg/settings/definitions/company.py | 8 +- .../settings/definitions/coordination.py | 40 +- src/synthorg/settings/resolver.py | 282 +++++++++ .../engine/test_coordination_wiring.py | 11 + .../unit/api/controllers/test_coordination.py | 14 +- tests/unit/settings/test_resolver.py | 534 ++++++++++++++++++ 13 files changed, 969 insertions(+), 16 deletions(-) create mode 100644 src/synthorg/settings/resolver.py create mode 100644 tests/unit/settings/test_resolver.py diff --git a/src/synthorg/api/controllers/autonomy.py b/src/synthorg/api/controllers/autonomy.py index 181cba7f8e..48b342fd04 100644 --- a/src/synthorg/api/controllers/autonomy.py +++ b/src/synthorg/api/controllers/autonomy.py @@ -71,8 +71,7 @@ async def get_autonomy( Current autonomy level info. """ app_state: AppState = state.app_state - config = app_state.config.config - level = config.autonomy.level + level = await app_state.config_resolver.get_autonomy_level() return ApiResponse( data=AutonomyLevelResponse( agent_id=agent_id, @@ -103,8 +102,7 @@ async def update_autonomy( Updated autonomy level info. """ app_state: AppState = state.app_state - config = app_state.config.config - current_level = config.autonomy.level + current_level = await app_state.config_resolver.get_autonomy_level() requested_level = data.level logger.info( diff --git a/src/synthorg/api/controllers/budget.py b/src/synthorg/api/controllers/budget.py index 96e78b3a8d..063e2aa5fe 100644 --- a/src/synthorg/api/controllers/budget.py +++ b/src/synthorg/api/controllers/budget.py @@ -51,7 +51,8 @@ async def get_budget_config( Budget config envelope. """ app_state: AppState = state.app_state - return ApiResponse(data=app_state.config.budget) + budget = await app_state.config_resolver.get_budget_config() + return ApiResponse(data=budget) @get("/records") async def list_cost_records( diff --git a/src/synthorg/api/controllers/company.py b/src/synthorg/api/controllers/company.py index 85ec37c87f..f44add92f3 100644 --- a/src/synthorg/api/controllers/company.py +++ b/src/synthorg/api/controllers/company.py @@ -38,9 +38,12 @@ async def get_company( Company configuration envelope. """ app_state: AppState = state.app_state + company_name = await app_state.config_resolver.get_str( + "company", "company_name" + ) config = app_state.config data: dict[str, Any] = { - "company_name": config.company_name, + "company_name": company_name, "agents": [a.model_dump(mode="json") for a in config.agents], "departments": [d.model_dump(mode="json") for d in config.departments], } diff --git a/src/synthorg/api/controllers/coordination.py b/src/synthorg/api/controllers/coordination.py index 0921e72e61..f55794fd14 100644 --- a/src/synthorg/api/controllers/coordination.py +++ b/src/synthorg/api/controllers/coordination.py @@ -158,7 +158,7 @@ async def coordinate_task( _validate_task_id(task_id) task = await self._get_task(app_state, task_id) agents = await self._resolve_agents(app_state, data, task_id) - context = self._build_context(app_state, task, agents, data) + context = await self._build_context(app_state, task, agents, data) _publish_ws_event( request, @@ -196,7 +196,7 @@ async def _get_task( raise NotFoundError(msg) return task - def _build_context( + async def _build_context( self, app_state: AppState, task: Task, @@ -208,7 +208,7 @@ def _build_context( DecompositionContext, ) - coord_config = app_state.config.coordination.to_coordination_config( + coord_config = await app_state.config_resolver.get_coordination_config( max_concurrency_per_wave=data.max_concurrency_per_wave, fail_fast=data.fail_fast, ) diff --git a/src/synthorg/api/state.py b/src/synthorg/api/state.py index 51b6587308..ccab206c5d 100644 --- a/src/synthorg/api/state.py +++ b/src/synthorg/api/state.py @@ -24,6 +24,7 @@ from synthorg.observability import get_logger from synthorg.observability.events.api import API_APP_STARTUP, API_SERVICE_UNAVAILABLE from synthorg.persistence.protocol import PersistenceBackend # noqa: TC001 +from synthorg.settings.resolver import ConfigResolver from synthorg.settings.service import SettingsService # noqa: TC001 logger = get_logger(__name__) @@ -236,6 +237,19 @@ def has_settings_service(self) -> bool: """Check whether the settings service is configured.""" return self._settings_service is not None + @property + def config_resolver(self) -> ConfigResolver: + """Return a typed config resolver backed by SettingsService. + + Creates a :class:`ConfigResolver` wrapping the settings service + and the YAML-loaded config. Raises 503 if the settings service + is not configured. + """ + return ConfigResolver( + settings_service=self.settings_service, + config=self.config, + ) + @property def has_auth_service(self) -> bool: """Check whether the auth service is already configured.""" diff --git a/src/synthorg/settings/__init__.py b/src/synthorg/settings/__init__.py index a8876b8a73..8e6cd2409b 100644 --- a/src/synthorg/settings/__init__.py +++ b/src/synthorg/settings/__init__.py @@ -18,8 +18,10 @@ ) from synthorg.settings.models import SettingDefinition, SettingEntry, SettingValue from synthorg.settings.registry import SettingsRegistry, get_registry +from synthorg.settings.resolver import ConfigResolver __all__ = [ + "ConfigResolver", "SettingDefinition", "SettingEntry", "SettingLevel", diff --git a/src/synthorg/settings/definitions/budget.py b/src/synthorg/settings/definitions/budget.py index 69cb37edca..ac95c560d2 100644 --- a/src/synthorg/settings/definitions/budget.py +++ b/src/synthorg/settings/definitions/budget.py @@ -72,3 +72,63 @@ yaml_path="budget.auto_downgrade.threshold", ) ) + +_r.register( + SettingDefinition( + namespace=SettingNamespace.BUDGET, + key="reset_day", + type=SettingType.INTEGER, + default="1", + description="Day of month when budget resets (1-28)", + group="Limits", + level=SettingLevel.ADVANCED, + min_value=1, + max_value=28, + yaml_path="budget.reset_day", + ) +) + +_r.register( + SettingDefinition( + namespace=SettingNamespace.BUDGET, + key="alert_warn_at", + type=SettingType.INTEGER, + default="75", + description="Budget usage percent that triggers a warning alert", + group="Alerts", + level=SettingLevel.ADVANCED, + min_value=0, + max_value=100, + yaml_path="budget.alerts.warn_at", + ) +) + +_r.register( + SettingDefinition( + namespace=SettingNamespace.BUDGET, + key="alert_critical_at", + type=SettingType.INTEGER, + default="90", + description="Budget usage percent that triggers a critical alert", + group="Alerts", + level=SettingLevel.ADVANCED, + min_value=0, + max_value=100, + yaml_path="budget.alerts.critical_at", + ) +) + +_r.register( + SettingDefinition( + namespace=SettingNamespace.BUDGET, + key="alert_hard_stop_at", + type=SettingType.INTEGER, + default="100", + description="Budget usage percent that triggers a hard stop", + group="Alerts", + level=SettingLevel.ADVANCED, + min_value=0, + max_value=100, + yaml_path="budget.alerts.hard_stop_at", + ) +) diff --git a/src/synthorg/settings/definitions/company.py b/src/synthorg/settings/definitions/company.py index c2a0b9a1b8..fab84afc05 100644 --- a/src/synthorg/settings/definitions/company.py +++ b/src/synthorg/settings/definitions/company.py @@ -27,12 +27,12 @@ description="Default company-wide autonomy level", group="General", enum_values=( - "full_autonomy", + "full", + "semi", "supervised", - "approval_required", - "human_in_the_loop", + "locked", ), - yaml_path="config.autonomy_level", + yaml_path="config.autonomy.level", ) ) diff --git a/src/synthorg/settings/definitions/coordination.py b/src/synthorg/settings/definitions/coordination.py index 47bb43bf13..d72162989a 100644 --- a/src/synthorg/settings/definitions/coordination.py +++ b/src/synthorg/settings/definitions/coordination.py @@ -21,7 +21,7 @@ "decentralized", "context_dependent", ), - yaml_path="coordination.default_topology", + yaml_path="coordination.topology", ) ) @@ -36,6 +36,42 @@ level=SettingLevel.ADVANCED, min_value=1, max_value=50, - yaml_path="coordination.max_wave_size", + yaml_path="coordination.max_concurrency_per_wave", + ) +) + +_r.register( + SettingDefinition( + namespace=SettingNamespace.COORDINATION, + key="fail_fast", + type=SettingType.BOOLEAN, + default="false", + description="Stop on first wave failure instead of continuing", + group="General", + yaml_path="coordination.fail_fast", + ) +) + +_r.register( + SettingDefinition( + namespace=SettingNamespace.COORDINATION, + key="enable_workspace_isolation", + type=SettingType.BOOLEAN, + default="true", + description="Create isolated workspaces for multi-agent execution", + group="General", + yaml_path="coordination.enable_workspace_isolation", + ) +) + +_r.register( + SettingDefinition( + namespace=SettingNamespace.COORDINATION, + key="base_branch", + type=SettingType.STRING, + default="main", + description="Git branch for workspace isolation", + group="General", + yaml_path="coordination.base_branch", ) ) diff --git a/src/synthorg/settings/resolver.py b/src/synthorg/settings/resolver.py new file mode 100644 index 0000000000..96a672deb9 --- /dev/null +++ b/src/synthorg/settings/resolver.py @@ -0,0 +1,282 @@ +"""Config resolver — typed config access backed by SettingsService. + +Bridges the gap between :class:`SettingsService` (which returns string +values) and consumers that need typed Python objects. Provides scalar +accessors and composed-read methods that assemble full Pydantic config +models from individually resolved settings. +""" + +from enum import StrEnum +from typing import TYPE_CHECKING + +from synthorg.observability import get_logger +from synthorg.observability.events.settings import SETTINGS_VALUE_RESOLVED + +if TYPE_CHECKING: + from synthorg.budget.config import BudgetConfig + from synthorg.config.schema import RootConfig + from synthorg.core.enums import AutonomyLevel + from synthorg.engine.coordination.config import CoordinationConfig + from synthorg.settings.service import SettingsService + +logger = get_logger(__name__) + + +class ConfigResolver: + """Typed config accessor backed by :class:`SettingsService`. + + Scalar accessors call ``SettingsService.get()`` and coerce the + string result to the requested Python type. + + Composed-read methods assemble full Pydantic config models by + reading individual settings and merging them onto a base config + loaded from YAML (for fields not yet in the settings registry). + + Args: + settings_service: The settings service for value resolution. + config: Root company configuration used as the base for + composed reads (provides defaults for unregistered fields). + """ + + def __init__( + self, + *, + settings_service: SettingsService, + config: RootConfig, + ) -> None: + self._settings = settings_service + self._config = config + + async def get_str(self, namespace: str, key: str) -> str: + """Resolve a setting as a string. + + Args: + namespace: Setting namespace. + key: Setting key. + + Returns: + The resolved value. + + Raises: + SettingNotFoundError: If the key is not in the registry. + """ + result = await self._settings.get(namespace, key) + return result.value + + async def get_int(self, namespace: str, key: str) -> int: + """Resolve a setting as an integer. + + Args: + namespace: Setting namespace. + key: Setting key. + + Returns: + The resolved value as an ``int``. + + Raises: + SettingNotFoundError: If the key is not in the registry. + ValueError: If the value cannot be parsed as an integer. + """ + result = await self._settings.get(namespace, key) + return int(result.value) + + async def get_float(self, namespace: str, key: str) -> float: + """Resolve a setting as a float. + + Args: + namespace: Setting namespace. + key: Setting key. + + Returns: + The resolved value as a ``float``. + + Raises: + SettingNotFoundError: If the key is not in the registry. + ValueError: If the value cannot be parsed as a float. + """ + result = await self._settings.get(namespace, key) + return float(result.value) + + async def get_bool(self, namespace: str, key: str) -> bool: + """Resolve a setting as a boolean. + + Accepts ``"true"``/``"false"``/``"1"``/``"0"`` + (case-insensitive). + + Args: + namespace: Setting namespace. + key: Setting key. + + Returns: + The resolved value as a ``bool``. + + Raises: + SettingNotFoundError: If the key is not in the registry. + ValueError: If the value is not a recognized boolean string. + """ + result = await self._settings.get(namespace, key) + return _parse_bool(result.value) + + async def get_enum[E: StrEnum]( + self, + namespace: str, + key: str, + enum_cls: type[E], + ) -> E: + """Resolve a setting as a ``StrEnum`` member. + + Args: + namespace: Setting namespace. + key: Setting key. + enum_cls: The enum class to coerce the value into. + + Returns: + The matching enum member. + + Raises: + SettingNotFoundError: If the key is not in the registry. + ValueError: If the value does not match any enum member. + """ + result = await self._settings.get(namespace, key) + return enum_cls(result.value) + + async def get_autonomy_level(self) -> AutonomyLevel: + """Resolve the company-wide default autonomy level. + + Returns: + The resolved ``AutonomyLevel`` enum member. + """ + from synthorg.core.enums import AutonomyLevel # noqa: PLC0415 + + return await self.get_enum("company", "autonomy_level", AutonomyLevel) + + async def get_budget_config(self) -> BudgetConfig: + """Assemble a ``BudgetConfig`` from individually resolved settings. + + Starts from the YAML-loaded base config and overrides fields + that have registered settings definitions. Unregistered fields + (e.g. ``downgrade_map``, ``boundary``) keep their YAML values. + + Returns: + A ``BudgetConfig`` with DB/env overrides applied. + """ + from synthorg.budget.config import ( # noqa: PLC0415 + BudgetAlertConfig, + ) + + base = self._config.budget + + total_monthly = await self.get_float("budget", "total_monthly") + per_task_limit = await self.get_float("budget", "per_task_limit") + per_agent_daily_limit = await self.get_float("budget", "per_agent_daily_limit") + auto_downgrade_enabled = await self.get_bool("budget", "auto_downgrade_enabled") + auto_downgrade_threshold = await self.get_int( + "budget", "auto_downgrade_threshold" + ) + reset_day = await self.get_int("budget", "reset_day") + alert_warn_at = await self.get_int("budget", "alert_warn_at") + alert_critical_at = await self.get_int("budget", "alert_critical_at") + alert_hard_stop_at = await self.get_int("budget", "alert_hard_stop_at") + + logger.debug( + SETTINGS_VALUE_RESOLVED, + namespace="budget", + key="_composed", + source="resolver", + ) + + return base.model_copy( + update={ + "total_monthly": total_monthly, + "per_task_limit": per_task_limit, + "per_agent_daily_limit": per_agent_daily_limit, + "reset_day": reset_day, + "alerts": BudgetAlertConfig( + warn_at=alert_warn_at, + critical_at=alert_critical_at, + hard_stop_at=alert_hard_stop_at, + ), + "auto_downgrade": base.auto_downgrade.model_copy( + update={ + "enabled": auto_downgrade_enabled, + "threshold": auto_downgrade_threshold, + }, + ), + }, + ) + + async def get_coordination_config( + self, + *, + max_concurrency_per_wave: int | None = None, + fail_fast: bool | None = None, + ) -> CoordinationConfig: + """Assemble a per-run ``CoordinationConfig`` from settings. + + Resolves coordination settings from the settings service, then + applies request-level overrides on top. + + Args: + max_concurrency_per_wave: Request-level override for max + concurrency (takes precedence over the setting value). + fail_fast: Request-level override for fail-fast behaviour. + + Returns: + A ``CoordinationConfig`` with settings + request overrides. + """ + from synthorg.engine.coordination.config import ( # noqa: PLC0415 + CoordinationConfig, + ) + + settings_max_wave = await self.get_int("coordination", "max_wave_size") + settings_fail_fast = await self.get_bool("coordination", "fail_fast") + settings_isolation = await self.get_bool( + "coordination", "enable_workspace_isolation" + ) + settings_branch = await self.get_str("coordination", "base_branch") + + logger.debug( + SETTINGS_VALUE_RESOLVED, + namespace="coordination", + key="_composed", + source="resolver", + ) + + return CoordinationConfig( + max_concurrency_per_wave=( + max_concurrency_per_wave + if max_concurrency_per_wave is not None + else settings_max_wave + ), + fail_fast=(fail_fast if fail_fast is not None else settings_fail_fast), + enable_workspace_isolation=settings_isolation, + base_branch=settings_branch, + ) + + +_BOOL_TRUE = frozenset({"true", "1"}) +_BOOL_FALSE = frozenset({"false", "0"}) + + +def _parse_bool(value: str) -> bool: + """Parse a string into a boolean. + + Accepts ``"true"``/``"false"``/``"1"``/``"0"`` + (case-insensitive). + + Args: + value: String to parse. + + Returns: + The parsed boolean. + + Raises: + ValueError: If the string is not a recognised boolean. + """ + lower = value.lower() + if lower in _BOOL_TRUE: + return True + if lower in _BOOL_FALSE: + return False + msg = f"Cannot parse {value!r} as boolean" + raise ValueError(msg) diff --git a/tests/integration/engine/test_coordination_wiring.py b/tests/integration/engine/test_coordination_wiring.py index 40bcdf030c..77164eb306 100644 --- a/tests/integration/engine/test_coordination_wiring.py +++ b/tests/integration/engine/test_coordination_wiring.py @@ -267,10 +267,20 @@ async def _mock_coordinate(context): # type: ignore[no-untyped-def] await registry.register(agent_alice) # 5. Build app + import synthorg.settings.definitions # noqa: F401 + from synthorg.settings.registry import get_registry + from synthorg.settings.service import SettingsService + backend = FakePersistenceBackend() auth_service = AuthService(AuthConfig(jwt_secret=_TEST_JWT_SECRET)) _seed_test_users(backend, auth_service) + settings_service = SettingsService( + repository=backend.settings, + registry=get_registry(), + config=config, + ) + app = create_app( config=config, persistence=backend, @@ -280,6 +290,7 @@ async def _mock_coordinate(context): # type: ignore[no-untyped-def] task_engine=task_engine, coordinator=coordinator, agent_registry=registry, + settings_service=settings_service, ) # 6. Use TestClient diff --git a/tests/unit/api/controllers/test_coordination.py b/tests/unit/api/controllers/test_coordination.py index 3e47b7a0b8..9182fa9185 100644 --- a/tests/unit/api/controllers/test_coordination.py +++ b/tests/unit/api/controllers/test_coordination.py @@ -111,8 +111,19 @@ def coordination_client( message_bus=EngineMessageBus(), # type: ignore[arg-type] ) + import synthorg.settings.definitions # noqa: F401 — trigger registration + from synthorg.settings.registry import get_registry + from synthorg.settings.service import SettingsService + + root_config = RootConfig(company_name="test") + settings_service = SettingsService( + repository=fake_persistence.settings, + registry=get_registry(), + config=root_config, + ) + app = create_app( - config=RootConfig(company_name="test"), + config=root_config, persistence=fake_persistence, message_bus=fake_message_bus, cost_tracker=CostTracker(), @@ -120,6 +131,7 @@ def coordination_client( task_engine=task_engine, coordinator=mock_coordinator, agent_registry=agent_registry, + settings_service=settings_service, ) with TestClient(app) as client: client.headers.update(make_auth_headers("ceo")) diff --git a/tests/unit/settings/test_resolver.py b/tests/unit/settings/test_resolver.py new file mode 100644 index 0000000000..7eaf87533e --- /dev/null +++ b/tests/unit/settings/test_resolver.py @@ -0,0 +1,534 @@ +"""Unit tests for ConfigResolver.""" + +from enum import StrEnum +from unittest.mock import AsyncMock + +import pytest +from hypothesis import given, settings +from hypothesis import strategies as st +from pydantic import BaseModel, ConfigDict + +from synthorg.settings.enums import SettingNamespace, SettingSource +from synthorg.settings.errors import SettingNotFoundError +from synthorg.settings.models import SettingValue +from synthorg.settings.resolver import ConfigResolver, _parse_bool + +# ── Helpers ─────────────────────────────────────────────────────── + + +class _Color(StrEnum): + RED = "red" + GREEN = "green" + BLUE = "blue" + + +def _make_value( + value: str, + namespace: str = "budget", + key: str = "total_monthly", +) -> SettingValue: + return SettingValue( + namespace=SettingNamespace(namespace) + if namespace in SettingNamespace.__members__.values() + else SettingNamespace.BUDGET, + key=key, + value=value, + source=SettingSource.DEFAULT, + ) + + +class _BudgetAlerts(BaseModel): + model_config = ConfigDict(frozen=True) + warn_at: int = 75 + critical_at: int = 90 + hard_stop_at: int = 100 + + +class _AutoDowngrade(BaseModel): + model_config = ConfigDict(frozen=True) + enabled: bool = False + threshold: int = 85 + downgrade_map: tuple[tuple[str, str], ...] = () + boundary: str = "task_assignment" + + +class _BudgetConfig(BaseModel): + model_config = ConfigDict(frozen=True) + total_monthly: float = 100.0 + per_task_limit: float = 5.0 + per_agent_daily_limit: float = 10.0 + alerts: _BudgetAlerts = _BudgetAlerts() + auto_downgrade: _AutoDowngrade = _AutoDowngrade() + reset_day: int = 1 + + +class _CoordinationSection(BaseModel): + model_config = ConfigDict(frozen=True) + topology: str = "auto" + max_concurrency_per_wave: int | None = None + fail_fast: bool = False + enable_workspace_isolation: bool = True + base_branch: str = "main" + + +class _CompanyConfig(BaseModel): + model_config = ConfigDict(frozen=True) + + +class _FakeRootConfig(BaseModel): + model_config = ConfigDict(frozen=True) + budget: _BudgetConfig = _BudgetConfig() + coordination: _CoordinationSection = _CoordinationSection() + config: _CompanyConfig = _CompanyConfig() + + +# ── Fixtures ────────────────────────────────────────────────────── + + +@pytest.fixture +def mock_settings() -> AsyncMock: + return AsyncMock() + + +@pytest.fixture +def root_config() -> _FakeRootConfig: + return _FakeRootConfig() + + +@pytest.fixture +def resolver(mock_settings: AsyncMock, root_config: _FakeRootConfig) -> ConfigResolver: + return ConfigResolver( + settings_service=mock_settings, + config=root_config, # type: ignore[arg-type] + ) + + +# ── Scalar Accessor Tests ──────────────────────────────────────── + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestGetStr: + """Tests for get_str().""" + + async def test_returns_string_value( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("hello") + result = await resolver.get_str("test", "key") + assert result == "hello" + mock_settings.get.assert_awaited_once_with("test", "key") + + async def test_returns_empty_string( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("") + result = await resolver.get_str("test", "key") + assert result == "" + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestGetInt: + """Tests for get_int().""" + + async def test_parses_positive_int( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("42") + assert await resolver.get_int("test", "key") == 42 + + async def test_parses_negative_int( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("-7") + assert await resolver.get_int("test", "key") == -7 + + async def test_parses_zero( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("0") + assert await resolver.get_int("test", "key") == 0 + + async def test_invalid_raises_value_error( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("not_a_number") + with pytest.raises(ValueError, match="invalid literal"): + await resolver.get_int("test", "key") + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestGetFloat: + """Tests for get_float().""" + + async def test_parses_float( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("3.14") + result = await resolver.get_float("test", "key") + assert result == pytest.approx(3.14) + + async def test_parses_integer_string_as_float( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("100") + assert await resolver.get_float("test", "key") == 100.0 + + async def test_invalid_raises_value_error( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("abc") + with pytest.raises(ValueError, match="could not convert"): + await resolver.get_float("test", "key") + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestGetBool: + """Tests for get_bool().""" + + @pytest.mark.parametrize( + ("raw", "expected"), + [ + ("true", True), + ("True", True), + ("TRUE", True), + ("1", True), + ("false", False), + ("False", False), + ("FALSE", False), + ("0", False), + ], + ) + async def test_parses_valid_values( + self, + resolver: ConfigResolver, + mock_settings: AsyncMock, + raw: str, + expected: bool, + ) -> None: + mock_settings.get.return_value = _make_value(raw) + assert await resolver.get_bool("test", "key") is expected + + async def test_invalid_raises_value_error( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("maybe") + with pytest.raises(ValueError, match="Cannot parse"): + await resolver.get_bool("test", "key") + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestGetEnum: + """Tests for get_enum().""" + + async def test_parses_valid_enum( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("red") + result = await resolver.get_enum("test", "key", _Color) + assert result is _Color.RED + + async def test_invalid_raises_value_error( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value("purple") + with pytest.raises(ValueError, match="purple"): + await resolver.get_enum("test", "key", _Color) + + +# ── Error Propagation ───────────────────────────────────────────── + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestErrorPropagation: + """SettingNotFoundError should propagate from SettingsService.""" + + async def test_not_found_propagates( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.side_effect = SettingNotFoundError("nope") + with pytest.raises(SettingNotFoundError): + await resolver.get_str("bad", "key") + + +# ── Composed Read: Autonomy Level ───────────────────────────────── + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestGetAutonomyLevel: + """Tests for get_autonomy_level().""" + + async def test_resolves_autonomy_level( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.return_value = _make_value( + "supervised", namespace="company", key="autonomy_level" + ) + from synthorg.core.enums import AutonomyLevel + + result = await resolver.get_autonomy_level() + assert result is AutonomyLevel.SUPERVISED + mock_settings.get.assert_awaited_once_with("company", "autonomy_level") + + async def test_resolves_all_levels( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + from synthorg.core.enums import AutonomyLevel + + for level in AutonomyLevel: + mock_settings.get.return_value = _make_value(level.value) + result = await resolver.get_autonomy_level() + assert result is level + + +# ── Composed Read: Budget Config ────────────────────────────────── + + +def _budget_get_side_effect( + overrides: dict[tuple[str, str], str] | None = None, +) -> AsyncMock: + """Create a mock .get() that returns budget defaults with optional overrides.""" + defaults = { + ("budget", "total_monthly"): "100.0", + ("budget", "per_task_limit"): "5.0", + ("budget", "per_agent_daily_limit"): "10.0", + ("budget", "auto_downgrade_enabled"): "false", + ("budget", "auto_downgrade_threshold"): "85", + ("budget", "reset_day"): "1", + ("budget", "alert_warn_at"): "75", + ("budget", "alert_critical_at"): "90", + ("budget", "alert_hard_stop_at"): "100", + } + merged = {**defaults, **(overrides or {})} + + async def _get(ns: str, key: str) -> SettingValue: + value = merged.get((ns, key)) + if value is None: + msg = f"Unknown: {ns}/{key}" + raise SettingNotFoundError(msg) + return _make_value(value, namespace=ns, key=key) + + return AsyncMock(side_effect=_get) + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestGetBudgetConfig: + """Tests for get_budget_config() composed read.""" + + async def test_returns_budget_config_from_defaults( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get = _budget_get_side_effect() + result = await resolver.get_budget_config() + + assert result.total_monthly == 100.0 + assert result.per_task_limit == 5.0 + assert result.per_agent_daily_limit == 10.0 + assert result.auto_downgrade.enabled is False + assert result.auto_downgrade.threshold == 85 + assert result.reset_day == 1 + assert result.alerts.warn_at == 75 + assert result.alerts.critical_at == 90 + assert result.alerts.hard_stop_at == 100 + + async def test_db_overrides_take_precedence( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get = _budget_get_side_effect( + { + ("budget", "total_monthly"): "500.0", + ("budget", "per_task_limit"): "25.0", + ("budget", "auto_downgrade_enabled"): "true", + } + ) + result = await resolver.get_budget_config() + + assert result.total_monthly == 500.0 + assert result.per_task_limit == 25.0 + assert result.auto_downgrade.enabled is True + # Non-overridden fields keep defaults + assert result.per_agent_daily_limit == 10.0 + assert result.auto_downgrade.threshold == 85 + + async def test_preserves_unregistered_fields( + self, + mock_settings: AsyncMock, + ) -> None: + """Unregistered fields (downgrade_map, boundary) keep YAML values.""" + custom_config = _FakeRootConfig( + budget=_BudgetConfig( + auto_downgrade=_AutoDowngrade( + downgrade_map=(("large", "small"),), + boundary="task_assignment", + ), + ), + ) + resolver = ConfigResolver( + settings_service=mock_settings, + config=custom_config, # type: ignore[arg-type] + ) + mock_settings.get = _budget_get_side_effect() + result = await resolver.get_budget_config() + + assert result.auto_downgrade.downgrade_map == (("large", "small"),) + assert result.auto_downgrade.boundary == "task_assignment" + + +# ── Composed Read: Coordination Config ──────────────────────────── + + +def _coordination_get_side_effect( + overrides: dict[tuple[str, str], str] | None = None, +) -> AsyncMock: + """Create a mock .get() for coordination settings.""" + defaults = { + ("coordination", "max_wave_size"): "5", + ("coordination", "fail_fast"): "false", + ("coordination", "enable_workspace_isolation"): "true", + ("coordination", "base_branch"): "main", + } + merged = {**defaults, **(overrides or {})} + + async def _get(ns: str, key: str) -> SettingValue: + value = merged.get((ns, key)) + if value is None: + msg = f"Unknown: {ns}/{key}" + raise SettingNotFoundError(msg) + return _make_value(value, namespace=ns, key=key) + + return AsyncMock(side_effect=_get) + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestGetCoordinationConfig: + """Tests for get_coordination_config() composed read.""" + + async def test_returns_config_from_defaults( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get = _coordination_get_side_effect() + result = await resolver.get_coordination_config() + + assert result.max_concurrency_per_wave == 5 + assert result.fail_fast is False + assert result.enable_workspace_isolation is True + assert result.base_branch == "main" + + async def test_request_overrides_take_precedence( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get = _coordination_get_side_effect() + result = await resolver.get_coordination_config( + max_concurrency_per_wave=10, + fail_fast=True, + ) + + assert result.max_concurrency_per_wave == 10 + assert result.fail_fast is True + # Non-overridden settings stay + assert result.enable_workspace_isolation is True + assert result.base_branch == "main" + + async def test_db_overrides_via_settings( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get = _coordination_get_side_effect( + { + ("coordination", "fail_fast"): "true", + ("coordination", "base_branch"): "develop", + } + ) + result = await resolver.get_coordination_config() + + assert result.fail_fast is True + assert result.base_branch == "develop" + + async def test_request_overrides_beat_db_overrides( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + """Request-level overrides beat DB overrides for supported fields.""" + mock_settings.get = _coordination_get_side_effect( + {("coordination", "fail_fast"): "true"} + ) + result = await resolver.get_coordination_config(fail_fast=False) + assert result.fail_fast is False + + +# ── _parse_bool Tests ───────────────────────────────────────────── + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestParseBool: + """Tests for the _parse_bool helper.""" + + @pytest.mark.parametrize( + ("raw", "expected"), + [ + ("true", True), + ("True", True), + ("TRUE", True), + ("1", True), + ("false", False), + ("False", False), + ("FALSE", False), + ("0", False), + ], + ) + def test_valid_values(self, raw: str, expected: bool) -> None: + assert _parse_bool(raw) is expected + + @pytest.mark.parametrize("raw", ["yes", "no", "maybe", "", "2", "tru"]) + def test_invalid_values(self, raw: str) -> None: + with pytest.raises(ValueError, match="Cannot parse"): + _parse_bool(raw) + + +# ── Property-Based Tests (Hypothesis) ───────────────────────────── + + +def _make_resolver() -> tuple[ConfigResolver, AsyncMock]: + """Create a fresh resolver + mock for property-based tests.""" + mock = AsyncMock() + config = _FakeRootConfig() + resolver = ConfigResolver( + settings_service=mock, + config=config, # type: ignore[arg-type] + ) + return resolver, mock + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestPropertyBased: + """Hypothesis-based roundtrip tests for scalar accessors.""" + + @given(st.integers(min_value=-(2**31), max_value=2**31)) + @settings(max_examples=200) + async def test_int_roundtrip(self, n: int) -> None: + resolver, mock = _make_resolver() + mock.get = AsyncMock(return_value=_make_value(str(n))) + assert await resolver.get_int("budget", "total_monthly") == n + + @given(st.floats(allow_nan=False, allow_infinity=False, allow_subnormal=False)) + @settings(max_examples=200) + async def test_float_roundtrip(self, x: float) -> None: + resolver, mock = _make_resolver() + mock.get = AsyncMock(return_value=_make_value(str(x))) + result = await resolver.get_float("budget", "total_monthly") + assert result == pytest.approx(x, rel=1e-9, abs=1e-15) + + @given(st.booleans()) + @settings(max_examples=200) + async def test_bool_roundtrip(self, b: bool) -> None: + resolver, mock = _make_resolver() + mock.get = AsyncMock(return_value=_make_value(str(b))) + assert await resolver.get_bool("budget", "total_monthly") is b From ca3de14d01690c9edbe27a2239225e0dd61530c8 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Mon, 16 Mar 2026 23:32:55 +0100 Subject: [PATCH 2/8] refactor: address review findings (TaskGroup, caching, logging, tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use asyncio.TaskGroup for parallel setting reads in composed methods - Cache ConfigResolver in AppState instead of creating per-access - Add warning logs before raising ValueError in scalar accessors - Fix autonomy_level default ("supervised" → "semi") to match model - Add error propagation tests for composed reads (ExceptionGroup) - Update CLAUDE.md Package Structure to document ConfigResolver Pre-reviewed by 3 agents, 9 findings addressed. --- CLAUDE.md | 2 +- src/synthorg/api/state.py | 18 ++- src/synthorg/settings/definitions/company.py | 2 +- src/synthorg/settings/resolver.py | 121 +++++++++++++------ tests/unit/settings/test_resolver.py | 44 +++++++ 5 files changed, 138 insertions(+), 49 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7e260997a6..f1763aff7e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -128,7 +128,7 @@ src/synthorg/ persistence/ # Operational data persistence — pluggable PersistenceBackend protocol, SQLite initial, SettingsRepository (namespaced settings CRUD) (see Memory & Persistence design page) observability/ # Structured logging, correlation tracking, log sinks providers/ # LLM provider abstraction (LiteLLM adapter) - settings/ # Runtime-editable settings persistence (DB > env > YAML > code defaults), typed definitions (8 namespaces), Fernet encryption for sensitive values, config bridge, validation, registry, change notifications via message bus + settings/ # Runtime-editable settings persistence (DB > env > YAML > code defaults), typed definitions (8 namespaces), Fernet encryption for sensitive values, config bridge, ConfigResolver (typed composed reads for controllers), validation, registry, change notifications via message bus definitions/ # Per-namespace setting definitions (company, providers, memory, budget, security, coordination, observability, backup) security/ # SecOps agent, rule engine (soft-allow/hard-deny, fail-closed), audit log, output scanner, output scan response policies (redact/withhold/log-only/autonomy-tiered), risk classifier, risk tier classifier, action type registry, ToolInvoker security integration, progressive trust (4 strategies: disabled/weighted/per-category/milestone), autonomy levels (presets, resolver, change strategy), timeout policies (park/resume) templates/ # Pre-built company templates, personality presets, and builder diff --git a/src/synthorg/api/state.py b/src/synthorg/api/state.py index ccab206c5d..b7cec8cb39 100644 --- a/src/synthorg/api/state.py +++ b/src/synthorg/api/state.py @@ -53,6 +53,7 @@ class AppState: "_agent_registry", "_approval_gate", "_auth_service", + "_config_resolver", "_coordinator", "_cost_tracker", "_meeting_orchestrator", @@ -101,6 +102,11 @@ def __init__( # noqa: PLR0913 self._meeting_orchestrator = meeting_orchestrator self._meeting_scheduler = meeting_scheduler self._settings_service = settings_service + self._config_resolver: ConfigResolver | None = ( + ConfigResolver(settings_service=settings_service, config=config) + if settings_service is not None + else None + ) self._ticket_store = WsTicketStore() self.startup_time = startup_time @@ -239,16 +245,8 @@ def has_settings_service(self) -> bool: @property def config_resolver(self) -> ConfigResolver: - """Return a typed config resolver backed by SettingsService. - - Creates a :class:`ConfigResolver` wrapping the settings service - and the YAML-loaded config. Raises 503 if the settings service - is not configured. - """ - return ConfigResolver( - settings_service=self.settings_service, - config=self.config, - ) + """Return the cached config resolver or raise 503.""" + return self._require_service(self._config_resolver, "config_resolver") @property def has_auth_service(self) -> bool: diff --git a/src/synthorg/settings/definitions/company.py b/src/synthorg/settings/definitions/company.py index fab84afc05..5cf9468804 100644 --- a/src/synthorg/settings/definitions/company.py +++ b/src/synthorg/settings/definitions/company.py @@ -23,7 +23,7 @@ namespace=SettingNamespace.COMPANY, key="autonomy_level", type=SettingType.ENUM, - default="supervised", + default="semi", description="Default company-wide autonomy level", group="General", enum_values=( diff --git a/src/synthorg/settings/resolver.py b/src/synthorg/settings/resolver.py index 96a672deb9..e428d59152 100644 --- a/src/synthorg/settings/resolver.py +++ b/src/synthorg/settings/resolver.py @@ -6,11 +6,15 @@ models from individually resolved settings. """ +import asyncio from enum import StrEnum from typing import TYPE_CHECKING from synthorg.observability import get_logger -from synthorg.observability.events.settings import SETTINGS_VALUE_RESOLVED +from synthorg.observability.events.settings import ( + SETTINGS_VALIDATION_FAILED, + SETTINGS_VALUE_RESOLVED, +) if TYPE_CHECKING: from synthorg.budget.config import BudgetConfig @@ -78,7 +82,16 @@ async def get_int(self, namespace: str, key: str) -> int: ValueError: If the value cannot be parsed as an integer. """ result = await self._settings.get(namespace, key) - return int(result.value) + try: + return int(result.value) + except ValueError: + logger.warning( + SETTINGS_VALIDATION_FAILED, + namespace=namespace, + key=key, + reason="invalid_integer", + ) + raise async def get_float(self, namespace: str, key: str) -> float: """Resolve a setting as a float. @@ -95,7 +108,16 @@ async def get_float(self, namespace: str, key: str) -> float: ValueError: If the value cannot be parsed as a float. """ result = await self._settings.get(namespace, key) - return float(result.value) + try: + return float(result.value) + except ValueError: + logger.warning( + SETTINGS_VALIDATION_FAILED, + namespace=namespace, + key=key, + reason="invalid_float", + ) + raise async def get_bool(self, namespace: str, key: str) -> bool: """Resolve a setting as a boolean. @@ -115,7 +137,16 @@ async def get_bool(self, namespace: str, key: str) -> bool: ValueError: If the value is not a recognized boolean string. """ result = await self._settings.get(namespace, key) - return _parse_bool(result.value) + try: + return _parse_bool(result.value) + except ValueError: + logger.warning( + SETTINGS_VALIDATION_FAILED, + namespace=namespace, + key=key, + reason="invalid_boolean", + ) + raise async def get_enum[E: StrEnum]( self, @@ -138,7 +169,16 @@ async def get_enum[E: StrEnum]( ValueError: If the value does not match any enum member. """ result = await self._settings.get(namespace, key) - return enum_cls(result.value) + try: + return enum_cls(result.value) + except ValueError: + logger.warning( + SETTINGS_VALIDATION_FAILED, + namespace=namespace, + key=key, + reason="invalid_enum", + ) + raise async def get_autonomy_level(self) -> AutonomyLevel: """Resolve the company-wide default autonomy level. @@ -157,6 +197,8 @@ async def get_budget_config(self) -> BudgetConfig: that have registered settings definitions. Unregistered fields (e.g. ``downgrade_map``, ``boundary``) keep their YAML values. + Uses ``asyncio.TaskGroup`` to resolve all settings in parallel. + Returns: A ``BudgetConfig`` with DB/env overrides applied. """ @@ -166,17 +208,20 @@ async def get_budget_config(self) -> BudgetConfig: base = self._config.budget - total_monthly = await self.get_float("budget", "total_monthly") - per_task_limit = await self.get_float("budget", "per_task_limit") - per_agent_daily_limit = await self.get_float("budget", "per_agent_daily_limit") - auto_downgrade_enabled = await self.get_bool("budget", "auto_downgrade_enabled") - auto_downgrade_threshold = await self.get_int( - "budget", "auto_downgrade_threshold" - ) - reset_day = await self.get_int("budget", "reset_day") - alert_warn_at = await self.get_int("budget", "alert_warn_at") - alert_critical_at = await self.get_int("budget", "alert_critical_at") - alert_hard_stop_at = await self.get_int("budget", "alert_hard_stop_at") + async with asyncio.TaskGroup() as tg: + t_monthly = tg.create_task(self.get_float("budget", "total_monthly")) + t_per_task = tg.create_task(self.get_float("budget", "per_task_limit")) + t_daily = tg.create_task(self.get_float("budget", "per_agent_daily_limit")) + t_downgrade_en = tg.create_task( + self.get_bool("budget", "auto_downgrade_enabled") + ) + t_downgrade_th = tg.create_task( + self.get_int("budget", "auto_downgrade_threshold") + ) + t_reset = tg.create_task(self.get_int("budget", "reset_day")) + t_warn = tg.create_task(self.get_int("budget", "alert_warn_at")) + t_crit = tg.create_task(self.get_int("budget", "alert_critical_at")) + t_stop = tg.create_task(self.get_int("budget", "alert_hard_stop_at")) logger.debug( SETTINGS_VALUE_RESOLVED, @@ -187,19 +232,19 @@ async def get_budget_config(self) -> BudgetConfig: return base.model_copy( update={ - "total_monthly": total_monthly, - "per_task_limit": per_task_limit, - "per_agent_daily_limit": per_agent_daily_limit, - "reset_day": reset_day, + "total_monthly": t_monthly.result(), + "per_task_limit": t_per_task.result(), + "per_agent_daily_limit": t_daily.result(), + "reset_day": t_reset.result(), "alerts": BudgetAlertConfig( - warn_at=alert_warn_at, - critical_at=alert_critical_at, - hard_stop_at=alert_hard_stop_at, + warn_at=t_warn.result(), + critical_at=t_crit.result(), + hard_stop_at=t_stop.result(), ), "auto_downgrade": base.auto_downgrade.model_copy( update={ - "enabled": auto_downgrade_enabled, - "threshold": auto_downgrade_threshold, + "enabled": t_downgrade_en.result(), + "threshold": t_downgrade_th.result(), }, ), }, @@ -213,8 +258,9 @@ async def get_coordination_config( ) -> CoordinationConfig: """Assemble a per-run ``CoordinationConfig`` from settings. - Resolves coordination settings from the settings service, then - applies request-level overrides on top. + Resolves coordination settings from the settings service using + ``asyncio.TaskGroup`` for parallel resolution, then applies + request-level overrides on top. Args: max_concurrency_per_wave: Request-level override for max @@ -228,12 +274,13 @@ async def get_coordination_config( CoordinationConfig, ) - settings_max_wave = await self.get_int("coordination", "max_wave_size") - settings_fail_fast = await self.get_bool("coordination", "fail_fast") - settings_isolation = await self.get_bool( - "coordination", "enable_workspace_isolation" - ) - settings_branch = await self.get_str("coordination", "base_branch") + async with asyncio.TaskGroup() as tg: + t_wave = tg.create_task(self.get_int("coordination", "max_wave_size")) + t_ff = tg.create_task(self.get_bool("coordination", "fail_fast")) + t_iso = tg.create_task( + self.get_bool("coordination", "enable_workspace_isolation") + ) + t_branch = tg.create_task(self.get_str("coordination", "base_branch")) logger.debug( SETTINGS_VALUE_RESOLVED, @@ -246,11 +293,11 @@ async def get_coordination_config( max_concurrency_per_wave=( max_concurrency_per_wave if max_concurrency_per_wave is not None - else settings_max_wave + else t_wave.result() ), - fail_fast=(fail_fast if fail_fast is not None else settings_fail_fast), - enable_workspace_isolation=settings_isolation, - base_branch=settings_branch, + fail_fast=(fail_fast if fail_fast is not None else t_ff.result()), + enable_workspace_isolation=t_iso.result(), + base_branch=t_branch.result(), ) diff --git a/tests/unit/settings/test_resolver.py b/tests/unit/settings/test_resolver.py index 7eaf87533e..2771208560 100644 --- a/tests/unit/settings/test_resolver.py +++ b/tests/unit/settings/test_resolver.py @@ -380,6 +380,28 @@ async def test_preserves_unregistered_fields( assert result.auto_downgrade.downgrade_map == (("large", "small"),) assert result.auto_downgrade.boundary == "task_assignment" + async def test_not_found_propagates( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + """SettingNotFoundError from a budget key propagates via ExceptionGroup.""" + mock_settings.get.side_effect = SettingNotFoundError("missing") + with pytest.raises(ExceptionGroup) as exc_info: + await resolver.get_budget_config() + assert any( + isinstance(e, SettingNotFoundError) for e in exc_info.value.exceptions + ) + + async def test_value_error_propagates( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + """ValueError from a corrupted DB value propagates via ExceptionGroup.""" + mock_settings.get = _budget_get_side_effect( + {("budget", "total_monthly"): "not-a-number"} + ) + with pytest.raises(ExceptionGroup) as exc_info: + await resolver.get_budget_config() + assert any(isinstance(e, ValueError) for e in exc_info.value.exceptions) + # ── Composed Read: Coordination Config ──────────────────────────── @@ -461,6 +483,28 @@ async def test_request_overrides_beat_db_overrides( result = await resolver.get_coordination_config(fail_fast=False) assert result.fail_fast is False + async def test_not_found_propagates( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + """SettingNotFoundError from a coordination key propagates.""" + mock_settings.get.side_effect = SettingNotFoundError("missing") + with pytest.raises(ExceptionGroup) as exc_info: + await resolver.get_coordination_config() + assert any( + isinstance(e, SettingNotFoundError) for e in exc_info.value.exceptions + ) + + async def test_value_error_propagates( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + """ValueError from a corrupted coordination value propagates.""" + mock_settings.get = _coordination_get_side_effect( + {("coordination", "max_wave_size"): "not-a-number"} + ) + with pytest.raises(ExceptionGroup) as exc_info: + await resolver.get_coordination_config() + assert any(isinstance(e, ValueError) for e in exc_info.value.exceptions) + # ── _parse_bool Tests ───────────────────────────────────────────── From 25095d16a167b719c7b16468da28b94473552801 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Mon, 16 Mar 2026 23:38:42 +0100 Subject: [PATCH 3/8] refactor: address remaining review findings (naming, tests, cleanup) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename TestPropertyBased → TestResolverScalarProperties for -k properties - Simplify _make_value helper to use SettingNamespace directly - Add AppState.config_resolver unit tests (raises 503 / returns resolver) --- tests/unit/api/test_state.py | 20 ++++++++++++++++++++ tests/unit/settings/test_resolver.py | 14 ++++++-------- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/tests/unit/api/test_state.py b/tests/unit/api/test_state.py index 9405f086f2..92f55b45f8 100644 --- a/tests/unit/api/test_state.py +++ b/tests/unit/api/test_state.py @@ -220,3 +220,23 @@ async def test_has_message_bus_true_when_set(self) -> None: await bus.start() state = _make_state(message_bus=bus) assert state.has_message_bus is True + + +@pytest.mark.unit +class TestAppStateConfigResolver: + """Tests for config_resolver property.""" + + def test_config_resolver_raises_when_settings_service_none(self) -> None: + state = _make_state(settings_service=None) + with pytest.raises(ServiceUnavailableError): + _ = state.config_resolver + + def test_config_resolver_returns_when_settings_service_set(self) -> None: + from unittest.mock import MagicMock + + from synthorg.settings.resolver import ConfigResolver + + mock_svc = MagicMock() + state = _make_state(settings_service=mock_svc) + resolver = state.config_resolver + assert isinstance(resolver, ConfigResolver) diff --git a/tests/unit/settings/test_resolver.py b/tests/unit/settings/test_resolver.py index 2771208560..3fc480f5cf 100644 --- a/tests/unit/settings/test_resolver.py +++ b/tests/unit/settings/test_resolver.py @@ -24,13 +24,11 @@ class _Color(StrEnum): def _make_value( value: str, - namespace: str = "budget", + namespace: SettingNamespace = SettingNamespace.BUDGET, key: str = "total_monthly", ) -> SettingValue: return SettingValue( - namespace=SettingNamespace(namespace) - if namespace in SettingNamespace.__members__.values() - else SettingNamespace.BUDGET, + namespace=namespace, key=key, value=value, source=SettingSource.DEFAULT, @@ -268,7 +266,7 @@ async def test_resolves_autonomy_level( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: mock_settings.get.return_value = _make_value( - "supervised", namespace="company", key="autonomy_level" + "supervised", namespace=SettingNamespace.COMPANY, key="autonomy_level" ) from synthorg.core.enums import AutonomyLevel @@ -312,7 +310,7 @@ async def _get(ns: str, key: str) -> SettingValue: if value is None: msg = f"Unknown: {ns}/{key}" raise SettingNotFoundError(msg) - return _make_value(value, namespace=ns, key=key) + return _make_value(value, namespace=SettingNamespace(ns), key=key) return AsyncMock(side_effect=_get) @@ -423,7 +421,7 @@ async def _get(ns: str, key: str) -> SettingValue: if value is None: msg = f"Unknown: {ns}/{key}" raise SettingNotFoundError(msg) - return _make_value(value, namespace=ns, key=key) + return _make_value(value, namespace=SettingNamespace(ns), key=key) return AsyncMock(side_effect=_get) @@ -552,7 +550,7 @@ def _make_resolver() -> tuple[ConfigResolver, AsyncMock]: @pytest.mark.unit @pytest.mark.timeout(30) -class TestPropertyBased: +class TestResolverScalarProperties: """Hypothesis-based roundtrip tests for scalar accessors.""" @given(st.integers(min_value=-(2**31), max_value=2**31)) From cf377110bc029e083cdf8a9ead5fa3729ca96be1 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 07:14:26 +0100 Subject: [PATCH 4/8] fix: address 29 PR review items from local agents, CodeRabbit, and Gemini MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness: - Fix cache race in SettingsService._cache under concurrent TaskGroup reads (dict rebuild → direct key assignment) - Unwrap ExceptionGroup in composed-read methods so controllers see clean SettingNotFoundError/ValueError instead of opaque 500s - Catch ValidationError from BudgetAlertConfig ordering constraint - Raise SettingNotFoundError in _resolve_fallback when default is None (was silently returning empty string) - Add MemoryError/RecursionError guard to _publish_change broad except Naming & consistency: - Rename setting key max_wave_size → max_concurrency_per_wave - Fix 503 error message: "Settings Service" instead of "Config Resolver" - Use InternalServerException for encryption errors (was ClientException) - Add has_config_resolver property for naming symmetry Security: - Sanitize raw values in ValueError messages across all scalar accessors - Sanitize _parse_bool error message Documentation: - Add Raises sections to composed-read and get_autonomy_level docstrings - Fix module docstring (SettingValue not "string values") - Fix get_str Returns section, get_budget_config nested field paths - Add constructor TypeError guard + stale-config invariant note - Remove redundant composed-read debug logs - Document ConfigResolver in operations.md Settings section Tests: - Add @pytest.mark.unit to TestAppStateConfigResolver - Add has_settings_service flag tests (both paths) - Add config_resolver singleton identity assertion - Add has_config_resolver flag tests - Use AsyncMock for async service in tests - Parametrize test_resolves_all_levels - Strengthen topology assertion (exact "sas" match) - Remove hardcoded @settings(max_examples=200) for Hypothesis profiles - Update error match patterns for sanitized messages - Update ExceptionGroup tests → expect unwrapped exceptions - Add constructor None guard test Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/design/operations.md | 2 +- src/synthorg/api/controllers/settings.py | 8 +- src/synthorg/api/state.py | 7 +- .../settings/definitions/coordination.py | 2 +- src/synthorg/settings/resolver.py | 173 ++++++++++++------ src/synthorg/settings/service.py | 16 +- .../engine/test_coordination_wiring.py | 5 +- tests/unit/api/test_state.py | 37 +++- tests/unit/settings/test_resolver.py | 88 +++++---- 9 files changed, 233 insertions(+), 105 deletions(-) diff --git a/docs/design/operations.md b/docs/design/operations.md index d8f3b1fa62..ae9b8e7bcd 100644 --- a/docs/design/operations.md +++ b/docs/design/operations.md @@ -1041,7 +1041,7 @@ and retry guidance. - **Budget Panel**: Spending charts, per-agent breakdown (projections/alerts planned) - **Meeting Logs**: Placeholder — coming soon - **Artifact Browser**: Placeholder — coming soon -- **Settings**: Runtime-editable configuration via DB-backed settings persistence (8 namespaces: company, providers, memory, budget, security, coordination, observability, backup). 4-layer resolution (DB > env > YAML > code defaults), Fernet encryption for sensitive values, REST API (GET/PUT/DELETE + schema endpoints for dynamic UI generation), change notifications via message bus +- **Settings**: Runtime-editable configuration via DB-backed settings persistence (8 namespaces: company, providers, memory, budget, security, coordination, observability, backup). 4-layer resolution (DB > env > YAML > code defaults), Fernet encryption for sensitive values, REST API (GET/PUT/DELETE + schema endpoints for dynamic UI generation), change notifications via message bus. `ConfigResolver` provides typed composed reads for API controllers (assembles full Pydantic config models from individually resolved settings, using `asyncio.TaskGroup` for parallel resolution) ### Human Roles diff --git a/src/synthorg/api/controllers/settings.py b/src/synthorg/api/controllers/settings.py index 4270c78310..e36606a723 100644 --- a/src/synthorg/api/controllers/settings.py +++ b/src/synthorg/api/controllers/settings.py @@ -2,7 +2,11 @@ from litestar import Controller, delete, get, put from litestar.datastructures import State # noqa: TC002 -from litestar.exceptions import ClientException, NotFoundException +from litestar.exceptions import ( + ClientException, + InternalServerException, + NotFoundException, +) from pydantic import BaseModel, ConfigDict, Field from synthorg.api.dto import ApiResponse @@ -187,7 +191,7 @@ async def update_setting( key=key, ) msg = "Internal error processing sensitive setting" - raise ClientException(msg, status_code=500) from None + raise InternalServerException(msg) from None return ApiResponse(data=entry) @delete( diff --git a/src/synthorg/api/state.py b/src/synthorg/api/state.py index b7cec8cb39..d5bfe98fae 100644 --- a/src/synthorg/api/state.py +++ b/src/synthorg/api/state.py @@ -243,10 +243,15 @@ def has_settings_service(self) -> bool: """Check whether the settings service is configured.""" return self._settings_service is not None + @property + def has_config_resolver(self) -> bool: + """Check whether the config resolver is configured.""" + return self._config_resolver is not None + @property def config_resolver(self) -> ConfigResolver: """Return the cached config resolver or raise 503.""" - return self._require_service(self._config_resolver, "config_resolver") + return self._require_service(self._config_resolver, "settings_service") @property def has_auth_service(self) -> bool: diff --git a/src/synthorg/settings/definitions/coordination.py b/src/synthorg/settings/definitions/coordination.py index d72162989a..c0e4a95bc7 100644 --- a/src/synthorg/settings/definitions/coordination.py +++ b/src/synthorg/settings/definitions/coordination.py @@ -28,7 +28,7 @@ _r.register( SettingDefinition( namespace=SettingNamespace.COORDINATION, - key="max_wave_size", + key="max_concurrency_per_wave", type=SettingType.INTEGER, default="5", description="Maximum number of agents in a single execution wave", diff --git a/src/synthorg/settings/resolver.py b/src/synthorg/settings/resolver.py index e428d59152..bb4220e70b 100644 --- a/src/synthorg/settings/resolver.py +++ b/src/synthorg/settings/resolver.py @@ -1,9 +1,10 @@ """Config resolver — typed config access backed by SettingsService. -Bridges the gap between :class:`SettingsService` (which returns string -values) and consumers that need typed Python objects. Provides scalar -accessors and composed-read methods that assemble full Pydantic config -models from individually resolved settings. +Bridges the gap between :class:`SettingsService` (which returns +:class:`~synthorg.settings.models.SettingValue` objects with a string +``.value``) and consumers that need typed Python objects. Provides +scalar accessors and composed-read methods that assemble full Pydantic +config models from individually resolved settings. """ import asyncio @@ -12,9 +13,10 @@ from synthorg.observability import get_logger from synthorg.observability.events.settings import ( + SETTINGS_NOT_FOUND, SETTINGS_VALIDATION_FAILED, - SETTINGS_VALUE_RESOLVED, ) +from synthorg.settings.errors import SettingNotFoundError if TYPE_CHECKING: from synthorg.budget.config import BudgetConfig @@ -36,10 +38,18 @@ class ConfigResolver: reading individual settings and merging them onto a base config loaded from YAML (for fields not yet in the settings registry). + The ``config`` snapshot is captured at construction time and is + **not** updated if the underlying ``RootConfig`` is replaced. + ``ConfigResolver`` and ``AppState`` must always hold the same + reference; see ``AppState.__init__`` for the wiring invariant. + Args: settings_service: The settings service for value resolution. config: Root company configuration used as the base for composed reads (provides defaults for unregistered fields). + + Raises: + TypeError: If *settings_service* is ``None``. """ def __init__( @@ -48,6 +58,10 @@ def __init__( settings_service: SettingsService, config: RootConfig, ) -> None: + # runtime defence for callers without type checking + if settings_service is None: + msg = "settings_service must not be None" # type: ignore[unreachable] + raise TypeError(msg) self._settings = settings_service self._config = config @@ -59,12 +73,20 @@ async def get_str(self, namespace: str, key: str) -> str: key: Setting key. Returns: - The resolved value. + The resolved value as a ``str``. Raises: SettingNotFoundError: If the key is not in the registry. """ - result = await self._settings.get(namespace, key) + try: + result = await self._settings.get(namespace, key) + except SettingNotFoundError: + logger.warning( + SETTINGS_NOT_FOUND, + namespace=namespace, + key=key, + ) + raise return result.value async def get_int(self, namespace: str, key: str) -> int: @@ -90,8 +112,10 @@ async def get_int(self, namespace: str, key: str) -> int: namespace=namespace, key=key, reason="invalid_integer", + exc_info=True, ) - raise + msg = f"Setting {namespace}/{key} has an invalid integer value" + raise ValueError(msg) from None async def get_float(self, namespace: str, key: str) -> float: """Resolve a setting as a float. @@ -116,14 +140,15 @@ async def get_float(self, namespace: str, key: str) -> float: namespace=namespace, key=key, reason="invalid_float", + exc_info=True, ) - raise + msg = f"Setting {namespace}/{key} has an invalid float value" + raise ValueError(msg) from None async def get_bool(self, namespace: str, key: str) -> bool: """Resolve a setting as a boolean. - Accepts ``"true"``/``"false"``/``"1"``/``"0"`` - (case-insensitive). + Accepted values are delegated to :func:`_parse_bool`. Args: namespace: Setting namespace. @@ -145,8 +170,10 @@ async def get_bool(self, namespace: str, key: str) -> bool: namespace=namespace, key=key, reason="invalid_boolean", + exc_info=True, ) - raise + msg = f"Setting {namespace}/{key} is not a recognized boolean" + raise ValueError(msg) from None async def get_enum[E: StrEnum]( self, @@ -177,14 +204,23 @@ async def get_enum[E: StrEnum]( namespace=namespace, key=key, reason="invalid_enum", + enum_cls=enum_cls.__name__, + exc_info=True, ) - raise + msg = f"Setting {namespace}/{key} has an invalid {enum_cls.__name__} value" + raise ValueError(msg) from None async def get_autonomy_level(self) -> AutonomyLevel: """Resolve the company-wide default autonomy level. Returns: The resolved ``AutonomyLevel`` enum member. + + Raises: + SettingNotFoundError: If the autonomy_level key is + not registered. + ValueError: If the stored value does not match any + ``AutonomyLevel`` member. """ from synthorg.core.enums import AutonomyLevel # noqa: PLC0415 @@ -195,40 +231,60 @@ async def get_budget_config(self) -> BudgetConfig: Starts from the YAML-loaded base config and overrides fields that have registered settings definitions. Unregistered fields - (e.g. ``downgrade_map``, ``boundary``) keep their YAML values. + on nested models (e.g. ``auto_downgrade.downgrade_map``, + ``auto_downgrade.boundary``) keep their YAML values. Uses ``asyncio.TaskGroup`` to resolve all settings in parallel. + If any individual resolution fails, the ``ExceptionGroup`` is + unwrapped and the first cause is re-raised directly. Returns: A ``BudgetConfig`` with DB/env overrides applied. + + Raises: + SettingNotFoundError: If a required budget setting is + missing from the registry. + ValueError: If a resolved value cannot be parsed or if + the assembled alert thresholds violate the ordering + constraint (``warn_at < critical_at < hard_stop_at``). """ + from pydantic import ValidationError # noqa: PLC0415 + from synthorg.budget.config import ( # noqa: PLC0415 BudgetAlertConfig, ) base = self._config.budget - async with asyncio.TaskGroup() as tg: - t_monthly = tg.create_task(self.get_float("budget", "total_monthly")) - t_per_task = tg.create_task(self.get_float("budget", "per_task_limit")) - t_daily = tg.create_task(self.get_float("budget", "per_agent_daily_limit")) - t_downgrade_en = tg.create_task( - self.get_bool("budget", "auto_downgrade_enabled") - ) - t_downgrade_th = tg.create_task( - self.get_int("budget", "auto_downgrade_threshold") + try: + async with asyncio.TaskGroup() as tg: + t_monthly = tg.create_task(self.get_float("budget", "total_monthly")) + t_per_task = tg.create_task(self.get_float("budget", "per_task_limit")) + t_daily = tg.create_task( + self.get_float("budget", "per_agent_daily_limit") + ) + t_downgrade_en = tg.create_task( + self.get_bool("budget", "auto_downgrade_enabled") + ) + t_downgrade_th = tg.create_task( + self.get_int("budget", "auto_downgrade_threshold") + ) + t_reset = tg.create_task(self.get_int("budget", "reset_day")) + t_warn = tg.create_task(self.get_int("budget", "alert_warn_at")) + t_crit = tg.create_task(self.get_int("budget", "alert_critical_at")) + t_stop = tg.create_task(self.get_int("budget", "alert_hard_stop_at")) + except ExceptionGroup as eg: + raise eg.exceptions[0] from eg + + try: + alerts = BudgetAlertConfig( + warn_at=t_warn.result(), + critical_at=t_crit.result(), + hard_stop_at=t_stop.result(), ) - t_reset = tg.create_task(self.get_int("budget", "reset_day")) - t_warn = tg.create_task(self.get_int("budget", "alert_warn_at")) - t_crit = tg.create_task(self.get_int("budget", "alert_critical_at")) - t_stop = tg.create_task(self.get_int("budget", "alert_hard_stop_at")) - - logger.debug( - SETTINGS_VALUE_RESOLVED, - namespace="budget", - key="_composed", - source="resolver", - ) + except ValidationError as exc: + msg = f"Budget alert thresholds violate ordering constraint: {exc}" + raise ValueError(msg) from exc return base.model_copy( update={ @@ -236,11 +292,7 @@ async def get_budget_config(self) -> BudgetConfig: "per_task_limit": t_per_task.result(), "per_agent_daily_limit": t_daily.result(), "reset_day": t_reset.result(), - "alerts": BudgetAlertConfig( - warn_at=t_warn.result(), - critical_at=t_crit.result(), - hard_stop_at=t_stop.result(), - ), + "alerts": alerts, "auto_downgrade": base.auto_downgrade.model_copy( update={ "enabled": t_downgrade_en.result(), @@ -260,7 +312,15 @@ async def get_coordination_config( Resolves coordination settings from the settings service using ``asyncio.TaskGroup`` for parallel resolution, then applies - request-level overrides on top. + request-level overrides on top. If any individual resolution + fails, the ``ExceptionGroup`` is unwrapped and the first cause + is re-raised directly. + + ``CoordinationConfig`` is constructed from scratch (not via + ``model_copy``) because all its fields are registered in the + settings registry. The ``default_topology`` setting is + resolved separately by the ``TopologyDispatcher`` and is not + part of ``CoordinationConfig``. Args: max_concurrency_per_wave: Request-level override for max @@ -269,25 +329,28 @@ async def get_coordination_config( Returns: A ``CoordinationConfig`` with settings + request overrides. + + Raises: + SettingNotFoundError: If a required coordination setting + is missing from the registry. + ValueError: If a resolved value cannot be parsed. """ from synthorg.engine.coordination.config import ( # noqa: PLC0415 CoordinationConfig, ) - async with asyncio.TaskGroup() as tg: - t_wave = tg.create_task(self.get_int("coordination", "max_wave_size")) - t_ff = tg.create_task(self.get_bool("coordination", "fail_fast")) - t_iso = tg.create_task( - self.get_bool("coordination", "enable_workspace_isolation") - ) - t_branch = tg.create_task(self.get_str("coordination", "base_branch")) - - logger.debug( - SETTINGS_VALUE_RESOLVED, - namespace="coordination", - key="_composed", - source="resolver", - ) + try: + async with asyncio.TaskGroup() as tg: + t_wave = tg.create_task( + self.get_int("coordination", "max_concurrency_per_wave") + ) + t_ff = tg.create_task(self.get_bool("coordination", "fail_fast")) + t_iso = tg.create_task( + self.get_bool("coordination", "enable_workspace_isolation") + ) + t_branch = tg.create_task(self.get_str("coordination", "base_branch")) + except ExceptionGroup as eg: + raise eg.exceptions[0] from eg return CoordinationConfig( max_concurrency_per_wave=( @@ -325,5 +388,5 @@ def _parse_bool(value: str) -> bool: return True if lower in _BOOL_FALSE: return False - msg = f"Cannot parse {value!r} as boolean" + msg = "Value is not a recognized boolean string" raise ValueError(msg) diff --git a/src/synthorg/settings/service.py b/src/synthorg/settings/service.py index f88055b8b2..284d9ad368 100644 --- a/src/synthorg/settings/service.py +++ b/src/synthorg/settings/service.py @@ -253,7 +253,7 @@ async def get(self, namespace: str, key: str) -> SettingValue: # Cache only non-sensitive values to avoid holding # plaintext secrets in memory. if not definition.sensitive: - self._cache = {**self._cache, cache_key: setting_value} + self._cache[cache_key] = setting_value logger.debug( SETTINGS_VALUE_RESOLVED, namespace=namespace, @@ -374,12 +374,20 @@ def _resolve_fallback( source=SettingSource.YAML, ) - default = definition.default if definition.default is not None else "" + if definition.default is None: + logger.warning( + SETTINGS_NOT_FOUND, + namespace=ns, + key=key, + reason="no_default_configured", + ) + msg = f"Setting {ns}/{key} has no configured value or default" + raise SettingNotFoundError(msg) logger.debug(SETTINGS_VALUE_RESOLVED, namespace=ns, key=key, source="default") return SettingValue( namespace=ns, key=key, - value=default, + value=definition.default, source=SettingSource.DEFAULT, ) @@ -612,6 +620,8 @@ async def _publish_change( namespace=namespace, key=key, ) + except MemoryError, RecursionError: + raise except Exception as exc: # Notification failure should not break settings writes. logger.warning( diff --git a/tests/integration/engine/test_coordination_wiring.py b/tests/integration/engine/test_coordination_wiring.py index 77164eb306..f1614d7da4 100644 --- a/tests/integration/engine/test_coordination_wiring.py +++ b/tests/integration/engine/test_coordination_wiring.py @@ -321,10 +321,7 @@ async def _mock_coordinate(context): # type: ignore[no-untyped-def] assert body["success"] is True data = body["data"] assert data["parent_task_id"] == task_id - resolved = [ - t.value for t in CoordinationTopology if t != CoordinationTopology.AUTO - ] - assert data["topology"] in resolved + assert data["topology"] == "sas" assert isinstance(data["total_duration_seconds"], float) assert isinstance(data["phases"], list) assert len(data["phases"]) >= 1 diff --git a/tests/unit/api/test_state.py b/tests/unit/api/test_state.py index 92f55b45f8..582cb8c319 100644 --- a/tests/unit/api/test_state.py +++ b/tests/unit/api/test_state.py @@ -1,5 +1,7 @@ """Tests for application state accessors.""" +from unittest.mock import AsyncMock + import pytest from synthorg.api.approval_store import ApprovalStore @@ -222,6 +224,20 @@ async def test_has_message_bus_true_when_set(self) -> None: assert state.has_message_bus is True +@pytest.mark.unit +class TestAppStateSettingsServiceFlag: + """Tests for has_settings_service property.""" + + def test_has_settings_service_false_when_none(self) -> None: + state = _make_state(settings_service=None) + assert state.has_settings_service is False + + def test_has_settings_service_true_when_set(self) -> None: + mock_svc = AsyncMock() + state = _make_state(settings_service=mock_svc) + assert state.has_settings_service is True + + @pytest.mark.unit class TestAppStateConfigResolver: """Tests for config_resolver property.""" @@ -232,11 +248,26 @@ def test_config_resolver_raises_when_settings_service_none(self) -> None: _ = state.config_resolver def test_config_resolver_returns_when_settings_service_set(self) -> None: - from unittest.mock import MagicMock - from synthorg.settings.resolver import ConfigResolver - mock_svc = MagicMock() + mock_svc = AsyncMock() state = _make_state(settings_service=mock_svc) resolver = state.config_resolver assert isinstance(resolver, ConfigResolver) + + def test_config_resolver_is_singleton(self) -> None: + """Successive property accesses return the same cached instance.""" + mock_svc = AsyncMock() + state = _make_state(settings_service=mock_svc) + first = state.config_resolver + second = state.config_resolver + assert first is second + + def test_has_config_resolver_false_when_none(self) -> None: + state = _make_state(settings_service=None) + assert state.has_config_resolver is False + + def test_has_config_resolver_true_when_set(self) -> None: + mock_svc = AsyncMock() + state = _make_state(settings_service=mock_svc) + assert state.has_config_resolver is True diff --git a/tests/unit/settings/test_resolver.py b/tests/unit/settings/test_resolver.py index 3fc480f5cf..27ab306845 100644 --- a/tests/unit/settings/test_resolver.py +++ b/tests/unit/settings/test_resolver.py @@ -8,6 +8,7 @@ from hypothesis import strategies as st from pydantic import BaseModel, ConfigDict +from synthorg.core.enums import AutonomyLevel from synthorg.settings.enums import SettingNamespace, SettingSource from synthorg.settings.errors import SettingNotFoundError from synthorg.settings.models import SettingValue @@ -124,6 +125,13 @@ async def test_returns_empty_string( result = await resolver.get_str("test", "key") assert result == "" + async def test_not_found_logs_and_propagates( + self, resolver: ConfigResolver, mock_settings: AsyncMock + ) -> None: + mock_settings.get.side_effect = SettingNotFoundError("nope") + with pytest.raises(SettingNotFoundError): + await resolver.get_str("bad", "key") + @pytest.mark.unit @pytest.mark.timeout(30) @@ -152,7 +160,7 @@ async def test_invalid_raises_value_error( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: mock_settings.get.return_value = _make_value("not_a_number") - with pytest.raises(ValueError, match="invalid literal"): + with pytest.raises(ValueError, match="invalid integer"): await resolver.get_int("test", "key") @@ -178,7 +186,7 @@ async def test_invalid_raises_value_error( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: mock_settings.get.return_value = _make_value("abc") - with pytest.raises(ValueError, match="could not convert"): + with pytest.raises(ValueError, match="invalid float"): await resolver.get_float("test", "key") @@ -214,7 +222,7 @@ async def test_invalid_raises_value_error( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: mock_settings.get.return_value = _make_value("maybe") - with pytest.raises(ValueError, match="Cannot parse"): + with pytest.raises(ValueError, match="not a recognized boolean"): await resolver.get_bool("test", "key") @@ -234,7 +242,7 @@ async def test_invalid_raises_value_error( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: mock_settings.get.return_value = _make_value("purple") - with pytest.raises(ValueError, match="purple"): + with pytest.raises(ValueError, match=r"invalid.*_Color"): await resolver.get_enum("test", "key", _Color) @@ -254,6 +262,25 @@ async def test_not_found_propagates( await resolver.get_str("bad", "key") +# ── Constructor Guard ───────────────────────────────────────────── + + +@pytest.mark.unit +@pytest.mark.timeout(30) +class TestConfigResolverInit: + """Tests for ConfigResolver constructor validation.""" + + def test_none_settings_service_raises_type_error( + self, + root_config: _FakeRootConfig, + ) -> None: + with pytest.raises(TypeError, match="settings_service must not be None"): + ConfigResolver( + settings_service=None, # type: ignore[arg-type] + config=root_config, # type: ignore[arg-type] + ) + + # ── Composed Read: Autonomy Level ───────────────────────────────── @@ -268,21 +295,20 @@ async def test_resolves_autonomy_level( mock_settings.get.return_value = _make_value( "supervised", namespace=SettingNamespace.COMPANY, key="autonomy_level" ) - from synthorg.core.enums import AutonomyLevel - result = await resolver.get_autonomy_level() assert result is AutonomyLevel.SUPERVISED mock_settings.get.assert_awaited_once_with("company", "autonomy_level") + @pytest.mark.parametrize("level", list(AutonomyLevel)) async def test_resolves_all_levels( - self, resolver: ConfigResolver, mock_settings: AsyncMock + self, + resolver: ConfigResolver, + mock_settings: AsyncMock, + level: AutonomyLevel, ) -> None: - from synthorg.core.enums import AutonomyLevel - - for level in AutonomyLevel: - mock_settings.get.return_value = _make_value(level.value) - result = await resolver.get_autonomy_level() - assert result is level + mock_settings.get.return_value = _make_value(level.value) + result = await resolver.get_autonomy_level() + assert result is level # ── Composed Read: Budget Config ────────────────────────────────── @@ -381,24 +407,20 @@ async def test_preserves_unregistered_fields( async def test_not_found_propagates( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: - """SettingNotFoundError from a budget key propagates via ExceptionGroup.""" + """SettingNotFoundError propagates directly (unwrapped from ExceptionGroup).""" mock_settings.get.side_effect = SettingNotFoundError("missing") - with pytest.raises(ExceptionGroup) as exc_info: + with pytest.raises(SettingNotFoundError): await resolver.get_budget_config() - assert any( - isinstance(e, SettingNotFoundError) for e in exc_info.value.exceptions - ) async def test_value_error_propagates( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: - """ValueError from a corrupted DB value propagates via ExceptionGroup.""" + """ValueError from a corrupted DB value propagates directly.""" mock_settings.get = _budget_get_side_effect( {("budget", "total_monthly"): "not-a-number"} ) - with pytest.raises(ExceptionGroup) as exc_info: + with pytest.raises(ValueError, match="invalid"): await resolver.get_budget_config() - assert any(isinstance(e, ValueError) for e in exc_info.value.exceptions) # ── Composed Read: Coordination Config ──────────────────────────── @@ -409,7 +431,7 @@ def _coordination_get_side_effect( ) -> AsyncMock: """Create a mock .get() for coordination settings.""" defaults = { - ("coordination", "max_wave_size"): "5", + ("coordination", "max_concurrency_per_wave"): "5", ("coordination", "fail_fast"): "false", ("coordination", "enable_workspace_isolation"): "true", ("coordination", "base_branch"): "main", @@ -484,24 +506,20 @@ async def test_request_overrides_beat_db_overrides( async def test_not_found_propagates( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: - """SettingNotFoundError from a coordination key propagates.""" + """SettingNotFoundError propagates directly (unwrapped from ExceptionGroup).""" mock_settings.get.side_effect = SettingNotFoundError("missing") - with pytest.raises(ExceptionGroup) as exc_info: + with pytest.raises(SettingNotFoundError): await resolver.get_coordination_config() - assert any( - isinstance(e, SettingNotFoundError) for e in exc_info.value.exceptions - ) async def test_value_error_propagates( self, resolver: ConfigResolver, mock_settings: AsyncMock ) -> None: - """ValueError from a corrupted coordination value propagates.""" + """ValueError from a corrupted coordination value propagates directly.""" mock_settings.get = _coordination_get_side_effect( - {("coordination", "max_wave_size"): "not-a-number"} + {("coordination", "max_concurrency_per_wave"): "not-a-number"} ) - with pytest.raises(ExceptionGroup) as exc_info: + with pytest.raises(ValueError, match="invalid"): await resolver.get_coordination_config() - assert any(isinstance(e, ValueError) for e in exc_info.value.exceptions) # ── _parse_bool Tests ───────────────────────────────────────────── @@ -530,7 +548,7 @@ def test_valid_values(self, raw: str, expected: bool) -> None: @pytest.mark.parametrize("raw", ["yes", "no", "maybe", "", "2", "tru"]) def test_invalid_values(self, raw: str) -> None: - with pytest.raises(ValueError, match="Cannot parse"): + with pytest.raises(ValueError, match="not a recognized boolean"): _parse_bool(raw) @@ -554,14 +572,14 @@ class TestResolverScalarProperties: """Hypothesis-based roundtrip tests for scalar accessors.""" @given(st.integers(min_value=-(2**31), max_value=2**31)) - @settings(max_examples=200) + @settings() async def test_int_roundtrip(self, n: int) -> None: resolver, mock = _make_resolver() mock.get = AsyncMock(return_value=_make_value(str(n))) assert await resolver.get_int("budget", "total_monthly") == n @given(st.floats(allow_nan=False, allow_infinity=False, allow_subnormal=False)) - @settings(max_examples=200) + @settings() async def test_float_roundtrip(self, x: float) -> None: resolver, mock = _make_resolver() mock.get = AsyncMock(return_value=_make_value(str(x))) @@ -569,7 +587,7 @@ async def test_float_roundtrip(self, x: float) -> None: assert result == pytest.approx(x, rel=1e-9, abs=1e-15) @given(st.booleans()) - @settings(max_examples=200) + @settings() async def test_bool_roundtrip(self, b: bool) -> None: resolver, mock = _make_resolver() mock.get = AsyncMock(return_value=_make_value(str(b))) From 206895e75a82d85b9ab85a5b97765708b571bb66 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 07:18:17 +0100 Subject: [PATCH 5/8] fix: revert _resolve_fallback default=None change (breaks optional settings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous change to raise SettingNotFoundError when default=None broke settings like providers/default_provider that legitimately have no built-in default. Revert to returning empty string as sentinel — consumers like ConfigResolver.get_int will raise ValueError on empty input, giving a clear error at the consumer layer. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/synthorg/settings/service.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/synthorg/settings/service.py b/src/synthorg/settings/service.py index 284d9ad368..f3213da7be 100644 --- a/src/synthorg/settings/service.py +++ b/src/synthorg/settings/service.py @@ -374,20 +374,16 @@ def _resolve_fallback( source=SettingSource.YAML, ) - if definition.default is None: - logger.warning( - SETTINGS_NOT_FOUND, - namespace=ns, - key=key, - reason="no_default_configured", - ) - msg = f"Setting {ns}/{key} has no configured value or default" - raise SettingNotFoundError(msg) + # default=None means "optional — no built-in default". Return + # empty string as a sentinel (callers like ConfigResolver.get_int + # will raise ValueError on empty, giving a clear error at the + # consumer layer rather than here). + default = definition.default if definition.default is not None else "" logger.debug(SETTINGS_VALUE_RESOLVED, namespace=ns, key=key, source="default") return SettingValue( namespace=ns, key=key, - value=definition.default, + value=default, source=SettingSource.DEFAULT, ) From 1f93576b1399fb36dd67861e6fb3106c334e4a46 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 07:35:24 +0100 Subject: [PATCH 6/8] fix: address CodeRabbit round-2 findings (logging consistency, guard label) - Add SettingNotFoundError catch + SETTINGS_NOT_FOUND log to get_int, get_float, get_bool, get_enum (consistent with get_str pattern) - Revert config_resolver guard label to "config_resolver" (callers see this property name, not the internal dependency) - Add comment explaining intentional direct dict mutation in cache (MappingProxyType copy-on-write would reintroduce TOCTOU race) Co-Authored-By: Claude Opus 4.6 (1M context) --- src/synthorg/api/state.py | 2 +- src/synthorg/settings/resolver.py | 40 +++++++++++++++++++++++++++---- src/synthorg/settings/service.py | 7 ++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/synthorg/api/state.py b/src/synthorg/api/state.py index d5bfe98fae..636ebef280 100644 --- a/src/synthorg/api/state.py +++ b/src/synthorg/api/state.py @@ -251,7 +251,7 @@ def has_config_resolver(self) -> bool: @property def config_resolver(self) -> ConfigResolver: """Return the cached config resolver or raise 503.""" - return self._require_service(self._config_resolver, "settings_service") + return self._require_service(self._config_resolver, "config_resolver") @property def has_auth_service(self) -> bool: diff --git a/src/synthorg/settings/resolver.py b/src/synthorg/settings/resolver.py index bb4220e70b..6c6b2ce6a7 100644 --- a/src/synthorg/settings/resolver.py +++ b/src/synthorg/settings/resolver.py @@ -103,7 +103,15 @@ async def get_int(self, namespace: str, key: str) -> int: SettingNotFoundError: If the key is not in the registry. ValueError: If the value cannot be parsed as an integer. """ - result = await self._settings.get(namespace, key) + try: + result = await self._settings.get(namespace, key) + except SettingNotFoundError: + logger.warning( + SETTINGS_NOT_FOUND, + namespace=namespace, + key=key, + ) + raise try: return int(result.value) except ValueError: @@ -131,7 +139,15 @@ async def get_float(self, namespace: str, key: str) -> float: SettingNotFoundError: If the key is not in the registry. ValueError: If the value cannot be parsed as a float. """ - result = await self._settings.get(namespace, key) + try: + result = await self._settings.get(namespace, key) + except SettingNotFoundError: + logger.warning( + SETTINGS_NOT_FOUND, + namespace=namespace, + key=key, + ) + raise try: return float(result.value) except ValueError: @@ -161,7 +177,15 @@ async def get_bool(self, namespace: str, key: str) -> bool: SettingNotFoundError: If the key is not in the registry. ValueError: If the value is not a recognized boolean string. """ - result = await self._settings.get(namespace, key) + try: + result = await self._settings.get(namespace, key) + except SettingNotFoundError: + logger.warning( + SETTINGS_NOT_FOUND, + namespace=namespace, + key=key, + ) + raise try: return _parse_bool(result.value) except ValueError: @@ -195,7 +219,15 @@ async def get_enum[E: StrEnum]( SettingNotFoundError: If the key is not in the registry. ValueError: If the value does not match any enum member. """ - result = await self._settings.get(namespace, key) + try: + result = await self._settings.get(namespace, key) + except SettingNotFoundError: + logger.warning( + SETTINGS_NOT_FOUND, + namespace=namespace, + key=key, + ) + raise try: return enum_cls(result.value) except ValueError: diff --git a/src/synthorg/settings/service.py b/src/synthorg/settings/service.py index f3213da7be..dae77ee1b1 100644 --- a/src/synthorg/settings/service.py +++ b/src/synthorg/settings/service.py @@ -252,6 +252,13 @@ async def get(self, namespace: str, key: str) -> SettingValue: ) # Cache only non-sensitive values to avoid holding # plaintext secrets in memory. + # + # Direct dict mutation is intentional: the previous + # copy-on-write pattern {**self._cache, k: v} had a + # TOCTOU race under concurrent TaskGroup reads (the + # spread sees a stale snapshot after an await). In + # asyncio's cooperative concurrency, dict item assignment + # is a single-opcode operation and safe without locking. if not definition.sensitive: self._cache[cache_key] = setting_value logger.debug( From 4ed0f50dea94927ef2c2c4dc35d19cad9986fde0 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 07:53:26 +0100 Subject: [PATCH 7/8] fix: address CodeRabbit round-3 findings (logging, function size) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Log SETTINGS_VALIDATION_FAILED before raising TypeError in ConfigResolver constructor (CLAUDE.md: all error paths must log) - Extract _build_budget_alerts() helper from get_budget_config to keep function body under 50-line limit - Log SETTINGS_VALIDATION_FAILED with exc_info before re-raising ValueError from BudgetAlertConfig ordering constraint Skipped: service.py default=None→"" change (already tried in earlier commit, broke providers/default_provider — reverted with comment) Co-Authored-By: Claude Opus 4.6 (1M context) --- src/synthorg/settings/resolver.py | 53 +++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/src/synthorg/settings/resolver.py b/src/synthorg/settings/resolver.py index 6c6b2ce6a7..1afa9e71ef 100644 --- a/src/synthorg/settings/resolver.py +++ b/src/synthorg/settings/resolver.py @@ -19,7 +19,7 @@ from synthorg.settings.errors import SettingNotFoundError if TYPE_CHECKING: - from synthorg.budget.config import BudgetConfig + from synthorg.budget.config import BudgetAlertConfig, BudgetConfig from synthorg.config.schema import RootConfig from synthorg.core.enums import AutonomyLevel from synthorg.engine.coordination.config import CoordinationConfig @@ -61,6 +61,7 @@ def __init__( # runtime defence for callers without type checking if settings_service is None: msg = "settings_service must not be None" # type: ignore[unreachable] + logger.error(SETTINGS_VALIDATION_FAILED, reason=msg) raise TypeError(msg) self._settings = settings_service self._config = config @@ -280,12 +281,6 @@ async def get_budget_config(self) -> BudgetConfig: the assembled alert thresholds violate the ordering constraint (``warn_at < critical_at < hard_stop_at``). """ - from pydantic import ValidationError # noqa: PLC0415 - - from synthorg.budget.config import ( # noqa: PLC0415 - BudgetAlertConfig, - ) - base = self._config.budget try: @@ -308,16 +303,7 @@ async def get_budget_config(self) -> BudgetConfig: except ExceptionGroup as eg: raise eg.exceptions[0] from eg - try: - alerts = BudgetAlertConfig( - warn_at=t_warn.result(), - critical_at=t_crit.result(), - hard_stop_at=t_stop.result(), - ) - except ValidationError as exc: - msg = f"Budget alert thresholds violate ordering constraint: {exc}" - raise ValueError(msg) from exc - + alerts = _build_budget_alerts(t_warn.result(), t_crit.result(), t_stop.result()) return base.model_copy( update={ "total_monthly": t_monthly.result(), @@ -396,6 +382,39 @@ async def get_coordination_config( ) +def _build_budget_alerts(warn: int, crit: int, stop: int) -> BudgetAlertConfig: + """Construct ``BudgetAlertConfig`` with ordering validation. + + Args: + warn: Warning threshold percent. + crit: Critical threshold percent. + stop: Hard-stop threshold percent. + + Returns: + A validated ``BudgetAlertConfig``. + + Raises: + ValueError: If the thresholds violate the ordering constraint + (``warn < crit < stop``). + """ + from pydantic import ValidationError # noqa: PLC0415 + + from synthorg.budget.config import BudgetAlertConfig # noqa: PLC0415 + + try: + return BudgetAlertConfig(warn_at=warn, critical_at=crit, hard_stop_at=stop) + except ValidationError as exc: + logger.warning( + SETTINGS_VALIDATION_FAILED, + namespace="budget", + key="_alerts", + reason="threshold_ordering", + exc_info=True, + ) + msg = f"Budget alert thresholds violate ordering constraint: {exc}" + raise ValueError(msg) from exc + + _BOOL_TRUE = frozenset({"true", "1"}) _BOOL_FALSE = frozenset({"false", "0"}) From 28528603ee6b9f5a9149187fe3c9f8eb2a0d6a7e Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 08:07:12 +0100 Subject: [PATCH 8/8] fix: log ExceptionGroup context before unwrapping in composed reads Both except ExceptionGroup handlers in get_budget_config and get_coordination_config now log SETTINGS_FETCH_FAILED at WARNING with namespace, error count, and exc_info before re-raising the first inner exception. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/synthorg/settings/resolver.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/synthorg/settings/resolver.py b/src/synthorg/settings/resolver.py index 1afa9e71ef..8e9743f130 100644 --- a/src/synthorg/settings/resolver.py +++ b/src/synthorg/settings/resolver.py @@ -13,6 +13,7 @@ from synthorg.observability import get_logger from synthorg.observability.events.settings import ( + SETTINGS_FETCH_FAILED, SETTINGS_NOT_FOUND, SETTINGS_VALIDATION_FAILED, ) @@ -301,6 +302,13 @@ async def get_budget_config(self) -> BudgetConfig: t_crit = tg.create_task(self.get_int("budget", "alert_critical_at")) t_stop = tg.create_task(self.get_int("budget", "alert_hard_stop_at")) except ExceptionGroup as eg: + logger.warning( + SETTINGS_FETCH_FAILED, + namespace="budget", + key="_composed", + error_count=len(eg.exceptions), + exc_info=True, + ) raise eg.exceptions[0] from eg alerts = _build_budget_alerts(t_warn.result(), t_crit.result(), t_stop.result()) @@ -368,6 +376,13 @@ async def get_coordination_config( ) t_branch = tg.create_task(self.get_str("coordination", "base_branch")) except ExceptionGroup as eg: + logger.warning( + SETTINGS_FETCH_FAILED, + namespace="coordination", + key="_composed", + error_count=len(eg.exceptions), + exc_info=True, + ) raise eg.exceptions[0] from eg return CoordinationConfig(