feat(settings): route structural data reads through SettingsService#525
feat(settings): route structural data reads through SettingsService#525
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughControllers now load structural config (agents, departments, provider configs) asynchronously via ConfigResolver/SettingsService using JSON-backed settings; config bridge serialization and typed resolver getters were added; controllers use asyncio.TaskGroup for parallel fetches; tests cover DB overrides and serialization. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant APIController as API Controller
participant ConfigResolver as ConfigResolver
participant SettingsService as SettingsService/DB
participant RootConfig as RootConfig (fallback)
Client->>APIController: HTTP GET (e.g. /agents, /company, /departments, /providers)
APIController->>ConfigResolver: await get_*() (may use TaskGroup)
ConfigResolver->>SettingsService: fetch JSON setting (namespace/key)
SettingsService-->>ConfigResolver: JSON value or NotFound
alt JSON present and valid
ConfigResolver->>ConfigResolver: parse JSON → Pydantic models
ConfigResolver-->>APIController: return typed models
else JSON missing/invalid
ConfigResolver->>RootConfig: read in-memory RootConfig structural value
RootConfig-->>ConfigResolver: return default typed models
ConfigResolver-->>APIController: return typed models (fallback)
end
APIController-->>Client: HTTP response (sensitive fields stripped, paginated if applicable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors how structural configuration data, such as agents, departments, and provider settings, is accessed and managed within the application. By routing these reads through the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request successfully refactors structural data reads (agents, departments, providers) to go through the new SettingsService, enabling runtime configuration overrides from the database. The changes include correcting serialization to produce valid JSON, adding new accessors to ConfigResolver with robust fallback mechanisms, and migrating several API controllers to use these new methods. A performance improvement was also made by parallelizing data fetching in one controller. My review focuses on opportunities to reduce code duplication in the new resolver methods and simplify serialization logic for improved maintainability.
| if isinstance(value, (tuple, list)): | ||
| if any(isinstance(item, BaseModel) for item in value): | ||
| return json.dumps( | ||
| [ | ||
| item.model_dump(mode="json") | ||
| if isinstance(item, BaseModel) | ||
| else item | ||
| for item in value | ||
| ] | ||
| ) | ||
| return json.dumps(list(value)) | ||
|
|
||
| if isinstance(value, dict): | ||
| if any(isinstance(v, BaseModel) for v in value.values()): | ||
| return json.dumps( | ||
| { | ||
| k: v.model_dump(mode="json") if isinstance(v, BaseModel) else v | ||
| for k, v in value.items() | ||
| } | ||
| ) | ||
| return json.dumps(value) |
There was a problem hiding this comment.
The any() checks to see if a collection contains BaseModel instances before serialization are unnecessary. The list/dict comprehensions with isinstance() checks can handle both cases (with and without models) correctly and more concisely. Removing the any() checks and the separate json.dumps() calls will simplify the code and improve maintainability.
if isinstance(value, (tuple, list)):
return json.dumps(
[
item.model_dump(mode="json") if isinstance(item, BaseModel) else item
for item in value
]
)
if isinstance(value, dict):
return json.dumps(
{
k: v.model_dump(mode="json") if isinstance(v, BaseModel) else v
for k, v in value.items()
}
)
src/synthorg/settings/resolver.py
Outdated
| async def get_agents(self) -> tuple[AgentConfig, ...]: | ||
| """Resolve agent configurations from settings. | ||
|
|
||
| Falls back to ``RootConfig.agents`` if the setting is | ||
| empty, contains invalid JSON, or fails schema validation. | ||
|
|
||
| Returns: | ||
| A tuple of ``AgentConfig`` objects. | ||
|
|
||
| Raises: | ||
| SettingNotFoundError: If the agents key is not | ||
| in the registry. | ||
| """ | ||
| from pydantic import ValidationError # noqa: PLC0415 | ||
|
|
||
| from synthorg.config.schema import AgentConfig # noqa: PLC0415 | ||
|
|
||
| try: | ||
| raw = await self.get_json("company", "agents") | ||
| except ValueError: | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="company", | ||
| key="agents", | ||
| reason="invalid_json_fallback", | ||
| ) | ||
| return self._config.agents | ||
| if not raw: | ||
| return self._config.agents | ||
| if not isinstance(raw, list): | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="company", | ||
| key="agents", | ||
| reason="expected_list_fallback", | ||
| ) | ||
| return self._config.agents | ||
| try: | ||
| return tuple(AgentConfig.model_validate(item) for item in raw) | ||
| except ValidationError: | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="company", | ||
| key="agents", | ||
| reason="invalid_schema_fallback", | ||
| exc_info=True, | ||
| ) | ||
| return self._config.agents | ||
|
|
||
| async def get_departments(self) -> tuple[Department, ...]: | ||
| """Resolve department configurations from settings. | ||
|
|
||
| Falls back to ``RootConfig.departments`` if the setting | ||
| is empty, contains invalid JSON, or fails schema validation. | ||
|
|
||
| Returns: | ||
| A tuple of ``Department`` objects. | ||
|
|
||
| Raises: | ||
| SettingNotFoundError: If the departments key is not | ||
| in the registry. | ||
| """ | ||
| from pydantic import ValidationError # noqa: PLC0415 | ||
|
|
||
| from synthorg.core.company import Department # noqa: PLC0415 | ||
|
|
||
| try: | ||
| raw = await self.get_json("company", "departments") | ||
| except ValueError: | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="company", | ||
| key="departments", | ||
| reason="invalid_json_fallback", | ||
| ) | ||
| return self._config.departments | ||
| if not raw: | ||
| return self._config.departments | ||
| if not isinstance(raw, list): | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="company", | ||
| key="departments", | ||
| reason="expected_list_fallback", | ||
| ) | ||
| return self._config.departments | ||
| try: | ||
| return tuple(Department.model_validate(item) for item in raw) | ||
| except ValidationError: | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="company", | ||
| key="departments", | ||
| reason="invalid_schema_fallback", | ||
| exc_info=True, | ||
| ) | ||
| return self._config.departments | ||
|
|
||
| async def get_provider_configs(self) -> dict[str, ProviderConfig]: | ||
| """Resolve provider configurations from settings. | ||
|
|
||
| Falls back to ``RootConfig.providers`` if the setting | ||
| is empty, contains invalid JSON, or fails schema validation. | ||
|
|
||
| Returns: | ||
| A dict of provider name to ``ProviderConfig``. | ||
|
|
||
| Raises: | ||
| SettingNotFoundError: If the configs key is not | ||
| in the registry. | ||
| """ | ||
| from pydantic import ValidationError # noqa: PLC0415 | ||
|
|
||
| from synthorg.config.schema import ProviderConfig # noqa: PLC0415 | ||
|
|
||
| try: | ||
| raw = await self.get_json("providers", "configs") | ||
| except ValueError: | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="providers", | ||
| key="configs", | ||
| reason="invalid_json_fallback", | ||
| ) | ||
| return dict(self._config.providers) | ||
| if not raw: | ||
| return dict(self._config.providers) | ||
| if not isinstance(raw, dict): | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="providers", | ||
| key="configs", | ||
| reason="expected_dict_fallback", | ||
| ) | ||
| return dict(self._config.providers) | ||
| try: | ||
| return { | ||
| name: ProviderConfig.model_validate(conf) for name, conf in raw.items() | ||
| } | ||
| except ValidationError: | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="providers", | ||
| key="configs", | ||
| reason="invalid_schema_fallback", | ||
| exc_info=True, | ||
| ) | ||
| return dict(self._config.providers) | ||
|
|
There was a problem hiding this comment.
The methods get_agents, get_departments, and get_provider_configs share a significant amount of boilerplate code for fetching a JSON setting, handling various failure modes (invalid JSON, wrong shape, schema validation error), and falling back to the configuration value. This repeated logic can be refactored into a single generic private helper method to reduce duplication and improve maintainability.
This helper could be parameterized with the setting details, the expected data shape (list or dict), the Pydantic model for validation, and the fallback value.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #525 +/- ##
==========================================
+ Coverage 93.18% 93.19% +0.01%
==========================================
Files 522 522
Lines 25157 25247 +90
Branches 2386 2396 +10
==========================================
+ Hits 23443 23530 +87
- Misses 1356 1360 +4
+ Partials 358 357 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/controllers/agents.py`:
- Around line 43-44: The module/class docstring for the agents controller still
says it serves agent configs from RootConfig but the implementation now reads
from ConfigResolver via app_state.config_resolver.get_agents(); update the
surrounding docstring (module or the controller class docstring that documents
the agents endpoint) to state that agent data is provided by ConfigResolver /
ConfigResolver.get_agents rather than RootConfig so the auto-generated API docs
reflect the new data source; mention the change near the handler that calls
get_agents and keep wording consistent with other docs produced by
mkdocstrings/Griffe.
In `@src/synthorg/settings/resolver.py`:
- Around line 329-331: The code currently does "if not raw:" which treats falsy
values like false or 0 as missing and skips shape validation/logging; change the
logic to first perform explicit type checks (use isinstance(raw, list) /
isinstance(raw, dict) where appropriate) and treat only None as the absent
fallback (e.g., "if raw is None: return self._config.agents"), otherwise emit
the expected_list_fallback / expected_dict_fallback diagnostic for non-matching
shapes; apply the same change to the other occurrences that use "if not raw:"
(the blocks around the other mentioned spots) so raw falsy-but-typed values are
validated and logged correctly.
In `@tests/unit/settings/test_config_bridge.py`:
- Around line 157-165: Change the falsy checks in the resolver so empty JSON
collections are treated as valid overrides: in src/synthorg/settings/resolver.py
replace each "if not raw:" with "if raw is None:" inside get_agents(),
get_departments(), and get_provider_configs() so only None triggers the
fallback; then update/add tests to assert that an explicit empty DB override
(e.g., "[]" or "{}") replaces non-empty YAML defaults (adjust tests like
test_empty_list_falls_back_to_config and test_empty_dict_falls_back_to_config to
expect override behavior and add regression cases confirming empty DB values do
not fall back).
In `@tests/unit/settings/test_resolver.py`:
- Around line 116-142: The file exceeds the repo size guideline because
structural resolver fixtures are embedded in test_resolver.py; extract the fake
Pydantic models (_FakeAgentConfig, _FakeDepartment, _FakeProviderConfig,
_FakeRootConfig and any related helper fixtures) into a separate test module
(e.g., tests/unit/settings/test_structural_fixtures.py or a conftest helper) and
import them into test_resolver.py; update imports/usages in test_resolver.py to
reference the new module and ensure the fixture names remain unchanged so
existing tests (that reference these classes/fixtures) continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a2923a6-3870-4e66-9e7b-3df3401b3dd9
📒 Files selected for processing (17)
src/synthorg/api/controllers/agents.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.pysrc/synthorg/api/controllers/departments.pysrc/synthorg/api/controllers/providers.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/resolver.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_providers.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver.pytests/unit/settings/test_resolver_structural.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling in Python 3.14, as enforced by ruff PEP 758
All public functions and methods require type hints; mypy strict mode enforced
Docstrings: Google style, required on all public classes and functions, enforced by ruff D rules
Create new objects instead of mutating existing ones; never mutate. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 with adopted conventions:@computed_fieldfor derived values instead of storing + validating redundant fields; useNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length: 88 characters (enforced by ruff)
Functions must be < 50 lines; files must be < 800 lines
Handle errors explicitly, never silently swallow them
Validate at system boundaries (user input, external APIs, config files)
Files:
src/synthorg/api/controllers/analytics.pysrc/synthorg/settings/config_bridge.pytests/unit/settings/test_resolver.pysrc/synthorg/settings/resolver.pytests/unit/api/controllers/test_providers.pysrc/synthorg/api/controllers/agents.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/api/controllers/company.pysrc/synthorg/settings/definitions/company.pytests/unit/settings/test_resolver_structural.pytests/unit/api/controllers/test_departments.pysrc/synthorg/api/controllers/departments.pytests/unit/settings/test_config_bridge.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_analytics.pysrc/synthorg/api/controllers/providers.pytests/unit/api/controllers/test_company.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST havefrom synthorg.observability import get_loggerthenlogger = get_logger(__name__); never useimport logging/logging.getLogger()/print()in application code
Logger variable name must always belogger(not_logger, notlog)
Event names must always use constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Always use structured logging:logger.info(EVENT, key=value)— neverlogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
DEBUG logging for object creation, internal flow, and entry/exit of key functions
All provider calls go throughBaseCompletionProviderwhich applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig; retryable errors (is_retryable=True) includeRateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError
Non-retryable errors raise immediately without retry;RetryExhaustedErrorsignals that all retries failed — engine layer catches this to trigger fallback chains
Rate limiter respectsRateLimitError.retry_afterfrom providers — automatically pauses future requests
Library reference: auto-generated from docstrings via mkdocstrings + Griffe (AST-based, no imports) indocs/api/
Files:
src/synthorg/api/controllers/analytics.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/resolver.pysrc/synthorg/api/controllers/agents.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/api/controllers/company.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/api/controllers/departments.pysrc/synthorg/api/controllers/providers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowfor test categorization
Useasyncio_mode = 'auto'in pytest — no manual@pytest.mark.asyncioneeded
Per-test timeout: 30 seconds
Prefer@pytest.mark.parametrizefor testing similar cases
Usetest-provider,test-small-001, etc. in tests instead of real vendor names
Use Python Hypothesis for property-based testing with@given+@settingsdecorators
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/settings/test_resolver.pytests/unit/api/controllers/test_providers.pytests/unit/settings/test_resolver_structural.pytests/unit/api/controllers/test_departments.pytests/unit/settings/test_config_bridge.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.py
🧠 Learnings (7)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig.
Applied to files:
src/synthorg/settings/resolver.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to src/synthorg/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in `ProviderConfig`; retryable errors (`is_retryable=True`) include `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`
Applied to files:
src/synthorg/settings/resolver.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/settings/resolver.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare create_task.
Applied to files:
src/synthorg/api/controllers/company.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to **/*.py : Prefer `asyncio.TaskGroup` for fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare `create_task`.
Applied to files:
src/synthorg/api/controllers/company.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : All provider calls go through BaseCompletionProvider which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code — it's handled by the base class.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to src/synthorg/**/*.py : All provider calls go through `BaseCompletionProvider` which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
Applied to files:
src/synthorg/api/controllers/providers.py
🧬 Code graph analysis (12)
src/synthorg/api/controllers/analytics.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_agents(302-349)
src/synthorg/api/controllers/agents.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_agents(302-349)
src/synthorg/settings/definitions/providers.py (3)
src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingType(24-36)
src/synthorg/api/controllers/company.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (3)
get_str(73-95)get_agents(302-349)get_departments(351-398)
src/synthorg/settings/definitions/company.py (3)
src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingType(24-36)
tests/unit/api/controllers/test_departments.py (3)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)
src/synthorg/api/controllers/departments.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_departments(351-398)
tests/unit/settings/test_config_bridge.py (1)
src/synthorg/settings/config_bridge.py (2)
_serialize_value(17-55)extract_from_config(58-91)
tests/unit/api/controllers/test_agents.py (5)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)tests/unit/settings/test_service.py (1)
service(106-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)tests/unit/api/conftest.py (2)
fake_persistence(154-157)fake_message_bus(161-164)
tests/unit/api/controllers/test_analytics.py (5)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)tests/unit/api/fakes.py (1)
FakePersistenceBackend(408-511)tests/unit/api/conftest.py (6)
make_auth_headers(111-137)fake_persistence(154-157)fake_message_bus(161-164)_make_test_auth_service(67-69)_seed_test_users(227-258)auth_service(149-150)
src/synthorg/api/controllers/providers.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_provider_configs(400-449)
tests/unit/api/controllers/test_company.py (7)
src/synthorg/config/schema.py (1)
RootConfig(387-651)tests/unit/persistence/test_protocol.py (8)
settings(358-359)get(51-52)get(149-150)get(184-185)get(204-205)get(237-238)get(254-255)get(265-266)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)tests/unit/engine/task_engine_helpers.py (2)
FakeMessageBus(64-82)get(29-31)tests/unit/api/fakes.py (1)
FakePersistenceBackend(408-511)tests/unit/api/conftest.py (8)
make_auth_headers(111-137)test_client(193-224)fake_persistence(154-157)fake_message_bus(161-164)_make_test_auth_service(67-69)_seed_test_users(227-258)auth_service(149-150)cost_tracker(168-169)
🔇 Additional comments (17)
src/synthorg/settings/config_bridge.py (1)
17-55: This is the right place to normalize structural values to JSON.Using
model_dump(mode="json")plusjson.dumps()here fixes the old Pythonrepr()leakage at the YAML/settings boundary without changing scalar behavior.src/synthorg/settings/definitions/company.py (1)
53-75: These definitions line up cleanly with the new resolver accessors.Registering
company/agentsandcompany/departmentsasSettingType.JSONkeeps structural data on the normal settings path instead of introducing special-case reads.src/synthorg/api/controllers/company.py (1)
43-46: Good use ofTaskGroupfor the fan-out here.
company_name, agents, and departments are independent reads, so structured concurrency avoids serializing three settings lookups.Based on learnings, "Prefer
asyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task."src/synthorg/api/controllers/analytics.py (1)
69-69: LGTM!The migration to async resolver-based agent retrieval is correct. The await evaluates before passing to the constructor, and the change properly enables DB-backed overrides for agent count.
tests/unit/api/controllers/test_analytics.py (1)
45-88: LGTM!The DB override test is well-structured and properly validates that the analytics endpoint reads agent count from the resolver (DB-backed settings). Test setup follows the established pattern across other controller tests in this PR.
tests/unit/api/controllers/test_providers.py (1)
57-100: LGTM!The DB override test correctly validates that:
- DB-stored provider configs take precedence over YAML defaults
- The
api_keyfield is stripped from responses (security requirement)Good coverage of the provider override flow.
src/synthorg/settings/definitions/providers.py (1)
53-63: LGTM!The new
configssetting definition is correctly structured:
SettingType.JSONappropriately handles the complex provider dict structureyaml_path="providers"correctly maps toRootConfig.providersfor fallback resolutiondefault=Noneensures fallback to YAML/code defaults when no DB override existstests/unit/api/controllers/test_departments.py (1)
35-77: LGTM!The DB override test properly validates that the departments endpoint reads from the resolver and respects DB-stored department configurations over YAML defaults.
tests/unit/api/controllers/test_company.py (1)
46-125: LGTM!Both DB override tests are well-structured:
test_db_company_departments_override: validates/api/v1/company/departmentsreturns DB-stored departmentstest_db_company_overview_includes_db_agents: validates/api/v1/companyoverview includes DB-stored agentsGood coverage of both company endpoints.
tests/unit/api/controllers/test_agents.py (2)
51-62: LGTM!The
SettingsServicewiring addition aligns with the new app factory signature and ensures consistent test setup.
77-124: LGTM!The DB override test correctly demonstrates that:
- YAML-defined agent (
yaml-agent) is superseded by DB agents- The endpoint returns only the 2 DB-stored agents (
db-agent-1,db-agent-2)- The test validates complete replacement semantics of the override
tests/unit/settings/test_resolver_structural.py (5)
1-78: LGTM!Well-organized test infrastructure:
- Docstring clearly explains the file extraction rationale (800-line limit)
- Fake models are minimal frozen Pydantic models appropriate for testing
- Fixtures are cleanly separated and reusable
84-123: LGTM!Thorough coverage of
get_json()accessor:
- Valid JSON array and object parsing
- Invalid JSON raises
ValueErrorSettingNotFoundErrorpropagation- Empty array handling
128-244: LGTM!Excellent coverage of
get_agents()fallback behaviors:
- JSON roundtrip to
AgentConfig- Empty list → fallback to
config.agents- Invalid JSON → fallback
- Schema validation failure → fallback
- Wrong JSON shape (dict instead of list) → fallback
SettingNotFoundErrorpropagation (no silent swallowing)
249-341: LGTM!
get_departments()tests mirror the agents pattern with appropriate coverage of all fallback scenarios.
346-460: LGTM!
get_provider_configs()tests correctly handle the dict-shaped response (vs list for agents/departments):
- JSON roundtrip to
ProviderConfigdict- Empty dict → fallback
- Invalid JSON → fallback
- Schema validation failure → fallback
- Wrong JSON shape (list instead of dict) → fallback
Comprehensive test coverage for all structural data accessors.
src/synthorg/settings/resolver.py (1)
266-301:get_json()implementation is solid and consistent.Parsing, not-found handling, and invalid JSON logging/exception mapping are all clean and aligned with resolver behavior.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/operations.md`:
- Line 1044: The Settings paragraph lacks terminal punctuation; edit the
paragraph describing Settings (references: ConfigResolver, get_agents,
get_departments, get_provider_configs, RootConfig, asyncio.TaskGroup) and add a
final period at the end of the sentence so it ends with proper punctuation.
In `@src/synthorg/api/controllers/analytics.py`:
- Around line 60-64: The TaskGroup block in analytics.py can raise an
ExceptionGroup which gets serialized by handle_unexpected; wrap the async with
asyncio.TaskGroup() block in a try/except catching ExceptionGroup and re-raise
its first exception (eg.exceptions[0]) to normalize failures as done in
company.py; modify the block around asyncio.TaskGroup() where t_tasks, t_cost,
t_agents are created so any ExceptionGroup is unwrapped before propagating to
handle_unexpected.
In `@src/synthorg/api/controllers/company.py`:
- Around line 48-49: In the except ExceptionGroup as eg block in company.py, add
a WARNING/ERROR log call before re-raising so grouped failures are recorded;
specifically, log a descriptive message including the ExceptionGroup (eg) and
its contained exceptions (eg.exceptions) and any relevant request/company
context available in the surrounding function (e.g., IDs or payload) using the
module logger (or process_logger) and then re-raise the first exception as
currently done (raise eg.exceptions[0] from eg).
In `@tests/unit/settings/conftest.py`:
- Around line 26-41: Add concise Google-style docstrings to the three test
fixture classes (FakeAgentConfig, FakeDepartment, FakeProviderConfig) to
describe their purpose and fields for clarity; update each class definition to
include a one- or two-line docstring above the class attributes that states it
is a frozen Pydantic model used as a test fixture and briefly lists the key
attributes (e.g., name, role, department for FakeAgentConfig; name, head for
FakeDepartment; driver for FakeProviderConfig).
In `@tests/unit/settings/test_config_bridge.py`:
- Around line 114-125: Replace the four repetitive tests
(test_scalar_string_unchanged, test_scalar_int_unchanged,
test_scalar_float_unchanged, test_scalar_bool_lowercase_json) with a single
parametrized test that calls _serialize_value for each input and compares to the
expected string; use pytest.mark.parametrize with cases ("hello", "hello"), (42,
"42"), (3.14, "3.14"), (True, "true"), (False, "false") so booleans assert
lowercase JSON strings and the intent is preserved while removing duplication.
In `@tests/unit/settings/test_resolver_structural.py`:
- Around line 17-24: Consolidate the duplicate import blocks by merging the two
"from tests.unit.settings.conftest import" statements into a single import that
includes FakeAgentConfig, FakeDepartment, FakeProviderConfig and
make_setting_value (alias _make_value) so you have one import line referencing
all four symbols; update the import to use the existing alias _make_value for
make_setting_value to match current usage in this test file.
In `@tests/unit/settings/test_resolver.py`:
- Around line 19-26: Consolidate the two duplicate import blocks by merging them
into a single import statement that imports FakeAgentConfig, FakeDepartment,
FakeProviderConfig and make_setting_value as _make_value from
tests.unit.settings.conftest; update the import line that currently references
these symbols separately so only one from ... import (...) statement remains,
keeping the same alias _make_value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95260fa3-3bfc-4d39-b01e-f9b17f8d8032
📒 Files selected for processing (15)
CLAUDE.mddocs/design/operations.mdsrc/synthorg/api/controllers/agents.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/resolver.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_providers.pytests/unit/settings/conftest.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver.pytests/unit/settings/test_resolver_structural.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling in Python 3.14, as enforced by ruff PEP 758
All public functions and methods require type hints; mypy strict mode enforced
Docstrings: Google style, required on all public classes and functions, enforced by ruff D rules
Create new objects instead of mutating existing ones; never mutate. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 with adopted conventions:@computed_fieldfor derived values instead of storing + validating redundant fields; useNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length: 88 characters (enforced by ruff)
Functions must be < 50 lines; files must be < 800 lines
Handle errors explicitly, never silently swallow them
Validate at system boundaries (user input, external APIs, config files)
Files:
src/synthorg/api/controllers/agents.pytests/unit/settings/test_resolver_structural.pysrc/synthorg/api/controllers/analytics.pytests/unit/settings/conftest.pytests/unit/api/controllers/test_company.pysrc/synthorg/api/controllers/company.pytests/unit/settings/test_config_bridge.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/config_bridge.pytests/unit/api/controllers/test_analytics.pysrc/synthorg/settings/resolver.pytests/unit/settings/test_resolver.pytests/unit/api/controllers/test_providers.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST havefrom synthorg.observability import get_loggerthenlogger = get_logger(__name__); never useimport logging/logging.getLogger()/print()in application code
Logger variable name must always belogger(not_logger, notlog)
Event names must always use constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Always use structured logging:logger.info(EVENT, key=value)— neverlogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
DEBUG logging for object creation, internal flow, and entry/exit of key functions
All provider calls go throughBaseCompletionProviderwhich applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig; retryable errors (is_retryable=True) includeRateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError
Non-retryable errors raise immediately without retry;RetryExhaustedErrorsignals that all retries failed — engine layer catches this to trigger fallback chains
Rate limiter respectsRateLimitError.retry_afterfrom providers — automatically pauses future requests
Library reference: auto-generated from docstrings via mkdocstrings + Griffe (AST-based, no imports) indocs/api/
Files:
src/synthorg/api/controllers/agents.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/resolver.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowfor test categorization
Useasyncio_mode = 'auto'in pytest — no manual@pytest.mark.asyncioneeded
Per-test timeout: 30 seconds
Prefer@pytest.mark.parametrizefor testing similar cases
Usetest-provider,test-small-001, etc. in tests instead of real vendor names
Use Python Hypothesis for property-based testing with@given+@settingsdecorators
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/settings/test_resolver_structural.pytests/unit/settings/conftest.pytests/unit/api/controllers/test_company.pytests/unit/settings/test_config_bridge.pytests/unit/api/controllers/test_analytics.pytests/unit/settings/test_resolver.pytests/unit/api/controllers/test_providers.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Docs: Markdown in
docs/, built with Zensical; Design spec indocs/design/(7 pages: index, agents, organization, communication, engine, memory, operations); Architecture indocs/architecture/(overview, tech-stack, decision log); Roadmap indocs/roadmap/; Security indocs/security.md; Licensing indocs/licensing.md; Reference indocs/reference/
Files:
docs/design/operations.md
🧠 Learnings (7)
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to docs/**/*.md : Docs: Markdown in `docs/`, built with Zensical; Design spec in `docs/design/` (7 pages: index, agents, organization, communication, engine, memory, operations); Architecture in `docs/architecture/` (overview, tech-stack, decision log); Roadmap in `docs/roadmap/`; Security in `docs/security.md`; Licensing in `docs/licensing.md`; Reference in `docs/reference/`
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare create_task.
Applied to files:
src/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to **/*.py : Prefer `asyncio.TaskGroup` for fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare `create_task`.
Applied to files:
src/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig.
Applied to files:
src/synthorg/settings/definitions/providers.pytests/unit/settings/test_resolver.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to src/synthorg/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in `ProviderConfig`; retryable errors (`is_retryable=True`) include `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`
Applied to files:
tests/unit/settings/test_resolver.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to tests/**/*.py : Use `test-provider`, `test-small-001`, etc. in tests instead of real vendor names
Applied to files:
tests/unit/api/controllers/test_providers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Tests must use test-provider, test-small-001, etc. for vendor-agnostic test data.
Applied to files:
tests/unit/api/controllers/test_providers.py
🧬 Code graph analysis (10)
src/synthorg/api/controllers/agents.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_agents(313-365)
tests/unit/settings/test_resolver_structural.py (3)
src/synthorg/settings/enums.py (1)
SettingNamespace(6-21)src/synthorg/settings/errors.py (1)
SettingNotFoundError(8-9)tests/unit/settings/conftest.py (4)
FakeAgentConfig(26-30)FakeDepartment(33-36)FakeProviderConfig(39-41)make_setting_value(9-20)
src/synthorg/api/controllers/analytics.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_agents(313-365)
tests/unit/settings/conftest.py (2)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingSource(50-61)src/synthorg/settings/models.py (1)
SettingValue(212-232)
src/synthorg/api/controllers/company.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (3)
get_str(73-95)get_agents(313-365)get_departments(367-419)
tests/unit/settings/test_config_bridge.py (1)
src/synthorg/settings/config_bridge.py (2)
_serialize_value(17-56)extract_from_config(59-92)
src/synthorg/settings/definitions/providers.py (3)
src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingType(24-36)
src/synthorg/settings/resolver.py (3)
src/synthorg/settings/errors.py (2)
SettingNotFoundError(8-9)SettingsEncryptionError(16-17)src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/service.py (1)
get(189-273)
tests/unit/settings/test_resolver.py (4)
src/synthorg/settings/enums.py (1)
SettingNamespace(6-21)src/synthorg/settings/errors.py (1)
SettingNotFoundError(8-9)src/synthorg/settings/models.py (1)
SettingValue(212-232)tests/unit/settings/conftest.py (4)
FakeAgentConfig(26-30)FakeDepartment(33-36)FakeProviderConfig(39-41)make_setting_value(9-20)
tests/unit/api/controllers/test_providers.py (5)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)tests/unit/api/conftest.py (4)
make_auth_headers(111-137)_make_test_auth_service(67-69)_seed_test_users(227-258)auth_service(149-150)src/synthorg/settings/encryption.py (1)
SettingsEncryptor(21-125)
🪛 LanguageTool
docs/design/operations.md
[grammar] ~1044-~1044: Please add a punctuation mark at the end of paragraph.
Context: ...ack to RootConfig defaults on invalid data ### Human Roles | Role | Access | Des...
(PUNCTUATION_PARAGRAPH_END)
🔇 Additional comments (23)
CLAUDE.md (1)
131-131: Settings architecture summary is clear and aligned.This update accurately reflects the JSON setting type, resolver accessors, and fallback behavior.
src/synthorg/settings/definitions/providers.py (1)
53-64: Good addition for provider runtime overrides.Registering
providers/configsas JSON withsensitive=Trueandyaml_path="providers"is consistent with secure settings resolution and fallback.src/synthorg/settings/config_bridge.py (1)
17-57: Serialization bridge update looks correct.Switching from
str()to JSON serialization for models/collections while keeping scalar behavior stable addresses the structural-settings path correctly.Also applies to: 63-69, 92-92
src/synthorg/api/controllers/agents.py (1)
19-19: Resolver migration is implemented cleanly.The docstring and both endpoints now consistently source agents via
ConfigResolver, which aligns behavior with runtime settings overrides.Also applies to: 43-44, 66-67
src/synthorg/api/controllers/company.py (1)
71-72: Department read path migration is solid.Using
config_resolver.get_departments()here correctly enables runtime settings overrides.tests/unit/settings/test_config_bridge.py (1)
68-184: Great coverage expansion for structural serialization paths.The new tests exercise model/list/dict/mixed-shape serialization and extraction behavior well.
tests/unit/settings/test_resolver.py (1)
107-114: LGTM!The
_FakeRootConfigexpansion withagents,departments, andprovidersfields properly mirrors theRootConfigstructure and reuses the shared fake models fromconftest.py, addressing the previous review comment about file size.tests/unit/api/controllers/test_analytics.py (1)
45-91: LGTM!The new
TestAnalyticsControllerDbOverridetest class correctly validates the DB-backed agent configuration flow:
- Seeds 3 agents via
settings_service.setwith valid JSON- Creates the app with the custom
SettingsService- Asserts
total_agents == 3in the analytics overviewThe test structure aligns with similar DB override tests in other controller test files.
tests/unit/settings/test_resolver_structural.py (4)
58-97: LGTM!The
TestGetJsonclass provides solid coverage for the generic JSON accessor including valid array/object parsing, invalid JSON error handling,SettingNotFoundErrorpropagation, and empty array handling.
102-214: LGTM!Comprehensive test coverage for
get_agents():
- JSON roundtrip with real
AgentConfigvalidation- Empty list as valid override (returns empty tuple, not fallback)
- Fallback to config on invalid JSON, invalid schema, and wrong JSON shape
SettingNotFoundErrorpropagation
220-329: LGTM!
TestGetDepartmentsmirrors the agent tests with appropriate coverage for department-specific behavior, including realDepartmentmodel validation.
335-447: LGTM!
TestGetProviderConfigscorrectly tests the dict-based provider configuration including empty dict as valid override, and list-instead-of-dict shape fallback.tests/unit/api/controllers/test_providers.py (2)
54-103: LGTM!The
TestProviderControllerDbOverridetest correctly:
- Creates an encryptor for the sensitive
providers/configssetting- Seeds DB-backed provider configurations via
settings_service.set- Validates that DB providers override YAML config
- Verifies
api_keyis stripped from responses (security check)The encryptor setup aligns with the PR commit notes marking
providers/configsas sensitive with Fernet encryption.
41-51: LGTM!Removing the unused
root_configfixture parameter is a good cleanup — the test creates its ownProviderConfiginline and doesn't need the fixture.tests/unit/settings/conftest.py (1)
9-20: LGTM!The
make_setting_valuehelper is well-designed with sensible defaults, providing a clean factory for creatingSettingValueinstances in tests.src/synthorg/settings/resolver.py (4)
266-311: LGTM!The
get_json()method correctly:
- Handles
SettingNotFoundErrorwith warning logging before re-raising- Handles
SettingsEncryptionErrorfor sensitive settings (likeproviders/configs)- Wraps
json.JSONDecodeErrorwith a descriptiveValueError, preserving the cause chainThe docstring properly documents the
SettingsEncryptionErrorraise condition.
313-365: LGTM!The
get_agents()implementation correctly handles all fallback scenarios:
- Invalid JSON → falls back to
config.agentsNonevalue → falls back toconfig.agents- Wrong shape (not list) → logs warning with
value_typeand falls back- Invalid schema → logs warning and falls back
- Empty list
[]→ valid override returning empty tupleThe
if raw is None:check addresses the past review comment about falsy-shape handling.
367-419: LGTM!
get_departments()follows the same robust pattern asget_agents()with appropriate fallback handling and logging.
421-475: LGTM!
get_provider_configs()correctly:
- Returns a defensive
dict()copy to prevent mutation of frozen config state- Validates shape as
dict(not list)- Uses
model_validatefor each provider entryThe fail-fast behavior on any invalid provider entry is acceptable since partial provider configurations could lead to inconsistent runtime state.
tests/unit/api/controllers/test_company.py (4)
38-38: LGTM!Changing from
isinstance(body["data"], list)tobody["data"] == []is a more precise assertion that verifies both the type and the expected empty state.
53-89: LGTM!The
test_db_company_departments_overridetest correctly validates that DB-stored departments override the empty YAML default, with proper app wiring and authentication.
91-128: LGTM!The
test_taskgroup_error_returns_clean_error_responsetest validates thatSettingNotFoundErrorpropagates cleanly to API error responses. The flexible assertionbody.get("success") is False or "detail" in bodyhandles different error response formats appropriately.
130-166: LGTM!The
test_db_company_overview_includes_db_agentstest correctly validates that DB-stored agents appear in the company overview endpoint response.
d52fab8 to
5b1e6ac
Compare
Fix config_bridge serialization to produce valid JSON for Pydantic models, tuples/lists, and dicts instead of Python repr strings. Register JSON setting definitions for agents, departments, and provider configs. Add get_json, get_agents, get_departments, and get_provider_configs accessors to ConfigResolver. Migrate 5 API controllers (10 endpoints) from direct RootConfig reads to resolver calls, enabling runtime DB overrides via the settings resolution chain (DB > env > YAML > code defaults).
Pre-reviewed by 12 agents, 11 findings addressed: - Catch ValidationError from model_validate() in get_agents, get_departments, get_provider_configs — fall back to YAML config instead of producing unhandled 500 errors - Add isinstance guards for JSON shape (list vs dict) before iterating or calling .items() - Use any() instead of value[0] check in _serialize_value for consistent heterogeneous collection handling - Return dict copy from provider fallback to prevent mutation of frozen config state - Parallelize 3 sequential awaits in CompanyController.get_company using asyncio.TaskGroup - Extract structural data tests to test_resolver_structural.py (keep test_resolver.py under 800 lines) - Add @pytest.mark.timeout(30) to all controller test classes - Add DB-override test for GET /company endpoint - Add ValidationError and wrong-shape fallback tests - Strengthen weak assertion in test_company.py
…ternal reviewers
Security: mark providers/configs as sensitive (Fernet encryption at rest),
catch SettingsEncryptionError in get_json, preserve JSONDecodeError chain.
Correctness: change `if not raw:` to `if raw is None:` so empty [] and {}
are valid overrides (not silent fallbacks), add ExceptionGroup unwrapping
in company controller, parallelize analytics controller with TaskGroup.
Code quality: simplify _serialize_value (remove redundant any() checks),
fix bool serialization to lowercase JSON "true"/"false", extract shared
test helpers to conftest.py (test_resolver.py back under 800 lines).
Tests: add missing departments wrong-shape fallback test, mixed list/dict
tests for _serialize_value, company TaskGroup error propagation test.
Docs: update CLAUDE.md and operations.md to document JSON setting type
and structural data accessors.
- Add terminal period to operations.md Settings paragraph - Wrap analytics TaskGroup in ExceptionGroup handler (matches company.py) - Add warning log in company ExceptionGroup handler with event constant - Add Google-style docstrings to test fixture classes in conftest.py - Parametrize scalar _serialize_value tests (DRY) - Consolidate split conftest imports via module-level alias
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/controllers/analytics.py`:
- Around line 60-66: The except ExceptionGroup block currently re-raises the
first exception without logging; update it to log a WARNING or ERROR with
contextual info before re-raising: inside the except ExceptionGroup as eg
handler, call the module/controller logger (e.g., logger.error or
logger.warning) and include a message that identifies the failing concurrent
reads (app_state.persistence.tasks.list_tasks,
app_state.cost_tracker.get_total_cost, app_state.config_resolver.get_agents),
the ExceptionGroup object (eg) and the specific first exception
(eg.exceptions[0]) for stack/context, then raise eg.exceptions[0] from eg as
before; ensure you reference asyncio.TaskGroup, t_tasks/t_cost/t_agents in the
log message so the controller-level context is preserved.
In `@src/synthorg/settings/config_bridge.py`:
- Around line 37-43: The current serialization branch for iterable values uses
json.dumps on a list comprehension that only calls item.model_dump() at the top
level, so nested BaseModel instances inside dicts/lists won't be converted;
update the serialization to either recursively walk the structure and replace
any BaseModel instances by their model_dump(mode="json") (handling dicts,
lists/tuples, and BaseModel) or simply pass Pydantic's encoder to json.dumps
(e.g., json.dumps(value, default=pydantic_encoder)) so nested BaseModel objects
are serialized correctly; refer to the existing symbols value, BaseModel,
model_dump, and the json.dumps call in this block when implementing the fix.
In `@src/synthorg/settings/resolver.py`:
- Around line 313-365: get_agents, get_departments, and get_provider_configs
duplicate the same resolve→type-check→pydantic-validate→fallback logic and
exceed the 50-line function rule; refactor by extracting the common flow into a
shared helper (e.g., a private async method like
_resolve_structured_setting(namespace, key, expected_type, model_cls,
fallback_value)) that: calls get_json, handles ValueError and None, checks
isinstance against expected_type, runs pydantic validation via
model_cls.model_validate (catching ValidationError), logs the appropriate
SETTINGS_FETCH_FAILED reasons, and returns either the validated tuple or the
provided fallback (self._config.agents / departments / provider_configs); then
have get_agents, get_departments, and get_provider_configs call this helper with
AgentConfig, DepartmentConfig, ProviderConfig respectively to remove duplication
and shrink each public method under the length limit.
In `@tests/unit/api/controllers/test_company.py`:
- Around line 83-89: The test is missing explicit HTTP status assertions: after
calling client.get("/api/v1/company/departments") and before parsing
resp.json(), assert resp.status_code == 200 (and similarly for the other
happy-path test that uses TestClient,
client.headers.update(make_auth_headers("ceo")), resp and body) to ensure
failures surface as HTTP errors rather than JSON shape mismatches; update both
tests that use resp and body to include this status check before inspecting
body["success"] and body["data"].
- Around line 126-129: The current assertion is too permissive; change it to
assert a deterministic error shape by requiring both that body.get("success") is
False and that a "detail" key exists (i.e., replace `body.get("success") is
False or "detail" in body` with `assert body.get("success") is False and
"detail" in body`) so the test explicitly verifies the expected error payload
for resp/body in tests/unit/api/controllers/test_company.py.
In `@tests/unit/api/controllers/test_providers.py`:
- Around line 99-103: Add an explicit HTTP status assertion before parsing the
response in the test: after calling client.get("/api/v1/providers") and storing
it in resp, assert resp.status_code == 200 (or the expected status) before
calling resp.json() and then proceed to check body["data"] and the api_key
absence; update the test around the resp variable to ensure status is validated
prior to payload assertions.
In `@tests/unit/settings/test_config_bridge.py`:
- Around line 73-144: The tests reveal _serialize_value currently falls back to
str() for unsupported top-level types (e.g., set, bytes) which violates its
TypeError contract; update the _serialize_value implementation (and its
docstring) so that after handling allowed top-level kinds (scalars:
str/int/float/bool, pydantic/model instances like _InnerConfig/_ItemModel,
lists/tuples, dicts) it raises a TypeError for any other top-level value instead
of stringifying it; locate and modify the _serialize_value function to add this
explicit rejection and adjust the docstring to document the exact accepted
types.
In `@tests/unit/settings/test_resolver_structural.py`:
- Around line 383-447: Add a regression test that verifies
ConfigResolver.get_provider_configs returns a defensive copy (not the original
self._config.providers) so callers cannot mutate shared state; specifically,
create a resolver with config having a provider dict, call
get_provider_configs() to get result, mutate result (e.g., add/remove keys or
modify nested dict) and then assert that resolver.config.providers (or
resolver._config.providers) remains unchanged; place this alongside the existing
fallback tests and reference ConfigResolver.get_provider_configs and the config
providers attribute to locate the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79c8b09d-1f93-4272-b84e-363572b14cac
📒 Files selected for processing (20)
CLAUDE.mddocs/design/operations.mdsrc/synthorg/api/controllers/agents.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.pysrc/synthorg/api/controllers/departments.pysrc/synthorg/api/controllers/providers.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/resolver.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_providers.pytests/unit/settings/conftest.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver.pytests/unit/settings/test_resolver_structural.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations.
Use PEP 758 except syntax:except A, B:(no parentheses) — ruff enforces this on Python 3.14.
Type hints required on all public functions. Use mypy strict mode.
Google-style docstrings required on all public classes and functions. Enforced by ruff D rules.
Line length: 88 characters. Enforced by ruff.
Functions must be less than 50 lines. Files must be less than 800 lines.
Files:
tests/unit/api/controllers/test_analytics.pysrc/synthorg/api/controllers/providers.pysrc/synthorg/settings/definitions/company.pytests/unit/settings/conftest.pysrc/synthorg/settings/config_bridge.pytests/unit/api/controllers/test_providers.pytests/unit/settings/test_resolver_structural.pysrc/synthorg/api/controllers/agents.pysrc/synthorg/settings/resolver.pysrc/synthorg/settings/definitions/providers.pytests/unit/api/controllers/test_company.pysrc/synthorg/api/controllers/departments.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_agents.pysrc/synthorg/api/controllers/analytics.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver.pysrc/synthorg/api/controllers/company.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow. Coverage: 80% minimum. Async:asyncio_mode = 'auto'— no manual@pytest.mark.asyncioneeded. Timeout: 30 seconds per test. Parallelism:pytest-xdistvia-n auto— ALWAYS include-n autowhen running pytest, never run tests sequentially.
Prefer@pytest.mark.parametrizefor testing similar cases.
Property-based testing: use Hypothesis (@given+@settings). Hypothesis profiles:ci(200 examples, default) anddev(1000 examples), controlled viaHYPOTHESIS_PROFILEenv var. Run dev profile:HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties.
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins.
Files:
tests/unit/api/controllers/test_analytics.pytests/unit/settings/conftest.pytests/unit/api/controllers/test_providers.pytests/unit/settings/test_resolver_structural.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_agents.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver.py
{src,tests}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:
example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases. Vendor names may only appear in: (1) Operations design page provider list (docs/design/operations.md), (2).claude/skill/agent files, (3) third-party import paths/module names. Tests must usetest-provider,test-small-001, etc.
Files:
tests/unit/api/controllers/test_analytics.pysrc/synthorg/api/controllers/providers.pysrc/synthorg/settings/definitions/company.pytests/unit/settings/conftest.pysrc/synthorg/settings/config_bridge.pytests/unit/api/controllers/test_providers.pytests/unit/settings/test_resolver_structural.pysrc/synthorg/api/controllers/agents.pysrc/synthorg/settings/resolver.pysrc/synthorg/settings/definitions/providers.pytests/unit/api/controllers/test_company.pysrc/synthorg/api/controllers/departments.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_agents.pysrc/synthorg/api/controllers/analytics.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver.pysrc/synthorg/api/controllers/company.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Create new objects for immutability, never mutate existing ones. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement. Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).
Use frozen Pydantic models for config/identity; separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 conventions:BaseModel,model_validator,computed_field,ConfigDict. For derived values use@computed_fieldinstead of storing + validating redundant fields. UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task. Existing code is being migrated incrementally.
Handle errors explicitly, never silently swallow. Validate at system boundaries (user input, external APIs, config files).
Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport logging/logging.getLogger()/print()in application code.
Variable name for logger: alwayslogger(not_logger, notlog).
Event names: always use constants from the domain-specific module undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api,TOOL_INVOKE_STARTfromevents.tool,GIT_COMMAND_STARTfromevents.git, `CONTEXT_BUDGET_FILL_U...
Files:
src/synthorg/api/controllers/providers.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/api/controllers/agents.pysrc/synthorg/settings/resolver.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/api/controllers/departments.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Files:
src/synthorg/api/controllers/providers.pysrc/synthorg/api/controllers/agents.pysrc/synthorg/api/controllers/departments.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.py
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.502Z
Learning: Settings: Runtime-editable settings persistence (DB > env > YAML > code defaults), typed definitions (9 namespaces), Fernet encryption for sensitive values, config bridge, ConfigResolver (typed composed reads for controllers), validation, registry, change notifications via message bus. Per-namespace setting definitions in definitions/ submodule (api, company, providers, memory, budget, security, coordination, observability, backup).
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to tests/**/*.py : Test markers: `pytest.mark.unit`, `pytest.mark.integration`, `pytest.mark.e2e`, `pytest.mark.slow`. Coverage: 80% minimum. Async: `asyncio_mode = 'auto'` — no manual `pytest.mark.asyncio` needed. Timeout: 30 seconds per test. Parallelism: `pytest-xdist` via `-n auto` — ALWAYS include `-n auto` when running pytest, never run tests sequentially.
Applied to files:
tests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_providers.pytests/unit/api/controllers/test_company.py
📚 Learning: 2026-03-17T21:13:19.502Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.502Z
Learning: Settings: Runtime-editable settings persistence (DB > env > YAML > code defaults), typed definitions (9 namespaces), Fernet encryption for sensitive values, config bridge, ConfigResolver (typed composed reads for controllers), validation, registry, change notifications via message bus. Per-namespace setting definitions in definitions/ submodule (api, company, providers, memory, budget, security, coordination, observability, backup).
Applied to files:
CLAUDE.mdsrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/providers.pydocs/design/operations.md
📚 Learning: 2026-03-17T21:13:19.502Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.502Z
Learning: Persistence backend: pluggable PersistenceBackend protocol in `src/synthorg/persistence/`, SQLite initial, SettingsRepository (namespaced settings CRUD).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-17T21:13:19.502Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.502Z
Learning: Documentation source in `docs/` (Markdown, built with Zensical). Design spec in `docs/design/` (7 pages: index, agents, organization, communication, engine, memory, operations). Architecture in `docs/architecture/` (overview, tech-stack, decision log). Roadmap in `docs/roadmap/`. Security in `docs/security.md`. Licensing in `docs/licensing.md`. Reference in `docs/reference/`. REST API reference in `docs/rest-api.md`. Library reference in `docs/api/` (auto-generated from docstrings). Custom templates in `docs/overrides/`. Config in `mkdocs.yml`.
Applied to files:
CLAUDE.mddocs/design/operations.md
📚 Learning: 2026-03-17T21:13:19.502Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.502Z
Learning: 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).
Applied to files:
CLAUDE.mddocs/design/operations.md
📚 Learning: 2026-03-17T21:13:19.502Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.502Z
Learning: Engine: Agent orchestration, execution loops, parallel execution, task decomposition, routing, task assignment, centralized single-writer task state engine (TaskEngine), task lifecycle, recovery, shutdown, workspace isolation, coordination (multi-agent pipeline: TopologyDispatcher protocol, 4 dispatchers — SAS/centralized/decentralized/context-dependent, wave execution, workspace lifecycle integration, CoordinationSectionConfig company config bridge, build_coordinator factory), coordination error classification, prompt policy validation, checkpoint recovery (checkpoint/, per-turn persistence, heartbeat detection, CheckpointRecoveryStrategy), approval gate (escalation detection, context parking/resume, EscalationInfo/ResumePayload models), stagnation detection (stagnation/, StagnationDetector protocol, ToolRepetitionDetector, dual-signal analysis, corrective prompt injection), agent runtime state (AgentRuntimeState, lightweight per-agent execution status for dashboard queries and recove...
Applied to files:
CLAUDE.mddocs/design/operations.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig.
Applied to files:
src/synthorg/api/controllers/providers.pytests/unit/api/controllers/test_providers.pysrc/synthorg/settings/definitions/providers.pytests/unit/settings/test_resolver.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : All provider calls go through BaseCompletionProvider which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code — it's handled by the base class.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to src/synthorg/**/*.py : All provider calls go through `BaseCompletionProvider` which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code — it's handled by the base class.
Applied to files:
src/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Tests must use test-provider, test-small-001, etc. for vendor-agnostic test data.
Applied to files:
tests/unit/api/controllers/test_providers.py
📚 Learning: 2026-03-17T21:13:19.502Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.502Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
docs/design/operations.md
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to src/synthorg/**/*.py : Prefer `asyncio.TaskGroup` for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare `create_task`. Existing code is being migrated incrementally.
Applied to files:
src/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare create_task.
Applied to files:
src/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.py
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to tests/**/*.py : Prefer `pytest.mark.parametrize` for testing similar cases.
Applied to files:
tests/unit/settings/test_config_bridge.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Parametrize: Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/settings/test_config_bridge.py
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to src/synthorg/**/*.py : `RetryConfig` and `RateLimiterConfig` are set per-provider in `ProviderConfig`. Retryable errors (`is_retryable=True`): `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`. Non-retryable errors raise immediately without retry. `RetryExhaustedError` signals that all retries failed — the engine layer catches this to trigger fallback chains. Rate limiter respects `RateLimitError.retry_after` from providers — automatically pauses future requests.
Applied to files:
tests/unit/settings/test_resolver.py
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/company.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/company.py
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG for object creation, internal flow, entry/exit of key functions.
Applied to files:
src/synthorg/api/controllers/company.py
📚 Learning: 2026-03-17T21:13:19.501Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T21:13:19.501Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow. Validate at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/company.py
🧬 Code graph analysis (15)
tests/unit/api/controllers/test_analytics.py (4)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)tests/unit/hr/test_offboarding_service.py (2)
FakeMessageBus(113-174)get(41-42)
src/synthorg/api/controllers/providers.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_provider_configs(421-475)
src/synthorg/settings/definitions/company.py (3)
src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingType(24-36)
tests/unit/settings/conftest.py (2)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingSource(50-61)src/synthorg/settings/models.py (1)
SettingValue(212-232)
tests/unit/api/controllers/test_providers.py (4)
src/synthorg/settings/registry.py (1)
get_registry(111-113)tests/unit/settings/test_service.py (1)
service(106-113)tests/unit/api/conftest.py (4)
make_auth_headers(111-137)_make_test_auth_service(67-69)_seed_test_users(227-258)auth_service(149-150)src/synthorg/settings/encryption.py (1)
SettingsEncryptor(21-125)
tests/unit/settings/test_resolver_structural.py (3)
src/synthorg/settings/enums.py (1)
SettingNamespace(6-21)src/synthorg/settings/errors.py (1)
SettingNotFoundError(8-9)tests/unit/settings/conftest.py (4)
FakeAgentConfig(26-38)FakeDepartment(41-51)FakeProviderConfig(54-62)make_setting_value(9-20)
src/synthorg/api/controllers/agents.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_agents(313-365)
src/synthorg/settings/resolver.py (3)
src/synthorg/settings/errors.py (2)
SettingNotFoundError(8-9)SettingsEncryptionError(16-17)src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/service.py (1)
get(189-273)
src/synthorg/api/controllers/departments.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_departments(367-419)
tests/unit/api/controllers/test_departments.py (7)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)tests/unit/hr/test_offboarding_service.py (2)
FakeMessageBus(113-174)get(41-42)tests/unit/api/conftest.py (5)
make_auth_headers(111-137)fake_persistence(154-157)_make_test_auth_service(67-69)auth_service(149-150)cost_tracker(168-169)src/synthorg/api/app.py (1)
create_app(496-656)tests/unit/hr/promotion/conftest.py (1)
tracker(104-106)
tests/unit/api/controllers/test_agents.py (3)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)
src/synthorg/api/controllers/analytics.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (1)
get_agents(313-365)
tests/unit/settings/test_config_bridge.py (1)
src/synthorg/settings/config_bridge.py (2)
_serialize_value(17-56)extract_from_config(59-92)
tests/unit/settings/test_resolver.py (5)
src/synthorg/settings/enums.py (1)
SettingNamespace(6-21)src/synthorg/settings/models.py (1)
SettingValue(212-232)tests/unit/settings/test_resolver_structural.py (1)
resolver(48-52)src/synthorg/settings/resolver.py (2)
ConfigResolver(35-676)_parse_bool(716-737)tests/unit/settings/conftest.py (4)
FakeAgentConfig(26-38)FakeDepartment(41-51)FakeProviderConfig(54-62)make_setting_value(9-20)
src/synthorg/api/controllers/company.py (2)
src/synthorg/api/state.py (1)
config_resolver(252-254)src/synthorg/settings/resolver.py (3)
get_str(73-95)get_agents(313-365)get_departments(367-419)
🔇 Additional comments (19)
src/synthorg/settings/config_bridge.py (2)
53-56: LGTM on bool and fallback handling.The lowercase
"true"/"false"for booleans aligns with JSON conventions, and thestr()fallback for other scalars is appropriate.
59-92: LGTM on extract_from_config update.The function now properly delegates to
_serialize_valuefor JSON-compatible output. The docstring accurately documents the new behavior for Pydantic models, collections, and scalars.src/synthorg/settings/definitions/company.py (1)
53-75: LGTM on new JSON setting registrations.The
agentsanddepartmentssettings are properly registered with:
- Appropriate
JSONtype for structural datayaml_pathmatchingRootConfigattributesdefault=Noneenabling graceful fallback to YAML config- Consistent
Structuregroup for organizational hierarchydocs/design/operations.md (1)
1044-1044: LGTM on Settings documentation update.The updated description accurately captures:
- All 6 setting types including the new
JSONtype- The 4-layer resolution chain (DB > env > YAML > code defaults)
ConfigResolvertyped accessors for both scalar and structural data- Graceful fallback semantics for
get_agents,get_departments,get_provider_configsThe terminal punctuation issue from the previous review has been addressed.
CLAUDE.md (1)
131-132: LGTM on CLAUDE.md settings description.The updated package description accurately reflects the new capabilities:
- JSON type for structural data
- Config bridge JSON serialization for Pydantic models/collections
- Typed scalar and structural data accessors with validation fallbacks
src/synthorg/api/controllers/departments.py (2)
43-44: LGTM on list_departments migration.Clean migration to resolver-based access. The
get_departments()method handles JSON parsing, validation, and fallback internally.
66-67: LGTM on get_department migration.Consistent pattern with
get_agent— fetches the full collection then iterates to find the match. Error handling and logging remain unchanged.src/synthorg/api/controllers/agents.py (2)
18-19: LGTM on docstring update.The class docstring now accurately describes that agent configurations are "resolved through settings" rather than read directly from
RootConfig. This addresses the previous review feedback.
43-44: LGTM on resolver migration.Both
list_agentsandget_agentcleanly migrate to the resolver pattern, consistent with the departments controller.Also applies to: 66-67
src/synthorg/api/controllers/providers.py (2)
46-47: LGTM on list_providers migration.The migration to resolver-based access is clean. The
_safe_providerfunction correctly stripsapi_keybefore returning to clients.
69-70: LGTM on get_provider and list_models migrations.Both endpoints cleanly migrate to the resolver pattern. Error handling with
NotFoundErrorremains intact.Also applies to: 96-97
src/synthorg/api/controllers/company.py (3)
44-57: LGTM on parallel fetching with TaskGroup and error handling.The implementation correctly:
- Uses
asyncio.TaskGroupfor structured concurrency (per coding guidelines)- Logs the
ExceptionGroupwithSETTINGS_FETCH_FAILEDbefore re-raising (addresses previous review feedback)- Preserves exception chain with
raise eg.exceptions[0] from eg- Uses the correct
except ExceptionGroup as eg:syntax (PEP 758 compliant — no parentheses needed for single exception type)
58-62: LGTM on response construction from parallel results.Clean extraction of results from completed tasks using
.result(). Themodel_dump(mode="json")calls ensure JSON-serializable output.
79-80: LGTM on list_departments migration.Consistent resolver-based pattern matching the standalone
DepartmentController.tests/unit/api/controllers/test_providers.py (1)
41-52: Good security assertion on redacted provider secretsThis test correctly verifies
_safe_provider()stripsapi_keyfrom serialized output.src/synthorg/settings/resolver.py (1)
266-311:get_json()error-path handling looks solidParsing, decryption failure logging, and invalid JSON diagnostics are explicit and consistent with resolver behavior.
tests/unit/settings/conftest.py (2)
9-20: Helper fixture constructor is clean and reusable
make_setting_value()is typed, minimal, and keeps resolver tests consistent.
26-62: Fixture models are well-structuredThe frozen fake models and concise class docstrings are clear and fit the structural resolver test usage.
tests/unit/api/controllers/test_company.py (1)
23-39: Good update to concrete departments expectationAsserting an explicit empty list for departments makes the contract tighter than a generic type check.
| assert resp.status_code == 500 | ||
| body = resp.json() | ||
| assert body.get("success") is False or "detail" in body | ||
|
|
There was a problem hiding this comment.
Tighten the 500 error payload assertion
body.get("success") is False or "detail" in body is overly permissive and can pass with unintended payloads. Assert a single expected error shape (or two explicit allowed shapes) deterministically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/controllers/test_company.py` around lines 126 - 129, The
current assertion is too permissive; change it to assert a deterministic error
shape by requiring both that body.get("success") is False and that a "detail"
key exists (i.e., replace `body.get("success") is False or "detail" in body`
with `assert body.get("success") is False and "detail" in body`) so the test
explicitly verifies the expected error payload for resp/body in
tests/unit/api/controllers/test_company.py.
| def test_single_model(self) -> None: | ||
| model = _InnerConfig(daily_limit=20.0, enabled=False) | ||
| result = _serialize_value(model) | ||
| parsed = json.loads(result) | ||
| assert parsed == {"daily_limit": 20.0, "enabled": False} | ||
|
|
||
| def test_tuple_of_models(self) -> None: | ||
| items = ( | ||
| _ItemModel(name="a", value=1), | ||
| _ItemModel(name="b", value=2), | ||
| ) | ||
| result = _serialize_value(items) | ||
| parsed = json.loads(result) | ||
| assert parsed == [ | ||
| {"name": "a", "value": 1}, | ||
| {"name": "b", "value": 2}, | ||
| ] | ||
|
|
||
| def test_dict_of_models(self) -> None: | ||
| providers = { | ||
| "p1": _InnerConfig(daily_limit=5.0), | ||
| "p2": _InnerConfig(daily_limit=15.0), | ||
| } | ||
| result = _serialize_value(providers) | ||
| parsed = json.loads(result) | ||
| assert parsed["p1"]["daily_limit"] == 5.0 | ||
| assert parsed["p2"]["daily_limit"] == 15.0 | ||
|
|
||
| def test_tuple_of_strings(self) -> None: | ||
| tags = ("alpha", "beta", "gamma") | ||
| result = _serialize_value(tags) | ||
| assert json.loads(result) == ["alpha", "beta", "gamma"] | ||
|
|
||
| def test_empty_tuple(self) -> None: | ||
| result = _serialize_value(()) | ||
| assert result == "[]" | ||
|
|
||
| def test_empty_dict(self) -> None: | ||
| result = _serialize_value({}) | ||
| assert result == "{}" | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ("value", "expected"), | ||
| [ | ||
| ("hello", "hello"), | ||
| (42, "42"), | ||
| (3.14, "3.14"), | ||
| (True, "true"), | ||
| (False, "false"), | ||
| ], | ||
| ) | ||
| def test_scalar_values(self, value: object, expected: str) -> None: | ||
| assert _serialize_value(value) == expected | ||
|
|
||
| def test_mixed_list_models_and_scalars(self) -> None: | ||
| items = [_ItemModel(name="x", value=1), "plain", 42] | ||
| result = _serialize_value(items) | ||
| parsed = json.loads(result) | ||
| assert parsed == [{"name": "x", "value": 1}, "plain", 42] | ||
|
|
||
| def test_mixed_dict_models_and_scalars(self) -> None: | ||
| providers: dict[str, object] = { | ||
| "a": _InnerConfig(daily_limit=5.0), | ||
| "b": "just-a-string", | ||
| } | ||
| result = _serialize_value(providers) | ||
| parsed = json.loads(result) | ||
| assert parsed == { | ||
| "a": {"daily_limit": 5.0, "enabled": True}, | ||
| "b": "just-a-string", | ||
| } | ||
|
|
There was a problem hiding this comment.
Reject unsupported top-level values instead of silently stringifying them.
These new tests lock in the happy paths, but _serialize_value() still falls through to str() for unsupported top-level inputs like set or bytes. That contradicts the helper’s own TypeError contract and would serialize Python repr strings if one of those types is ever bridged. Please add a regression here and tighten the helper or its docstring accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/test_config_bridge.py` around lines 73 - 144, The tests
reveal _serialize_value currently falls back to str() for unsupported top-level
types (e.g., set, bytes) which violates its TypeError contract; update the
_serialize_value implementation (and its docstring) so that after handling
allowed top-level kinds (scalars: str/int/float/bool, pydantic/model instances
like _InnerConfig/_ItemModel, lists/tuples, dicts) it raises a TypeError for any
other top-level value instead of stringifying it; locate and modify the
_serialize_value function to add this explicit rejection and adjust the
docstring to document the exact accepted types.
- Add ExceptionGroup warning log in analytics controller (matches company.py) - Recursive _to_json_compatible for nested BaseModel serialization in config bridge - Raise TypeError for unsupported types in _serialize_value (set, bytes, etc.) - Extract _resolve_list_setting / _resolve_dict_setting helpers in resolver (DRY, each public method now under 50 lines) - Add HTTP status assertions in company/provider DB-override tests - Add defensive-copy regression test for get_provider_configs
5b1e6ac to
52d626a
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/controllers/analytics.py`:
- Around line 61-74: The current TaskGroup lumps
app_state.config_resolver.get_agents() with non-settings calls so the except
block wrongly emits SETTINGS_FETCH_FAILED for unrelated failures; fix by
separating the resolver call out and only emit SETTINGS_FETCH_FAILED (with a
valid settings namespace such as "api") when
app_state.config_resolver.get_agents() fails—e.g., call get_agents() in its own
try/except and log SETTINGS_FETCH_FAILED (namespace="api") only on that
exception, while keeping the list_tasks()/get_total_cost() TaskGroup and
emitting a different analytics/error event for their failures.
In `@src/synthorg/settings/config_bridge.py`:
- Around line 7-12: extract_from_config() delegates to _serialize_value() which
may raise TypeError for unsupported objects, but that exception currently
escapes without logging the failing yaml_path; update extract_from_config (and
the error handling around _serialize_value) to catch TypeError, log a structured
warning/error via the module logger (use get_logger() and the existing
SETTINGS_CONFIG_PATH_MISS event or a clear message) including the yaml_path and
the original exception before re-raising the error so callers retain the
stacktrace and you have context for the failing setting path.
In `@tests/unit/api/controllers/test_agents.py`:
- Around line 118-124: Add an explicit HTTP status assertion before parsing the
JSON response in the test: after calling client.get("/api/v1/agents") and
obtaining resp (in the TestClient block where make_auth_headers("observer") is
used), assert resp.status_code equals the expected success code (e.g., 200)
before calling resp.json(); this aligns the test with other DB override tests
and makes failures clearer.
In `@tests/unit/api/controllers/test_analytics.py`:
- Around line 45-47: The test class TestAnalyticsControllerDbOverride is a
DB-backed/integration test and should be marked with `@pytest.mark.integration`
instead of `@pytest.mark.unit`; update the decorator on the
TestAnalyticsControllerDbOverride class accordingly and likewise replace
`@pytest.mark.unit` with `@pytest.mark.integration` for the other DB-override
controller test classes in this file so all multi-component tests use the
integration marker.
In `@tests/unit/api/controllers/test_departments.py`:
- Around line 40-77: Add a new assertion in the
test_db_departments_override_config test to exercise the detail endpoint: after
the existing list GET (client.get("/api/v1/departments")), call
client.get("/api/v1/departments/db-dept") and assert the response body contains
the expected department (e.g., body["name"] == "db-dept" and status code is 200)
to ensure DepartmentController.get_department() uses the settings override;
reference the existing test_db_departments_override_config function, the
settings_service.set call that seeds "company"/"departments", and the
DepartmentController.get_department resolver-backed read to locate where to add
the new GET/assertion.
In `@tests/unit/api/controllers/test_providers.py`:
- Around line 59-104: The test test_db_providers_override_config currently only
checks the list endpoint; add a complementary assertion that the named-provider
detail endpoint also reads the seeded override. After the existing list
assertions, perform a GET to "/api/v1/providers/db-provider" (using the same
TestClient and auth headers from the test), assert a 200 response, that the
returned provider key exists and that "api_key" is absent (or None) on the
detail response; this ensures SettingsService overrides are honored by the
detail handler in create_app.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94879049-5cd6-4506-be8c-f82d1d690c60
📒 Files selected for processing (20)
CLAUDE.mddocs/design/operations.mdsrc/synthorg/api/controllers/agents.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/company.pysrc/synthorg/api/controllers/departments.pysrc/synthorg/api/controllers/providers.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/resolver.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_providers.pytests/unit/settings/conftest.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver.pytests/unit/settings/test_resolver_structural.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling — PEP 758 except syntax enforced by ruff on Python 3.14
All public functions must have type hints, and mypy strict mode must pass
Use Google-style docstrings on all public classes and functions; docstrings are enforced by ruff D rules
Line length must not exceed 88 characters (enforced by ruff)
Functions must be less than 50 lines, and files less than 800 lines
Files:
tests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver_structural.pytests/unit/settings/test_resolver.pytests/unit/api/controllers/test_providers.pysrc/synthorg/api/controllers/company.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/api/controllers/agents.pysrc/synthorg/api/controllers/analytics.pytests/unit/api/controllers/test_analytics.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/providers.pytests/unit/settings/conftest.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_departments.pysrc/synthorg/settings/resolver.pytests/unit/api/controllers/test_company.pysrc/synthorg/api/controllers/providers.pysrc/synthorg/api/controllers/departments.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark unit tests with@pytest.mark.unit, integration tests with@pytest.mark.integration, e2e tests with@pytest.mark.e2e, and slow tests with@pytest.mark.slow
Maintain 80% code coverage minimum (enforced in CI)
Useasyncio_mode = 'auto'in pytest config — no manual@pytest.mark.asyncioneeded
Tests must have a 30-second timeout per test
Always run tests with-n autofor parallelism via pytest-xdist — never run tests sequentially
Prefer@pytest.mark.parametrizefor testing similar cases in Python tests
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names for tests only:test-provider,test-small-001, etc.
Use Hypothesis for property-based testing with@givenand@settingsdecorators. Run dev profile withHYPOTHESIS_PROFILE=devfor 1000 examples (default ci profile uses 200)
Never skip, dismiss, or ignore flaky tests — fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins
Files:
tests/unit/settings/test_config_bridge.pytests/unit/settings/test_resolver_structural.pytests/unit/settings/test_resolver.pytests/unit/api/controllers/test_providers.pytests/unit/api/controllers/test_analytics.pytests/unit/settings/conftest.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_company.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Create new objects instead of mutating existing ones; usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement on non-Pydantic internal collections (registries,BaseTool)
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config and identity; use separate mutable-via-copy models (withmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 withBaseModel,model_validator,computed_field, andConfigDict
Use@computed_fieldfor derived values instead of storing and validating redundant fields (e.g.,TokenUsage.total_tokens)
UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g., multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task
Handle errors explicitly and never silently swallow exceptions
Validate input at system boundaries (user input, external APIs, config files)
Every module with business logic must have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport logging/logging.getLogger()/print()
Always useloggeras the variable name (not_logger, notlog) for logger instances
Always use event name constants fromsynthorg.observability.events.<domain>module (e.g.,API_REQUEST_STARTEDfromevents.api). Import directly: `from synthorg.obs...
Files:
src/synthorg/api/controllers/company.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/api/controllers/agents.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/resolver.pysrc/synthorg/api/controllers/providers.pysrc/synthorg/api/controllers/departments.py
src/synthorg/settings/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/settings/**/*.py: Use Fernet encryption for sensitive settings at rest (viacryptographylibrary). Settings namespaces: api, company, providers, memory, budget, security, coordination, observability, backup
Settings must be persisted in a typed, runtime-editable system with precedence: DB > env > YAML > code defaults. UseConfigResolverfor composed reads
Files:
src/synthorg/settings/config_bridge.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/resolver.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation source is in
docs/(Markdown). Built with Zensical. Read from mkdocs.yml configuration
Files:
docs/design/operations.md
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Design spec pages are in
docs/design/(7 pages: index, agents, organization, communication, engine, memory, operations). Always read relevant design pages before implementing
Files:
docs/design/operations.md
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.493Z
Learning: Applies to src/synthorg/settings/**/*.py : Settings must be persisted in a typed, runtime-editable system with precedence: DB > env > YAML > code defaults. Use `ConfigResolver` for composed reads
📚 Learning: 2026-03-17T22:03:39.493Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.493Z
Learning: Applies to src/synthorg/settings/**/*.py : Settings must be persisted in a typed, runtime-editable system with precedence: DB > env > YAML > code defaults. Use `ConfigResolver` for composed reads
Applied to files:
CLAUDE.mdtests/unit/settings/test_resolver.pysrc/synthorg/settings/resolver.pydocs/design/operations.md
📚 Learning: 2026-03-17T22:03:39.493Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.493Z
Learning: Applies to src/synthorg/settings/**/*.py : Use Fernet encryption for sensitive settings at rest (via `cryptography` library). Settings namespaces: api, company, providers, memory, budget, security, coordination, observability, backup
Applied to files:
CLAUDE.mdtests/unit/settings/test_resolver.pysrc/synthorg/settings/resolver.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Parametrize: Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/settings/test_config_bridge.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to tests/**/*.py : Prefer `pytest.mark.parametrize` for testing similar cases in Python tests
Applied to files:
tests/unit/settings/test_config_bridge.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig.
Applied to files:
tests/unit/settings/test_resolver.pytests/unit/api/controllers/test_providers.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/api/controllers/providers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test timeout: 30 seconds per test.
Applied to files:
tests/unit/api/controllers/test_providers.pytests/unit/api/controllers/test_agents.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Tests must use test-provider, test-small-001, etc. for vendor-agnostic test data.
Applied to files:
tests/unit/api/controllers/test_providers.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to tests/**/*.py : Tests must have a 30-second timeout per test
Applied to files:
tests/unit/api/controllers/test_providers.pytests/unit/api/controllers/test_agents.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare create_task.
Applied to files:
src/synthorg/api/controllers/company.pysrc/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Prefer `asyncio.TaskGroup` for fan-out/fan-in parallel operations in new code (e.g., multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare `create_task`
Applied to files:
src/synthorg/api/controllers/company.pysrc/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/company.pysrc/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : All error paths must log at WARNING or ERROR with context before raising an exception
Applied to files:
src/synthorg/api/controllers/company.pysrc/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : For `dict`/`list` fields in frozen Pydantic models, use `copy.deepcopy()` at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : For dict/list fields in frozen Pydantic models, rely on frozen=True for field reassignment prevention and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : For non-Pydantic internal collections (registries, `BaseTool`), use `copy.deepcopy()` at construction and `MappingProxyType` wrapping for read-only enforcement
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Use Pydantic v2 with `BaseModel`, `model_validator`, `computed_field`, and `ConfigDict`
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Use frozen Pydantic models for config and identity; use separate mutable-via-copy models (with `model_copy(update=...)`) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Handle errors explicitly and never silently swallow exceptions
Applied to files:
src/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to tests/**/*.py : NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names for tests only: `test-provider`, `test-small-001`, etc.
Applied to files:
tests/unit/settings/conftest.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : All provider calls go through BaseCompletionProvider which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code — it's handled by the base class.
Applied to files:
src/synthorg/api/controllers/providers.py
🧬 Code graph analysis (11)
tests/unit/settings/test_resolver_structural.py (4)
src/synthorg/settings/enums.py (1)
SettingNamespace(6-21)src/synthorg/settings/errors.py (1)
SettingNotFoundError(8-9)src/synthorg/settings/resolver.py (4)
get_json(268-313)get_agents(411-430)get_departments(432-451)get_provider_configs(453-472)tests/unit/settings/conftest.py (4)
FakeAgentConfig(26-38)FakeDepartment(41-51)FakeProviderConfig(54-62)make_setting_value(9-20)
tests/unit/api/controllers/test_providers.py (6)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)tests/unit/hr/test_offboarding_service.py (2)
FakeMessageBus(113-174)get(41-42)tests/unit/api/conftest.py (3)
make_auth_headers(111-137)_make_test_auth_service(67-69)auth_service(149-150)src/synthorg/settings/encryption.py (1)
SettingsEncryptor(21-125)
src/synthorg/api/controllers/company.py (2)
src/synthorg/api/state.py (1)
config_resolver(259-261)src/synthorg/settings/resolver.py (3)
get_str(75-97)get_agents(411-430)get_departments(432-451)
src/synthorg/api/controllers/analytics.py (2)
src/synthorg/api/state.py (1)
config_resolver(259-261)src/synthorg/settings/resolver.py (1)
get_agents(411-430)
src/synthorg/settings/definitions/company.py (3)
src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingType(24-36)
src/synthorg/settings/definitions/providers.py (3)
src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingType(24-36)
tests/unit/settings/conftest.py (2)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-21)SettingSource(50-61)src/synthorg/settings/models.py (1)
SettingValue(212-232)
tests/unit/api/controllers/test_agents.py (5)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)tests/unit/settings/test_service.py (1)
service(106-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)tests/unit/api/conftest.py (1)
cost_tracker(168-169)
src/synthorg/settings/resolver.py (1)
src/synthorg/settings/errors.py (2)
SettingNotFoundError(8-9)SettingsEncryptionError(16-17)
src/synthorg/api/controllers/providers.py (2)
src/synthorg/api/state.py (1)
config_resolver(259-261)src/synthorg/settings/resolver.py (1)
get_provider_configs(453-472)
src/synthorg/api/controllers/departments.py (2)
src/synthorg/api/state.py (1)
config_resolver(259-261)src/synthorg/settings/resolver.py (1)
get_departments(432-451)
🔇 Additional comments (17)
src/synthorg/settings/config_bridge.py (1)
17-29: Recursive JSON normalization looks solid.Walking nested lists and dicts before
json.dumps()closes the old repr-based gap for embedded Pydantic models.src/synthorg/settings/resolver.py (1)
315-472: The structural getter factoring is a good cleanup.Centralizing the JSON → shape → validation → fallback flow in
_resolve_list_setting()and_resolve_dict_setting()keeps the public accessors small and consistent.src/synthorg/api/controllers/providers.py (1)
46-47: Good move to resolver-backed provider reads.All three endpoints now honor runtime settings overrides instead of reading the YAML snapshot directly.
Also applies to: 69-70, 96-97
src/synthorg/api/controllers/company.py (1)
43-57: Nice use ofTaskGrouphere.These resolver reads are independent, and the grouped warning preserves a clear failure path.
tests/unit/settings/conftest.py (1)
54-62:⚠️ Potential issue | 🟡 MinorKeep the shared provider fixture generic.
FakeProviderConfig.driver = "litellm"bakes a concrete integration name into every structural settings test that uses this fixture. Prefer a neutral value such astest-driverhere, and keep the new override payloads aligned, so the shared test data stays provider-agnostic.Based on learnings, "Tests must use test-provider, test-small-001, etc. for vendor-agnostic test data."
⛔ Skipped due to learnings
Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T11:48:14.867Z Learning: Applies to tests/**/*.py : Tests must use test-provider, test-small-001, etc. for vendor-agnostic test data.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-17T22:03:39.492Z Learning: Applies to tests/**/*.py : NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names for tests only: `test-provider`, `test-small-001`, etc.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-17T22:03:39.492Z Learning: Applies to src/**/*.py : NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names: `example-provider`, `example-large-001`, `example-medium-001`, `example-small-001`, `large`/`medium`/`small` as aliases. Tests must use `test-provider`, `test-small-001`, etc.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-17T22:03:39.492Z Learning: Applies to src/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`; never duplicate retry logic in driver codeLearnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T11:48:14.867Z Learning: Applies to {src,tests,web,cli,site}/**/*.{py,ts,tsx,go,astro} : Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names: example-provider, example-large-001, example-medium-001, example-small-001. Vendor names may only appear in: (1) Operations design page provider list (docs/design/operations.md), (2) .claude/ skill/agent files, (3) third-party import paths/module names.tests/unit/settings/test_resolver_structural.py (5)
1-52: LGTM! Well-structured test module with comprehensive fixture setup.The module docstring clearly explains the purpose, and the fixtures (
mock_settings,root_config,resolver) provide clean test setup. The_FakeRootConfigmodel appropriately mirrors the real config structure with frozen Pydantic semantics.
58-97: LGTM! Comprehensive coverage ofget_json()edge cases.Tests cover valid JSON (arrays/objects), invalid JSON error propagation,
SettingNotFoundErrorpropagation, and empty array handling. Mock assertions verify correct delegation to the settings service.
102-215: LGTM! Thorough testing ofget_agents()fallback behavior.The tests cover all critical scenarios: successful JSON parsing, empty list as valid override, fallback on invalid JSON, schema validation failures, and wrong JSON shape (dict instead of list). This aligns well with the resolver's graceful fallback contract.
220-329: LGTM! Consistent test coverage forget_departments().Test structure mirrors
TestGetAgentsappropriately, covering the same fallback scenarios for department configurations.
449-468: Good addition: Defensive copy regression test.This test verifies that mutating the returned dict from
get_provider_configs()does not affect the underlying config state. Using JSON"null"to trigger fallback is a clean approach. This addresses the previous review suggestion for mutation regression coverage.tests/unit/api/controllers/test_company.py (4)
23-45: LGTM! Test class properly updated with timeout and assertion improvements.The
@pytest.mark.timeout(30)marker is correctly added, andtest_list_departmentsnow asserts the expected empty list explicitly rather than just checking type.
53-90: LGTM! Well-structured DB override integration test.The test properly seeds DB data via
SettingsService.set(), injects the service into app creation, and verifies the API returns DB-overridden departments. Status code assertion at line 86 is correctly placed before body inspection.
92-130: LGTM! Error handling test with tightened assertions.The test verifies that TaskGroup exceptions are properly unwrapped to clean API error responses. The assertions are now deterministic: checking status 500,
successisFalse, anderroris notNone.
132-169: LGTM! Complete DB override test for company overview endpoint.The test verifies that DB-stored agents appear in the
/api/v1/companyresponse. Status code assertion at line 165 is correctly placed.src/synthorg/api/controllers/agents.py (2)
18-45: LGTM! Controller correctly updated to use ConfigResolver.The
list_agentsmethod now fetches agents asynchronously viaconfig_resolver.get_agents(), and the docstring accurately reflects the new data source ("resolved through settings"). The pagination logic remains unchanged.
47-72: LGTM!get_agentmethod properly updated.The method now uses the same resolver pattern as
list_agents. The linear search through agents is acceptable given that agent lists are typically small. Error logging with structured kwargs follows the project conventions.tests/unit/api/controllers/test_agents.py (1)
19-74: LGTM! Test class updated with timeout and SettingsService integration.The
@pytest.mark.timeout(30)marker is correctly added per coding guidelines. Thetest_list_agents_with_datatest now properly instantiates and injectsSettingsServiceinto app creation, enabling the resolver-based agent retrieval path.
| try: | ||
| async with asyncio.TaskGroup() as tg: | ||
| t_tasks = tg.create_task(app_state.persistence.tasks.list_tasks()) | ||
| t_cost = tg.create_task(app_state.cost_tracker.get_total_cost()) | ||
| t_agents = tg.create_task(app_state.config_resolver.get_agents()) | ||
| except ExceptionGroup as eg: | ||
| logger.warning( | ||
| SETTINGS_FETCH_FAILED, | ||
| namespace="analytics", | ||
| key="_overview", | ||
| error_count=len(eg.exceptions), | ||
| exc_info=True, | ||
| ) | ||
| raise eg.exceptions[0] from eg |
There was a problem hiding this comment.
Don't emit SETTINGS_FETCH_FAILED for every TaskGroup failure.
This group wraps list_tasks() and get_total_cost() as well as config_resolver.get_agents(), so the current branch will label non-settings failures as settings outages. namespace="analytics" is also not a valid settings namespace, which can skew settings-focused alerting and log filters. Use an API/analytics event here, or only emit SETTINGS_FETCH_FAILED when the resolver task is the one that failed.
Based on learnings, "Settings namespaces: api, company, providers, memory, budget, security, coordination, observability, backup".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/analytics.py` around lines 61 - 74, The current
TaskGroup lumps app_state.config_resolver.get_agents() with non-settings calls
so the except block wrongly emits SETTINGS_FETCH_FAILED for unrelated failures;
fix by separating the resolver call out and only emit SETTINGS_FETCH_FAILED
(with a valid settings namespace such as "api") when
app_state.config_resolver.get_agents() fails—e.g., call get_agents() in its own
try/except and log SETTINGS_FETCH_FAILED (namespace="api") only on that
exception, while keeping the list_tasks()/get_total_cost() TaskGroup and
emitting a different analytics/error event for their failures.
- Use API_REQUEST_ERROR instead of SETTINGS_FETCH_FAILED in analytics TaskGroup handler (mixed settings + non-settings calls) - Catch TypeError from _serialize_value in extract_from_config with yaml_path context logging before re-raise - Add HTTP status assertion in agents DB-override test - Mark all DbOverride test classes as @pytest.mark.integration - Add detail endpoint assertions in departments and providers DB-override tests (get_department, get_provider)
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/settings/config_bridge.py`:
- Around line 78-80: The docstring describing scalar serialization is incorrect:
update the scalar serialization sentence in the docstring of config_bridge.py
(the section describing "For Pydantic models, tuples/lists..., dicts... For
scalars...") to note the boolean exception — i.e., state that booleans are
serialized as the lowercase strings "true"/"false" while other scalars use
str(value); make the wording match the actual implementation around the boolean
handling (lines referencing "true"/"false") so the docstring and code behavior
are consistent.
In `@tests/unit/api/controllers/test_agents.py`:
- Around line 118-125: Add a new assertion to exercise the agent detail endpoint
after the list assertions: use the same TestClient and auth
(make_auth_headers("observer")) to GET "/api/v1/agents/db-agent-1", assert
resp.status_code == 200, parse resp.json() and assert the returned agent's
"name" == "db-agent-1" (and any expected fields like "id" or "type" as
appropriate) so the resolver-backed read via the named-agent path is verified;
place this right after the existing list checks in the test function that
contains the TestClient block.
In `@tests/unit/api/controllers/test_analytics.py`:
- Around line 84-91: Add an explicit status-code assertion before parsing the
response in the test: after calling client.get("/api/v1/analytics/overview")
assert resp.status_code == 200 (or the expected HTTP status) prior to calling
resp.json(), so that the TestClient call in the test_analytics.py block (where
TestClient(app) is used and client.get(...) is invoked) checks the response
status first and then inspects body["success"], body["data"]["total_agents"],
body["data"]["total_tasks"], and body["data"]["total_cost_usd"].
In `@tests/unit/api/controllers/test_company.py`:
- Around line 53-161: Multiple tests (test_db_company_departments_override,
test_taskgroup_error_returns_clean_error_response,
test_db_company_overview_includes_db_agents) duplicate setup; extract a pytest
fixture that builds RootConfig, calls _make_test_auth_service and
_seed_test_users, constructs SettingsService, and returns the created app and
settings_service so tests can reuse create_app and settings initialization;
update the three tests to accept the fixture (e.g., db_override_app) and use the
returned app and settings_service instead of repeating the setup logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b17278a-137f-448e-9149-37394e7c451b
📒 Files selected for processing (7)
src/synthorg/api/controllers/analytics.pysrc/synthorg/settings/config_bridge.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_providers.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Deploy Preview
- GitHub Check: Type Check
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling — PEP 758 except syntax enforced by ruff on Python 3.14
All public functions must have type hints, and mypy strict mode must pass
Use Google-style docstrings on all public classes and functions; docstrings are enforced by ruff D rules
Line length must not exceed 88 characters (enforced by ruff)
Functions must be less than 50 lines, and files less than 800 lines
Files:
src/synthorg/settings/config_bridge.pysrc/synthorg/api/controllers/analytics.pytests/unit/api/controllers/test_providers.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Create new objects instead of mutating existing ones; usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement on non-Pydantic internal collections (registries,BaseTool)
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config and identity; use separate mutable-via-copy models (withmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 withBaseModel,model_validator,computed_field, andConfigDict
Use@computed_fieldfor derived values instead of storing and validating redundant fields (e.g.,TokenUsage.total_tokens)
UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g., multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task
Handle errors explicitly and never silently swallow exceptions
Validate input at system boundaries (user input, external APIs, config files)
Every module with business logic must have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport logging/logging.getLogger()/print()
Always useloggeras the variable name (not_logger, notlog) for logger instances
Always use event name constants fromsynthorg.observability.events.<domain>module (e.g.,API_REQUEST_STARTEDfromevents.api). Import directly: `from synthorg.obs...
Files:
src/synthorg/settings/config_bridge.pysrc/synthorg/api/controllers/analytics.py
src/synthorg/settings/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/settings/**/*.py: Use Fernet encryption for sensitive settings at rest (viacryptographylibrary). Settings namespaces: api, company, providers, memory, budget, security, coordination, observability, backup
Settings must be persisted in a typed, runtime-editable system with precedence: DB > env > YAML > code defaults. UseConfigResolverfor composed reads
Files:
src/synthorg/settings/config_bridge.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark unit tests with@pytest.mark.unit, integration tests with@pytest.mark.integration, e2e tests with@pytest.mark.e2e, and slow tests with@pytest.mark.slow
Maintain 80% code coverage minimum (enforced in CI)
Useasyncio_mode = 'auto'in pytest config — no manual@pytest.mark.asyncioneeded
Tests must have a 30-second timeout per test
Always run tests with-n autofor parallelism via pytest-xdist — never run tests sequentially
Prefer@pytest.mark.parametrizefor testing similar cases in Python tests
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names for tests only:test-provider,test-small-001, etc.
Use Hypothesis for property-based testing with@givenand@settingsdecorators. Run dev profile withHYPOTHESIS_PROFILE=devfor 1000 examples (default ci profile uses 200)
Never skip, dismiss, or ignore flaky tests — fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins
Files:
tests/unit/api/controllers/test_providers.pytests/unit/api/controllers/test_agents.pytests/unit/api/controllers/test_departments.pytests/unit/api/controllers/test_analytics.pytests/unit/api/controllers/test_company.py
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.493Z
Learning: Applies to src/synthorg/settings/**/*.py : Settings must be persisted in a typed, runtime-editable system with precedence: DB > env > YAML > code defaults. Use `ConfigResolver` for composed reads
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : For `dict`/`list` fields in frozen Pydantic models, use `copy.deepcopy()` at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : For dict/list fields in frozen Pydantic models, rely on frozen=True for field reassignment prevention and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : For non-Pydantic internal collections (registries, `BaseTool`), use `copy.deepcopy()` at construction and `MappingProxyType` wrapping for read-only enforcement
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Use Pydantic v2 with `BaseModel`, `model_validator`, `computed_field`, and `ConfigDict`
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Use frozen Pydantic models for config and identity; use separate mutable-via-copy models (with `model_copy(update=...)`) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare create_task.
Applied to files:
src/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Prefer `asyncio.TaskGroup` for fan-out/fan-in parallel operations in new code (e.g., multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare `create_task`
Applied to files:
src/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Handle errors explicitly and never silently swallow exceptions
Applied to files:
src/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to src/**/*.py : Always use event name constants from `synthorg.observability.events.<domain>` module (e.g., `API_REQUEST_STARTED` from `events.api`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/api/controllers/analytics.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test timeout: 30 seconds per test.
Applied to files:
tests/unit/api/controllers/test_providers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Tests must use test-provider, test-small-001, etc. for vendor-agnostic test data.
Applied to files:
tests/unit/api/controllers/test_providers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig.
Applied to files:
tests/unit/api/controllers/test_providers.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to tests/**/*.py : Tests must have a 30-second timeout per test
Applied to files:
tests/unit/api/controllers/test_providers.py
📚 Learning: 2026-03-17T22:03:39.493Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.493Z
Learning: Applies to src/synthorg/settings/**/*.py : Settings must be persisted in a typed, runtime-editable system with precedence: DB > env > YAML > code defaults. Use `ConfigResolver` for composed reads
Applied to files:
tests/unit/api/controllers/test_providers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.e2e, pytest.mark.slow. Coverage: 80% minimum (enforced in CI).
Applied to files:
tests/unit/api/controllers/test_analytics.py
📚 Learning: 2026-03-17T22:03:39.492Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:03:39.492Z
Learning: Applies to tests/**/*.py : Mark unit tests with `pytest.mark.unit`, integration tests with `pytest.mark.integration`, e2e tests with `pytest.mark.e2e`, and slow tests with `pytest.mark.slow`
Applied to files:
tests/unit/api/controllers/test_analytics.py
🧬 Code graph analysis (3)
src/synthorg/api/controllers/analytics.py (2)
src/synthorg/api/state.py (1)
config_resolver(259-261)src/synthorg/settings/resolver.py (1)
get_agents(411-430)
tests/unit/api/controllers/test_providers.py (5)
src/synthorg/config/schema.py (1)
RootConfig(387-651)src/synthorg/settings/registry.py (1)
get_registry(111-113)tests/unit/api/conftest.py (5)
make_auth_headers(111-137)_make_test_auth_service(67-69)_seed_test_users(227-258)auth_service(149-150)cost_tracker(168-169)src/synthorg/api/app.py (1)
create_app(533-700)src/synthorg/settings/encryption.py (1)
SettingsEncryptor(21-125)
tests/unit/api/controllers/test_agents.py (4)
src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(152-637)src/synthorg/api/app.py (1)
create_app(533-700)tests/unit/api/fakes.py (1)
FakePersistenceBackend(408-511)
🔇 Additional comments (10)
src/synthorg/settings/config_bridge.py (2)
17-29: Good recursive normalization for nested models.This correctly handles
BaseModelinstances at arbitrary depth inside lists/tuples/dicts and avoids mutating the input object graph.
104-114: Nice error-path logging before re-raise.Catching serialization
TypeError, logging withyaml_pathcontext, and re-raising preserves traceback while improving diagnosability.tests/unit/api/controllers/test_company.py (5)
1-21: LGTM!Imports are well-organized and all are utilized by the new test class. The additions for
json,AsyncMock, and the settings-related modules appropriately support the new DB-override integration tests.
23-45: LGTM!The timeout marker addition aligns with coding guidelines. The assertion change on line 38 to
== []correctly reflects the expected behavior when no departments are configured in DB or YAML.
48-91: LGTM - past review comments addressed.The status code assertion on line 86 addresses the previous feedback. The test properly validates DB-override behavior for departments.
92-131: LGTM - error assertions tightened.The assertions on lines 129-130 now check both
body["success"] is Falseandbody["error"] is not None, addressing the previous feedback about overly permissive error shape validation.
132-169: LGTM - status assertion present.The test correctly validates that DB-seeded agents appear in the company overview, with proper status code assertion on line 165.
src/synthorg/api/controllers/analytics.py (1)
61-75: LGTM! TaskGroup concurrency with proper error handling.The parallel fetch pattern correctly uses
asyncio.TaskGroupfor fan-out operations, properly unwrapsExceptionGroupto preserve the API error contract, and logs with the appropriateAPI_REQUEST_ERRORevent before re-raising. The.result()calls on completed tasks are safe after theTaskGroupcontext exits successfully.tests/unit/api/controllers/test_departments.py (1)
35-83: LGTM! Comprehensive DB override test with list and detail coverage.The integration test properly seeds a DB-backed department via
SettingsService, verifies the list endpoint returns the override, and exercises the detail endpoint to ensureget_department()also uses resolver-backed reads. Both endpoints have explicit status assertions.tests/unit/api/controllers/test_providers.py (1)
54-110: LGTM! Comprehensive DB override test with proper security handling.The integration test properly:
- Creates a
SettingsEncryptorfor handling sensitive provider fields- Seeds DB-backed providers via
SettingsService- Verifies both list and detail endpoints return the override
- Confirms
api_keyis stripped in both response types- Includes explicit HTTP status assertions
- Fix extract_from_config docstring to document boolean serialization - Add agent detail endpoint assertion in DB-override test - Add HTTP status assertion in analytics DB-override test - Extract shared db_override_app fixture in test_company.py (DRY)
🤖 I have created a release *beep* *boop* --- ## [0.3.2](v0.3.1...v0.3.2) (2026-03-17) ### Features * **settings:** route structural data reads through SettingsService ([#525](#525)) ([289f604](289f604)) ### Bug Fixes * **cli:** add fallback arch detection in PowerShell installer ([#529](#529)) ([0250afb](0250afb)), closes [#521](#521) ### CI/CD * bump the minor-and-patch group with 2 updates ([#517](#517)) ([46bdd1a](46bdd1a)) * bump wrangler from 4.73.0 to 4.74.0 in /.github in the minor-and-patch group ([#511](#511)) ([903b71a](903b71a)) ### Maintenance * bump node from `7a4ef57` to `44bcbf4` in /docker/sandbox ([#515](#515)) ([3cbddd1](3cbddd1)) * bump python from `6a27522` to `584e89d` in /docker/backend ([#513](#513)) ([0715910](0715910)) * bump python from `6a27522` to `584e89d` in /docker/sandbox ([#514](#514)) ([787dfe1](787dfe1)) * bump the minor-and-patch group across 1 directory with 2 updates ([#527](#527)) ([e96c0d4](e96c0d4)) * bump the minor-and-patch group across 2 directories with 3 updates ([#512](#512)) ([b95ba3d](b95ba3d)) * **docker:** disable Mem0 telemetry in container config ([#531](#531)) ([9fc29eb](9fc29eb)) * improve GitHub issue templates with structured forms ([#528](#528)) ([4fb66cf](4fb66cf)), closes [#522](#522) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
config_bridgeserialization to produce valid JSON for Pydantic models, tuples/lists, and dicts (was usingstr()which produced Python repr)company/agents,company/departments,providers/configsget_json(),get_agents(),get_departments(),get_provider_configs()accessors toConfigResolverwith graceful fallback to YAML config on invalid JSON, schema validation failures, or wrong JSON shapeRootConfigreads to resolver calls, enabling runtime DB overrides via the settings resolution chain (DB > env > YAML > code defaults)CompanyController.get_companyusingasyncio.TaskGroupdict()copy from provider fallback to prevent mutation of frozen config stateCloses #502
Review coverage
Pre-reviewed by 12 agents (docs-consistency, code-reviewer, python-reviewer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, logging-audit, resilience-audit, conventions-enforcer, async-concurrency-reviewer, test-quality-reviewer, issue-resolution-verifier). 11 findings addressed across 2 commits.
Key fixes from review:
ValidationErrorfrommodel_validate()— prevents unhandled 500s on structurally-invalid DB dataisinstanceguards for JSON shape (list vs dict) before iteratingany()instead ofvalue[0]check in_serialize_valuefor heterogeneous collectionstest_resolver_structural.py(keepstest_resolver.pyunder 800 lines)Test plan
_serialize_value(10 cases),extract_from_configstructural (5 cases)get_json(5 cases),get_agents(6 cases),get_departments(5 cases),get_provider_configs(6 cases) — including ValidationError fallback and wrong-shape fallback