feat: implement settings persistence layer (DB-backed config)#495
Merged
feat: implement settings persistence layer (DB-backed config)#495
Conversation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a full runtime settings subsystem: registry, models, encryption, SettingsService, namespaced persistence (protocol + SQLite repo), API controller and app-state wiring, migration to schema v9, observability events, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as "Settings API\nController"
participant Service as "SettingsService"
participant Registry as "Settings\nRegistry"
participant Repo as "Settings\nRepository"
participant DB as "SQLite\nDatabase"
participant Encryptor as "Fernet\nEncryptor"
participant Bus as "Message\nBus"
Client->>API: GET /api/v1/settings/{ns}/{key}
API->>Service: get(namespace, key)
Service->>Registry: get(namespace, key)
Registry-->>Service: SettingDefinition
Service->>Repo: get(namespace, key)
Repo->>DB: SELECT value, updated_at
DB-->>Repo: (value, updated_at) | None
alt value present & sensitive
Service->>Encryptor: decrypt(value)
Encryptor-->>Service: plaintext
end
Service-->>API: SettingValue
API-->>Client: 200
Client->>API: PUT /api/v1/settings/{ns}/{key}
API->>Service: set(namespace, key, value)
Service->>Registry: get(namespace, key)
Registry-->>Service: SettingDefinition
Service->>Service: validate(value)
alt sensitive
Service->>Encryptor: encrypt(value)
Encryptor-->>Service: ciphertext
end
Service->>Repo: set(namespace, key, stored_value, updated_at)
Repo->>DB: INSERT/UPSERT
DB-->>Repo: OK
Service->>Bus: publish(SETTINGS_VALUE_SET)
Bus-->>Service: OK
Service-->>API: SettingEntry
API-->>Client: 200
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
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 |
Contributor
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 introduces a robust and flexible settings management system, enabling dynamic, runtime configuration of the application. It establishes a clear hierarchy for resolving setting values, from database overrides to code defaults, and incorporates essential features like data validation, encryption for sensitive information, and real-time change notifications. This foundational change significantly enhances the application's configurability and maintainability. 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.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthorg/api/app.py (1)
445-512:⚠️ Potential issue | 🟡 MinorInclude
settings_servicein the degraded-services startup warning.Line 445 adds a new optional dependency, but Line 501’s warning condition/message still tracks only persistence/message_bus/cost_tracker/task_engine. If
settings_serviceis missing, settings endpoints can return 503 without being called out at startup.🔧 Suggested patch
- if ( - persistence is None - or message_bus is None - or cost_tracker is None - or task_engine is None - ): + if ( + persistence is None + or message_bus is None + or cost_tracker is None + or task_engine is None + or settings_service is None + ): msg = ( - "create_app called without persistence, message_bus, " - "cost_tracker, and/or task_engine — controllers accessing " - "missing services will return 503. Use test fakes for testing." + "create_app called without persistence, message_bus, " + "cost_tracker, task_engine, and/or settings_service — " + "controllers accessing missing services will return 503. " + "Use test fakes for testing." ) logger.warning(API_APP_STARTUP, note=msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/app.py` around lines 445 - 512, The startup degraded-services warning omits the newly added settings_service: update the conditional that builds the warning (currently checking persistence, message_bus, cost_tracker, task_engine) to also include settings_service, and update the logged message text to mention settings_service so missing settings endpoints are called out; modify the same block that calls logger.warning(API_APP_STARTUP, note=msg) and references those symbols (settings_service, persistence, message_bus, cost_tracker, task_engine, API_APP_STARTUP, logger.warning) to include the new dependency.src/synthorg/persistence/sqlite/backend.py (1)
420-464: 🧹 Nitpick | 🔵 TrivialAvoid maintaining a second SQL path for
_systemsettings.Line 420 and Line 446 currently bypass
SQLiteSettingsRepositoryand duplicate persistence logic. This creates drift risk (for example, SQL behavior, timestamp conventions, and error-path consistency) between backend-level settings and the repository used bySettingsService.♻️ Proposed consolidation
+from datetime import UTC, datetime @@ async def get_setting(self, key: NotBlankStr) -> str | None: @@ - try: - cursor = await self._db.execute( - "SELECT value FROM settings WHERE namespace = '_system' AND key = ?", - (key,), - ) - row = await cursor.fetchone() - except (sqlite3.Error, aiosqlite.Error) as exc: + try: + row = await self.settings.get("_system", key) + except QueryError as exc: msg = f"Failed to get setting {key!r}" logger.exception( PERSISTENCE_SETTING_FETCH_FAILED, key=key, error=str(exc), ) raise QueryError(msg) from exc - return str(row[0]) if row else None + return row[0] if row else None @@ async def set_setting(self, key: NotBlankStr, value: str) -> None: @@ - try: - await self._db.execute( - "INSERT INTO settings (namespace, key, value, updated_at) " - "VALUES ('_system', ?, ?, datetime('now')) " - "ON CONFLICT(namespace, key) DO UPDATE SET " - "value=excluded.value, updated_at=excluded.updated_at", - (key, value), - ) - await self._db.commit() - except (sqlite3.Error, aiosqlite.Error) as exc: + try: + await self.settings.set( + "_system", + key, + value, + datetime.now(UTC).isoformat(), + ) + except QueryError as exc: msg = f"Failed to set setting {key!r}" logger.exception( PERSISTENCE_SETTING_SAVE_FAILED, key=key, error=str(exc), ) raise QueryError(msg) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/persistence/sqlite/backend.py` around lines 420 - 464, The get_setting and set_setting methods are duplicating SQL logic for the '_system' namespace instead of reusing the central SQLiteSettingsRepository; refactor these methods to delegate to SQLiteSettingsRepository (use its get and upsert/update methods) so the backend no longer executes raw SQL for settings, preserve the same namespace '_system' when calling the repository, and map/raise the same PersistenceConnectionError/QueryError semantics as before; locate this change inside the get_setting and set_setting functions and remove the direct execute/commit SQL paths so all timestamp, conflict, and error behavior is centralized in SQLiteSettingsRepository.
🤖 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/observability/events/settings.py`:
- Around line 1-14: Add two new observability event constants to complement
existing success events: define SETTINGS_SET_FAILED and SETTINGS_DELETE_FAILED
as Final[str] values following the same naming pattern (e.g.,
"settings.set.failed" and "settings.delete.failed") in the same module where
SETTINGS_VALUE_SET, SETTINGS_VALUE_DELETED, SETTINGS_VALUE_RESOLVED, etc. are
declared so they can be imported directly (from
synthorg.observability.events.settings import SETTINGS_SET_FAILED,
SETTINGS_DELETE_FAILED) and used by settings_repo.py for error logging where
success events are currently referenced.
In `@src/synthorg/persistence/sqlite/settings_repo.py`:
- Around line 141-149: In the error handlers that currently call
logger.exception with SETTINGS_VALUE_DELETED (used in the delete-setting and
delete_namespace code paths), replace that success-sounding event constant with
an error-specific one (e.g., SETTINGS_VALUE_DELETE_FAILED for the setting delete
and SETTINGS_NAMESPACE_DELETE_FAILED for delete_namespace) so the logged event
reflects failure; update the logger.exception calls in the delete_setting and
delete_namespace handlers to use the new error constants and keep the same
namespace/key/error fields and exception chaining.
- Around line 114-122: In the exception handler inside the settings repository
(the except block used when setting a key in the method that raises QueryError),
replace the incorrect success event constant SETTINGS_VALUE_SET with a
failure-specific constant (e.g. SETTINGS_SET_FAILED); if SETTINGS_SET_FAILED
does not yet exist, add it to events/settings.py (and consider adding
SETTINGS_DELETE_FAILED too) and then use SETTINGS_SET_FAILED in the
logger.exception call, leaving the namespace, key, and error fields intact so
the failure is logged with correct semantics.
In `@src/synthorg/settings/config_bridge.py`:
- Around line 7-9: The module currently creates a logger via
get_logger(__name__) and never uses it; either remove the unused logger variable
or wire it into the path resolution code so failures are logged at DEBUG level.
Locate the path resolution logic in this module and, in any except blocks or
error branches, call logger.debug(...) with the resolved path input and the
exception or failure reason; if you prefer removal, delete the logger =
get_logger(__name__) line and any unused imports.
In `@src/synthorg/settings/definitions/__init__.py`:
- Around line 7-16: Add an explicit __all__ list to this package initializer to
declare the public API and avoid linter false-positives for the imported modules
(backup, budget, company, coordination, memory, observability, providers,
security); keep the existing imports for side effects/registration and then
define __all__ =
["backup","budget","company","coordination","memory","observability","providers","security"]
so tools and readers know these names are intentionally exported.
In `@src/synthorg/settings/encryption.py`:
- Around line 110-115: In the from_env path where you currently call
logger.exception(SETTINGS_ENCRYPTION_ERROR, operation="from_env", error=msg) (in
src/synthorg/settings/encryption.py inside the from_env flow), replace
logger.exception with logger.error (or logger.error(..., exc_info=False)) so you
log the validation failure without an exception traceback, keeping the same
structured fields (operation="from_env", error=msg), then raise
SettingsEncryptionError(msg) as before.
In `@src/synthorg/settings/service.py`:
- Around line 378-389: The DB-return path currently leaves raw ciphertext in
value when definition.sensitive is True but self._encryptor is None; update the
logic in this helper (the block constructing SettingEntry) to mirror the fix in
get(): if definition.sensitive then if self._encryptor is present use
self._encryptor.decrypt(raw_value) else do not return raw_value but set display
to _SENSITIVE_MASK; ensure SettingEntry(...) uses display for value and keep
source=SettingSource.DATABASE and updated_at as before.
- Around line 594-600: The except block that catches Exception when sending
settings notifications (in service.py where SETTINGS_NOTIFICATION_FAILED is
logged with namespace and key) should include the actual exception details;
modify the except clause to capture the exception (e.g., "except Exception as
e:") and pass the exception information to the logger (use exc_info=True or
include str(e) in the message) so notification failures are logged with
stack/exception context for debugging.
- Around line 211-233: The code returns raw encrypted ciphertext when a
sensitive DB setting is read while self._encryptor is None (raw_value used as
value), so modify the branch in the result-handling block (where raw_value,
updated_at are unpacked) to detect if definition.sensitive and self._encryptor
is None and raise SettingsEncryptionError (the same error used in set()),
otherwise decrypt via self._encryptor.decrypt(raw_value) as before; keep the
existing caching behavior (only cache when not definition.sensitive) and
preserve the SettingValue construction and logging paths for the decrypted
value.
- Around line 134-141: The _check_range function raises SettingValidationError
with the raw numeric value; update it to mask the actual value when the
SettingDefinition indicates a sensitive setting (e.g., check
definition.sensitive or equivalent attribute) so error messages remain
consistent with other validators—compose messages like "Value <hidden> below
minimum {definition.min_value}" or "Value <hidden> above maximum
{definition.max_value}" for sensitive definitions, and keep the existing
detailed messages for non-sensitive settings; adjust only _check_range and reuse
SettingDefinition and SettingValidationError symbols.
- Around line 126-131: The JSON validator _validate_json currently includes the
raw json.JSONDecodeError text in the SettingValidationError, risking leakage of
sensitive content; change _validate_json to raise SettingValidationError with a
generic message like "Invalid JSON" (keep "from exc" so the original exception
is preserved as the cause but do not include exc in the public message). Then
update the call site in _validate_by_type to ensure sensitive settings use the
new generic-error behavior (or pass an is_sensitive flag if you prefer) so no
raw JSON or exception text is ever incorporated into the SettingValidationError
message.
In `@tests/unit/api/controllers/test_settings.py`:
- Around line 10-11: The module currently calls make_auth_headers at import time
to create _HEADERS and _OBSERVER_HEADERS which can break test isolation;
refactor by removing those top-level calls and instead add module-scoped pytest
fixtures (e.g., ceo_headers and observer_headers) that call make_auth_headers
when the fixture runs, then update test functions to accept these fixtures (use
ceo_headers / observer_headers parameters) and replace references to _HEADERS
and _OBSERVER_HEADERS with the fixture names; ensure fixtures are module-scoped
if you want per-module reuse.
- Around line 83-97: Extend test_delete_setting to verify post-delete resolution
by performing a GET on "/api/v1/settings/budget/total_monthly" after the DELETE
and asserting the returned value/source matches the expected fallback
(env/YAML/default) behavior; update the test_delete_setting function to call the
GET endpoint (same headers) and assert body["value"] and/or body["source"]
equals the known fallback value/source used in your settings resolution logic.
In `@tests/unit/api/fakes.py`:
- Around line 425-426: Add a brief clarifying comment near the
FakeSettingsRepository usage explaining that the class exposes both a new
namespaced API via the settings property (backed by _settings_repo /
FakeSettingsRepository) and the legacy flat API via get_setting/set_setting
(backed by the _settings dict), and that they intentionally use separate storage
to mirror production behavior so tests using either API remain accurate; place
this comment adjacent to the declarations of _settings_repo, _settings, and the
settings/get_setting/set_setting methods.
- Around line 547-551: delete_namespace currently rebuilds self._store inside
the loop for each key which is inefficient; instead compute keys once (as
already done) and rebuild the dict a single time or remove keys in-place: update
the function delete_namespace to use one dict comprehension like rebuilding
self._store = {kk: vv for kk, vv in self._store.items() if kk not in keys} (or
iterate keys and call pop on self._store) and then return len(keys); reference
symbols: delete_namespace, self._store, keys.
In `@tests/unit/persistence/sqlite/test_migrations_v9.py`:
- Around line 31-37: The v8_db fixture claims to produce a DB at schema version
8 but calls run_migrations(memory_db), which migrates to the current schema and
breaks the fixture contract; update v8_db to either (A) remove the
run_migrations call and construct a true v8 DB manually (create tables/rows
matching V8 schema) so it returns a database with user_version 8, or (B) if you
intended a current-schema DB, rename the fixture to reflect that (e.g.,
current_db) and keep run_migrations; change references accordingly to use the
correct fixture name; ensure the fixture sets/prunes user_version and schema
objects via memory_db so tests relying on v8_db get the expected schema.
- Around line 178-182: The current test in
tests/unit/persistence/sqlite/test_migrations_v9.py reads settings via cursor =
await memory_db.execute("SELECT namespace, key, value FROM settings") and only
asserts len(rows) >= 1 and rows[0][0] == "_system", which is too weak; replace
these with deterministic checks that validate exact migrated content from
settings_old: fetch rows into the rows list, assert an exact expected length
(e.g., equals the number of entries in settings_old), and assert that rows
contain the exact tuples (namespace, key, value) expected (either by comparing
sorted lists or comparing sets) so duplicates/incorrect merges are detected (use
the existing cursor/rows variables and compare against the known settings_old
values).
In `@tests/unit/settings/test_config_bridge.py`:
- Around line 22-23: Add the pytest timeout marker to the TestExtractFromConfig
test class so tests cannot run longer than 30 seconds; modify the class
decorator to include pytest.mark.timeout(30) (i.e., change or augment the
existing `@pytest.mark.unit` on TestExtractFromConfig to also apply
`@pytest.mark.timeout`(30) or combine markers) to ensure the class-level timeout
is enforced for all tests within.
In `@tests/unit/settings/test_definitions.py`:
- Around line 18-31: The two tests test_all_definitions_load_without_error and
test_no_duplicate_registrations are redundant because both only assert
registry.size > 0; either merge them into a single test that names the intent
clearly (e.g., test_definitions_load_and_unique) or enhance
test_no_duplicate_registrations to assert uniqueness explicitly: after calling
get_registry(), iterate the registry entries (or use registry.list /
registry.items / registry._entries depending on available API) and assert no
duplicate keys/composite identifiers exist (for example by comparing len(list)
vs len(set(...)) of the unique identifier for each definition). Ensure you
reference the registry returned from get_registry() and use its public accessors
to check uniqueness rather than relying solely on size.
- Around line 14-15: Add a 30-second pytest timeout marker to the test class:
decorate TestDefinitionsLoading with `@pytest.mark.timeout`(30) (either on its own
line or combined with the existing `@pytest.mark.unit` decorator) so every test in
the class uses a 30s timeout; ensure pytest is imported if not already.
In `@tests/unit/settings/test_models.py`:
- Around line 1-12: Add a 30-second timeout marker to the test module by
defining a module-level pytest mark (e.g., add "pytestmark =
pytest.mark.timeout(30)") near the top of tests/unit/settings/test_models.py so
all tests in this file (which reference imports like pytest, ValidationError,
and models SettingDefinition/SettingEntry/SettingValue) inherit the 30s timeout
per the coding guidelines.
---
Outside diff comments:
In `@src/synthorg/api/app.py`:
- Around line 445-512: The startup degraded-services warning omits the newly
added settings_service: update the conditional that builds the warning
(currently checking persistence, message_bus, cost_tracker, task_engine) to also
include settings_service, and update the logged message text to mention
settings_service so missing settings endpoints are called out; modify the same
block that calls logger.warning(API_APP_STARTUP, note=msg) and references those
symbols (settings_service, persistence, message_bus, cost_tracker, task_engine,
API_APP_STARTUP, logger.warning) to include the new dependency.
In `@src/synthorg/persistence/sqlite/backend.py`:
- Around line 420-464: The get_setting and set_setting methods are duplicating
SQL logic for the '_system' namespace instead of reusing the central
SQLiteSettingsRepository; refactor these methods to delegate to
SQLiteSettingsRepository (use its get and upsert/update methods) so the backend
no longer executes raw SQL for settings, preserve the same namespace '_system'
when calling the repository, and map/raise the same
PersistenceConnectionError/QueryError semantics as before; locate this change
inside the get_setting and set_setting functions and remove the direct
execute/commit SQL paths so all timestamp, conflict, and error behavior is
centralized in SQLiteSettingsRepository.
🪄 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: 5e892f4c-5b57-4016-be29-22c046d4aa11
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
pyproject.tomlsrc/synthorg/api/app.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/state.pysrc/synthorg/observability/events/settings.pysrc/synthorg/persistence/protocol.pysrc/synthorg/persistence/repositories.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/persistence/sqlite/migrations.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/settings/__init__.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/settings/definitions/budget.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/coordination.pysrc/synthorg/settings/definitions/memory.pysrc/synthorg/settings/definitions/observability.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/encryption.pysrc/synthorg/settings/enums.pysrc/synthorg/settings/errors.pysrc/synthorg/settings/models.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/service.pytests/unit/api/conftest.pytests/unit/api/controllers/test_settings.pytests/unit/api/fakes.pytests/unit/config/conftest.pytests/unit/observability/test_events.pytests/unit/persistence/sqlite/test_migrations_v6.pytests/unit/persistence/sqlite/test_migrations_v9.pytests/unit/persistence/sqlite/test_settings_repo.pytests/unit/persistence/test_migrations_v2.pytests/unit/persistence/test_protocol.pytests/unit/settings/__init__.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_definitions.pytests/unit/settings/test_encryption.pytests/unit/settings/test_models.pytests/unit/settings/test_registry.pytests/unit/settings/test_service.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: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- 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) per PEP 758 — ruff enforces this on Python 3.14
Add type hints to all public functions and classes; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules
Keep line length to 88 characters (ruff enforced)
Files:
src/synthorg/persistence/repositories.pysrc/synthorg/persistence/protocol.pytests/unit/config/conftest.pysrc/synthorg/settings/models.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/errors.pysrc/synthorg/api/controllers/__init__.pytests/unit/api/conftest.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/settings/encryption.pysrc/synthorg/observability/events/settings.pysrc/synthorg/settings/enums.pysrc/synthorg/settings/definitions/__init__.pytests/unit/observability/test_events.pytests/unit/settings/test_service.pytests/unit/settings/test_definitions.pytests/unit/settings/test_config_bridge.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/definitions/coordination.pytests/unit/settings/test_registry.pytests/unit/api/fakes.pytests/unit/settings/test_encryption.pytests/unit/persistence/sqlite/test_migrations_v9.pysrc/synthorg/api/app.pysrc/synthorg/settings/definitions/budget.pytests/unit/persistence/sqlite/test_migrations_v6.pysrc/synthorg/persistence/sqlite/backend.pytests/unit/persistence/sqlite/test_settings_repo.pysrc/synthorg/settings/definitions/observability.pysrc/synthorg/persistence/sqlite/migrations.pysrc/synthorg/api/state.pytests/unit/persistence/test_migrations_v2.pytests/unit/settings/test_models.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/definitions/memory.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/settings/__init__.pytests/unit/api/controllers/test_settings.pysrc/synthorg/settings/service.pytests/unit/persistence/test_protocol.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Create new objects for immutability instead of mutating existing ones; 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/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 (BaseModel,model_validator,computed_field,ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; useNotBlankStrfor all identifier/name fields
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate at system boundaries (user input, external APIs, config files)
Every module with business logic must importfrom synthorg.observability import get_loggerand createlogger = get_logger(__name__); never useimport loggingorlogging.getLogger()orprint()in application code
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget); import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Pure data models, enums, and re-e...
Files:
src/synthorg/persistence/repositories.pysrc/synthorg/persistence/protocol.pysrc/synthorg/settings/models.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/errors.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/settings/encryption.pysrc/synthorg/observability/events/settings.pysrc/synthorg/settings/enums.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/definitions/coordination.pysrc/synthorg/api/app.pysrc/synthorg/settings/definitions/budget.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/settings/definitions/observability.pysrc/synthorg/persistence/sqlite/migrations.pysrc/synthorg/api/state.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/definitions/memory.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/settings/__init__.pysrc/synthorg/settings/service.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
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set 30-second timeout per test
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python (@given+@settings); useciprofile (200 examples, default) anddevprofile (1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
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/config/conftest.pytests/unit/api/conftest.pytests/unit/observability/test_events.pytests/unit/settings/test_service.pytests/unit/settings/test_definitions.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_registry.pytests/unit/api/fakes.pytests/unit/settings/test_encryption.pytests/unit/persistence/sqlite/test_migrations_v9.pytests/unit/persistence/sqlite/test_migrations_v6.pytests/unit/persistence/sqlite/test_settings_repo.pytests/unit/persistence/test_migrations_v2.pytests/unit/settings/test_models.pytests/unit/api/controllers/test_settings.pytests/unit/persistence/test_protocol.py
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: Use Pydantic v2 inpyproject.tomlwithmem0ai(Mem0 memory backend) pinned as a required dependency with==version specifier
Pin all dependency versions using==inpyproject.toml; organize into groups:test(pytest + plugins, hypothesis),dev(includes test + ruff, mypy, pre-commit, commitizen, pip-audit)
Files:
pyproject.toml
🧠 Learnings (10)
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using `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/models.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`); use `computed_field` for derived values instead of storing redundant fields; use `NotBlankStr` for all identifier/name fields
Applied to files:
src/synthorg/settings/models.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow exceptions
Applied to files:
src/synthorg/settings/errors.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/errors.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/observability/events/settings.pytests/unit/observability/test_events.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/settings.pytests/unit/observability/test_events.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.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/providers/**/*.py : Set `RetryConfig` and `RateLimiterConfig` per-provider in `ProviderConfig`; retryable errors (`RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`) are retried automatically; non-retryable errors raise immediately; `RetryExhaustedError` signals all retries failed
Applied to files:
src/synthorg/settings/definitions/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 : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
src/synthorg/api/state.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic must import `from synthorg.observability import get_logger` and create `logger = get_logger(__name__)`; never use `import logging` or `logging.getLogger()` or `print()` in application code
Applied to files:
src/synthorg/api/state.py
🧬 Code graph analysis (35)
src/synthorg/persistence/repositories.py (2)
src/synthorg/persistence/sqlite/settings_repo.py (5)
get(32-56)get_namespace(58-78)get_all(80-95)delete(129-157)delete_namespace(159-181)src/synthorg/settings/service.py (2)
get_namespace(310-335)get_all(337-360)
src/synthorg/persistence/protocol.py (4)
src/synthorg/persistence/repositories.py (1)
SettingsRepository(665-771)src/synthorg/persistence/sqlite/backend.py (1)
settings(412-418)tests/unit/api/fakes.py (1)
settings(502-503)tests/unit/persistence/test_protocol.py (1)
settings(358-359)
src/synthorg/settings/models.py (3)
src/synthorg/persistence/sqlite/backend.py (1)
settings(412-418)src/synthorg/persistence/protocol.py (1)
settings(165-167)src/synthorg/settings/enums.py (4)
SettingLevel(38-46)SettingNamespace(6-20)SettingSource(49-60)SettingType(23-35)
src/synthorg/settings/config_bridge.py (1)
tests/unit/settings/test_service.py (1)
config(83-84)
src/synthorg/api/controllers/__init__.py (1)
src/synthorg/api/controllers/settings.py (1)
SettingsController(45-220)
tests/unit/api/conftest.py (2)
src/synthorg/settings/registry.py (1)
get_registry(96-98)src/synthorg/settings/service.py (1)
SettingsService(144-600)
src/synthorg/settings/encryption.py (3)
src/synthorg/settings/errors.py (1)
SettingsEncryptionError(16-17)tests/unit/settings/test_encryption.py (1)
key(15-16)src/synthorg/settings/service.py (1)
get(181-283)
src/synthorg/settings/definitions/__init__.py (4)
src/synthorg/persistence/sqlite/backend.py (1)
settings(412-418)src/synthorg/persistence/protocol.py (1)
settings(165-167)tests/unit/api/fakes.py (1)
settings(502-503)tests/unit/persistence/test_protocol.py (1)
settings(358-359)
tests/unit/settings/test_service.py (4)
src/synthorg/settings/encryption.py (3)
SettingsEncryptor(21-125)decrypt(56-88)encrypt(31-54)src/synthorg/settings/registry.py (3)
SettingsRegistry(6-89)register(21-35)get(37-47)src/synthorg/settings/service.py (7)
SettingsService(144-600)get(181-283)delete(508-540)get_entry(285-308)get_schema(542-554)get_namespace(310-335)get_all(337-360)src/synthorg/persistence/sqlite/settings_repo.py (4)
get(32-56)delete(129-157)get_namespace(58-78)get_all(80-95)
tests/unit/settings/test_definitions.py (2)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/registry.py (5)
SettingsRegistry(6-89)get_registry(96-98)size(87-89)namespaces(78-84)list_all(65-76)
tests/unit/settings/test_config_bridge.py (1)
src/synthorg/settings/config_bridge.py (1)
extract_from_config(12-36)
src/synthorg/settings/definitions/company.py (3)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (1)
register(21-35)
src/synthorg/settings/definitions/security.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (1)
register(21-35)
src/synthorg/settings/definitions/providers.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (2)
get_registry(96-98)register(21-35)
src/synthorg/settings/definitions/coordination.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (2)
get_registry(96-98)register(21-35)
tests/unit/settings/test_registry.py (3)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (7)
SettingsRegistry(6-89)register(21-35)get(37-47)list_namespace(49-63)list_all(65-76)namespaces(78-84)size(87-89)
tests/unit/api/fakes.py (4)
src/synthorg/persistence/sqlite/backend.py (3)
settings(412-418)get_setting(420-444)set_setting(446-472)src/synthorg/persistence/protocol.py (3)
settings(165-167)get_setting(169-181)set_setting(183-195)tests/unit/persistence/test_protocol.py (20)
settings(358-359)get_setting(361-362)get(51-52)get(149-150)get(184-185)get(204-205)get(237-238)get(254-255)get(265-266)set_setting(364-365)get_namespace(268-269)get_all(271-272)delete(63-64)delete(158-159)delete(196-197)delete(213-214)delete(246-247)delete(260-261)delete(277-278)delete_namespace(280-281)src/synthorg/persistence/sqlite/settings_repo.py (5)
get(32-56)get_namespace(58-78)get_all(80-95)delete(129-157)delete_namespace(159-181)
tests/unit/settings/test_encryption.py (2)
src/synthorg/settings/encryption.py (4)
SettingsEncryptor(21-125)encrypt(31-54)decrypt(56-88)from_env(91-125)src/synthorg/settings/errors.py (1)
SettingsEncryptionError(16-17)
tests/unit/persistence/sqlite/test_migrations_v9.py (1)
src/synthorg/persistence/sqlite/migrations.py (4)
_apply_v9(466-513)get_user_version(283-287)run_migrations(531-599)set_user_version(290-309)
src/synthorg/api/app.py (2)
src/synthorg/settings/service.py (1)
SettingsService(144-600)src/synthorg/api/state.py (1)
settings_service(225-227)
src/synthorg/settings/definitions/budget.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (2)
get_registry(96-98)register(21-35)
tests/unit/persistence/sqlite/test_migrations_v6.py (1)
tests/unit/persistence/test_migrations_v2.py (1)
test_schema_version_is_nine(32-33)
src/synthorg/persistence/sqlite/backend.py (2)
src/synthorg/persistence/sqlite/settings_repo.py (1)
SQLiteSettingsRepository(19-181)src/synthorg/persistence/protocol.py (3)
settings(165-167)get_setting(169-181)set_setting(183-195)
tests/unit/persistence/sqlite/test_settings_repo.py (1)
src/synthorg/persistence/sqlite/settings_repo.py (6)
SQLiteSettingsRepository(19-181)get(32-56)delete(129-157)get_namespace(58-78)get_all(80-95)delete_namespace(159-181)
src/synthorg/settings/definitions/observability.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (2)
get_registry(96-98)register(21-35)
src/synthorg/api/state.py (3)
src/synthorg/persistence/sqlite/backend.py (1)
settings(412-418)src/synthorg/persistence/protocol.py (1)
settings(165-167)src/synthorg/settings/service.py (1)
SettingsService(144-600)
tests/unit/persistence/test_migrations_v2.py (1)
tests/unit/persistence/sqlite/test_migrations_v6.py (1)
test_schema_version_is_nine(20-21)
tests/unit/settings/test_models.py (2)
src/synthorg/settings/enums.py (4)
SettingLevel(38-46)SettingNamespace(6-20)SettingSource(49-60)SettingType(23-35)src/synthorg/settings/models.py (3)
SettingDefinition(14-79)SettingEntry(105-126)SettingValue(82-102)
src/synthorg/settings/definitions/backup.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (2)
get_registry(96-98)register(21-35)
src/synthorg/settings/registry.py (2)
src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/service.py (1)
get(181-283)
src/synthorg/settings/definitions/memory.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(14-79)src/synthorg/settings/registry.py (2)
get_registry(96-98)register(21-35)
src/synthorg/settings/__init__.py (4)
src/synthorg/settings/enums.py (4)
SettingLevel(38-46)SettingNamespace(6-20)SettingSource(49-60)SettingType(23-35)src/synthorg/settings/errors.py (4)
SettingNotFoundError(8-9)SettingsEncryptionError(16-17)SettingsError(4-5)SettingValidationError(12-13)src/synthorg/settings/models.py (3)
SettingDefinition(14-79)SettingEntry(105-126)SettingValue(82-102)src/synthorg/settings/registry.py (2)
SettingsRegistry(6-89)get_registry(96-98)
tests/unit/api/controllers/test_settings.py (4)
tests/unit/api/conftest.py (1)
make_auth_headers(100-126)src/synthorg/persistence/sqlite/settings_repo.py (2)
get(32-56)delete(129-157)src/synthorg/settings/service.py (2)
get(181-283)delete(508-540)src/synthorg/settings/registry.py (1)
get(37-47)
src/synthorg/settings/service.py (5)
src/synthorg/settings/config_bridge.py (1)
extract_from_config(12-36)src/synthorg/settings/models.py (3)
SettingDefinition(14-79)SettingEntry(105-126)SettingValue(82-102)src/synthorg/persistence/repositories.py (17)
SettingsRepository(665-771)get(62-74)get(221-233)get(349-361)get(429-441)get(554-566)get(621-633)get(672-689)get_namespace(691-706)get_all(708-718)delete(98-110)delete(263-275)delete(399-411)delete(471-483)delete(586-598)delete(649-661)delete(740-757)src/synthorg/settings/encryption.py (3)
SettingsEncryptor(21-125)decrypt(56-88)encrypt(31-54)src/synthorg/settings/registry.py (4)
SettingsRegistry(6-89)get(37-47)list_namespace(49-63)list_all(65-76)
tests/unit/persistence/test_protocol.py (2)
src/synthorg/persistence/repositories.py (18)
SettingsRepository(665-771)get(62-74)get(221-233)get(349-361)get(429-441)get(554-566)get(621-633)get(672-689)get_namespace(691-706)get_all(708-718)delete(98-110)delete(263-275)delete(399-411)delete(471-483)delete(586-598)delete(649-661)delete(740-757)delete_namespace(759-771)src/synthorg/persistence/sqlite/backend.py (1)
settings(412-418)
Comment on lines
+141
to
+149
| except (sqlite3.Error, aiosqlite.Error) as exc: | ||
| msg = f"Failed to delete setting {namespace}/{key}" | ||
| logger.exception( | ||
| SETTINGS_VALUE_DELETED, | ||
| namespace=namespace, | ||
| key=key, | ||
| error=str(exc), | ||
| ) | ||
| raise QueryError(msg) from exc |
There was a problem hiding this comment.
Same issue: SETTINGS_VALUE_DELETED used for error logging.
This event name suggests successful deletion, but it's logging a failure. The same applies to line 170 in delete_namespace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/persistence/sqlite/settings_repo.py` around lines 141 - 149, In
the error handlers that currently call logger.exception with
SETTINGS_VALUE_DELETED (used in the delete-setting and delete_namespace code
paths), replace that success-sounding event constant with an error-specific one
(e.g., SETTINGS_VALUE_DELETE_FAILED for the setting delete and
SETTINGS_NAMESPACE_DELETE_FAILED for delete_namespace) so the logged event
reflects failure; update the logger.exception calls in the delete_setting and
delete_namespace handlers to use the new error constants and keep the same
namespace/key/error fields and exception chaining.
Comment on lines
+7
to
+16
| from synthorg.settings.definitions import ( | ||
| backup, | ||
| budget, | ||
| company, | ||
| coordination, | ||
| memory, | ||
| observability, | ||
| providers, | ||
| security, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding __all__ for explicit public API.
The imports are used for their side effects (registration), but without __all__, some linters may flag these as unused. Adding __all__ clarifies the module's intent and prevents false-positive warnings.
Proposed fix
from synthorg.settings.definitions import (
backup,
budget,
company,
coordination,
memory,
observability,
providers,
security,
)
+
+__all__ = [
+ "backup",
+ "budget",
+ "company",
+ "coordination",
+ "memory",
+ "observability",
+ "providers",
+ "security",
+]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/settings/definitions/__init__.py` around lines 7 - 16, Add an
explicit __all__ list to this package initializer to declare the public API and
avoid linter false-positives for the imported modules (backup, budget, company,
coordination, memory, observability, providers, security); keep the existing
imports for side effects/registration and then define __all__ =
["backup","budget","company","coordination","memory","observability","providers","security"]
so tools and readers know these names are intentionally exported.
Comment on lines
+22
to
+23
| @pytest.mark.unit | ||
| class TestExtractFromConfig: |
There was a problem hiding this comment.
Missing pytest.mark.timeout(30) marker.
Per coding guidelines, tests should have a 30-second timeout.
🛠️ Proposed fix
-@pytest.mark.unit
+@pytest.mark.unit
+@pytest.mark.timeout(30)
class TestExtractFromConfig:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.unit | |
| class TestExtractFromConfig: | |
| `@pytest.mark.unit` | |
| `@pytest.mark.timeout`(30) | |
| class TestExtractFromConfig: |
🤖 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 22 - 23, Add the
pytest timeout marker to the TestExtractFromConfig test class so tests cannot
run longer than 30 seconds; modify the class decorator to include
pytest.mark.timeout(30) (i.e., change or augment the existing `@pytest.mark.unit`
on TestExtractFromConfig to also apply `@pytest.mark.timeout`(30) or combine
markers) to ensure the class-level timeout is enforced for all tests within.
Comment on lines
+14
to
+15
| @pytest.mark.unit | ||
| class TestDefinitionsLoading: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add timeout marker per coding guidelines.
The test file is missing the required 30-second timeout marker.
Proposed fix
import synthorg.settings.definitions # noqa: F401 — trigger registration
from synthorg.settings.enums import SettingNamespace, SettingType
from synthorg.settings.registry import SettingsRegistry, get_registry
+pytestmark = pytest.mark.timeout(30)
+
`@pytest.mark.unit`
class TestDefinitionsLoading:As per coding guidelines: "Set 30-second timeout per test".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.unit | |
| class TestDefinitionsLoading: | |
| import synthorg.settings.definitions # noqa: F401 — trigger registration | |
| from synthorg.settings.enums import SettingNamespace, SettingType | |
| from synthorg.settings.registry import SettingsRegistry, get_registry | |
| pytestmark = pytest.mark.timeout(30) | |
| `@pytest.mark.unit` | |
| class TestDefinitionsLoading: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/test_definitions.py` around lines 14 - 15, Add a
30-second pytest timeout marker to the test class: decorate
TestDefinitionsLoading with `@pytest.mark.timeout`(30) (either on its own line or
combined with the existing `@pytest.mark.unit` decorator) so every test in the
class uses a 30s timeout; ensure pytest is imported if not already.
Comment on lines
+18
to
+31
| def test_all_definitions_load_without_error(self) -> None: | ||
| """Importing the definitions package must not raise.""" | ||
| registry = get_registry() | ||
| assert registry.size > 0 | ||
|
|
||
| def test_no_duplicate_registrations(self) -> None: | ||
| """The global registry must have no duplicates. | ||
|
|
||
| Duplicates would have raised ValueError during import, | ||
| so if we get here the registry is duplicate-free. | ||
| Verify it has a reasonable number of definitions. | ||
| """ | ||
| registry = get_registry() | ||
| assert registry.size > 0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating near-duplicate tests.
test_all_definitions_load_without_error and test_no_duplicate_registrations have identical assertions (registry.size > 0). The docstring for test_no_duplicate_registrations explains the rationale (duplicates would raise during import), but both tests verify the same condition.
Consider either:
- Merging them into a single test with a more descriptive name, or
- Adding a distinct assertion to
test_no_duplicate_registrations(e.g., verifying unique composite keys explicitly).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/test_definitions.py` around lines 18 - 31, The two tests
test_all_definitions_load_without_error and test_no_duplicate_registrations are
redundant because both only assert registry.size > 0; either merge them into a
single test that names the intent clearly (e.g.,
test_definitions_load_and_unique) or enhance test_no_duplicate_registrations to
assert uniqueness explicitly: after calling get_registry(), iterate the registry
entries (or use registry.list / registry.items / registry._entries depending on
available API) and assert no duplicate keys/composite identifiers exist (for
example by comparing len(list) vs len(set(...)) of the unique identifier for
each definition). Ensure you reference the registry returned from get_registry()
and use its public accessors to check uniqueness rather than relying solely on
size.
Comment on lines
+1
to
+12
| """Unit tests for settings domain models.""" | ||
|
|
||
| import pytest | ||
| from pydantic import ValidationError | ||
|
|
||
| from synthorg.settings.enums import ( | ||
| SettingLevel, | ||
| SettingNamespace, | ||
| SettingSource, | ||
| SettingType, | ||
| ) | ||
| from synthorg.settings.models import SettingDefinition, SettingEntry, SettingValue |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add timeout marker per coding guidelines.
The test file is missing the required 30-second timeout marker.
Proposed fix
from synthorg.settings.models import SettingDefinition, SettingEntry, SettingValue
+pytestmark = pytest.mark.timeout(30)
+
`@pytest.mark.unit`
class TestSettingDefinition:As per coding guidelines: "Set 30-second timeout per test".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """Unit tests for settings domain models.""" | |
| import pytest | |
| from pydantic import ValidationError | |
| from synthorg.settings.enums import ( | |
| SettingLevel, | |
| SettingNamespace, | |
| SettingSource, | |
| SettingType, | |
| ) | |
| from synthorg.settings.models import SettingDefinition, SettingEntry, SettingValue | |
| """Unit tests for settings domain models.""" | |
| import pytest | |
| from pydantic import ValidationError | |
| from synthorg.settings.enums import ( | |
| SettingLevel, | |
| SettingNamespace, | |
| SettingSource, | |
| SettingType, | |
| ) | |
| from synthorg.settings.models import SettingDefinition, SettingEntry, SettingValue | |
| pytestmark = pytest.mark.timeout(30) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/test_models.py` around lines 1 - 12, Add a 30-second
timeout marker to the test module by defining a module-level pytest mark (e.g.,
add "pytestmark = pytest.mark.timeout(30)") near the top of
tests/unit/settings/test_models.py so all tests in this file (which reference
imports like pytest, ValidationError, and models
SettingDefinition/SettingEntry/SettingValue) inherit the 30s timeout per the
coding guidelines.
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive, database-backed settings persistence layer, which is a significant and well-executed feature. It enables runtime configuration changes through a new REST API, with a 4-layer resolution chain (DB > env > YAML > code defaults). The implementation includes a robust settings service with caching, type validation, and encryption for sensitive values using Fernet. The database migration is designed to be crash-safe, and the new API endpoints are well-guarded and handle errors gracefully. The addition of extensive unit tests covering the new functionality, including complex scenarios like migrations and resolution logic, is commendable. My feedback focuses on minor performance optimizations in the caching logic.
| # Cache only non-sensitive values to avoid holding | ||
| # plaintext secrets in memory. | ||
| if not definition.sensitive: | ||
| self._cache = {**self._cache, cache_key: setting_value} |
Contributor
There was a problem hiding this comment.
For performance, it's better to update the cache in-place rather than recreating the dictionary on each write. Using dictionary unpacking ({**self._cache, ...}) creates a new dictionary object, which can lead to unnecessary memory allocations and garbage collector overhead in a high-traffic scenario.
Suggested change
| self._cache = {**self._cache, cache_key: setting_value} | |
| self._cache[cache_key] = setting_value |
src/synthorg/settings/service.py
Outdated
|
|
||
| # Invalidate cache | ||
| cache_key = (namespace, key) | ||
| self._cache = {k: v for k, v in self._cache.items() if k != cache_key} |
Contributor
There was a problem hiding this comment.
Rebuilding the dictionary with a comprehension to remove a single key is inefficient, especially as the cache size grows. A more direct and performant approach is to use dict.pop(). This same improvement can be applied to the cache invalidation in the delete method on line 527.
Suggested change
| self._cache = {k: v for k, v in self._cache.items() if k != cache_key} | |
| self._cache.pop(cache_key, None) |
9 tasks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #495 +/- ##
==========================================
- Coverage 93.37% 93.27% -0.11%
==========================================
Files 480 500 +20
Lines 23250 23915 +665
Branches 2217 2277 +60
==========================================
+ Hits 21710 22306 +596
- Misses 1216 1269 +53
- Partials 324 340 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/persistence/sqlite/backend.py (1)
411-473: 🧹 Nitpick | 🔵 TrivialRoute the
_systemcompatibility helpers throughSQLiteSettingsRepository.You already expose
self.settingshere, butget_setting()andset_setting()still duplicate the SQL, namespace convention, and timestamp behavior themselves. That leaves the backend with two sources of truth for the same table and guarantees drift the next time settings persistence changes. A thin wrapper aroundself.settingskeeps the legacy API without duplicating schema logic.As per coding guidelines,
Code smells such as violations of SOLID, DRY, KISS, or YAGNI principles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/persistence/sqlite/backend.py` around lines 411 - 473, get_setting and set_setting duplicate SQL, namespace and timestamp logic; instead delegate to the SQLiteSettingsRepository exposed by the settings property. Replace the SQL and direct DB access in get_setting and set_setting with calls into self.settings (e.g., use methods like SQLiteSettingsRepository.get(key) and SQLiteSettingsRepository.set(key, value) or the repository's equivalent names), keep the same PersistenceConnectionError behavior by using self._require_connected or the existing connection check, and preserve exception translation (raise QueryError) around the repository calls so the public API and error semantics remain unchanged.
♻️ Duplicate comments (2)
tests/unit/api/fakes.py (1)
549-553: 🧹 Nitpick | 🔵 Trivial
delete_namespacestill rebuilds the dict on each iteration.This is the same previously flagged cleanup item; behavior is correct, but a single rebuild is cleaner and cheaper.
♻️ Suggested simplification
async def delete_namespace(self, namespace: str) -> int: keys = [k for k in self._store if k[0] == namespace] - for k in keys: - self._store = {kk: vv for kk, vv in self._store.items() if kk != k} + self._store = {k: v for k, v in self._store.items() if k[0] != namespace} return len(keys)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/fakes.py` around lines 549 - 553, The delete_namespace implementation repeatedly rebuilds self._store inside the loop; instead compute the list of matching keys once (already in keys) and then rebuild self._store a single time by filtering out entries whose key tuple first element equals the given namespace (use the same namespace check used to collect keys), finally return len(keys); update the function delete_namespace to perform one comprehension pass rather than one per key.tests/unit/persistence/sqlite/test_migrations_v9.py (1)
150-172:⚠️ Potential issue | 🟠 MajorAuthoritative-source behavior is still not proven in the dual-table crash test.
settings_oldandsettingscurrently contain the same value, so this test can pass even if source selection is wrong. Make the values differ and assert the exact migrated tuple.✅ Stronger test assertion example
await memory_db.execute( "INSERT INTO settings_old (key, value) VALUES ('k1', 'old_val')" ) @@ await memory_db.execute( "INSERT INTO settings VALUES " - "('_system', 'k1', 'old_val', '2026-01-01T00:00:00+00:00')" + "('_system', 'k1', 'new_val', '2026-01-01T00:00:00+00:00')" ) @@ - cursor = await memory_db.execute("SELECT namespace, key, value FROM settings") + cursor = await memory_db.execute( + "SELECT namespace, key, value FROM settings ORDER BY key" + ) rows = list(await cursor.fetchall()) - assert len(rows) == 1 - assert rows[0][0] == "_system" + assert [(r[0], r[1], r[2]) for r in rows] == [("_system", "k1", "old_val")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/persistence/sqlite/test_migrations_v9.py` around lines 150 - 172, The test currently seeds settings_old and settings with the same value so it can't prove authoritative-source selection; change the inserted values so settings_old and settings differ (e.g. settings_old has "old_val" and settings has "new_val"), run _apply_v9, then assert the settings table contains the exact tuple migrated from settings_old (namespace "_system", key "k1", value matching settings_old) to prove the migration used the authoritative source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Line 67: Replace the floating constraint for Bandit in the dev dependency
group with an exact pinned version; locate the entry "bandit>=1.9.4" in
pyproject.toml and change it to "bandit==1.9.4" (or another specific patch
version if you intend to pin to a different precise release) so all dev
dependencies use exact == pins for reproducible builds.
In `@src/synthorg/persistence/sqlite/migrations.py`:
- Around line 483-515: The current copy logic can treat an existing v9-shaped
"settings" table as a v5 source and clobber real data; update the migration to
detect when the "settings" table already has the v9 schema and, in that case,
skip the copy/no-op instead of using it as source. Concretely: when choosing
source (the code using has_old/has_original, _V9_VALID_SOURCES and
_V9_COPY_ROWS), add a quick schema-detection check (e.g. implement
_is_v9_settings_schema(db) or reuse an existing helper) and if source ==
"settings" and that check returns true, do not set source / do not execute the
_V9_COPY_ROWS copy; proceed directly to renaming/cleanup. Also add a regression
test that bootstraps user_version=8 with a v9-shaped settings table and no
settings_old, runs the migration, and asserts the table/rows and updated_at are
preserved.
In `@src/synthorg/settings/models.py`:
- Around line 44-47: The Field.default value is not being validated by
_check_cross_field_constraints(), so invalid defaults (wrong type, out-of-bounds
numerics, invalid enum members, or pattern mismatches) can slip through; update
_check_cross_field_constraints() to run the same type/bounds/enum/pattern
validation that you use for incoming values against Field.default (the default:
str | None = Field(...) property), reusing the existing validation helpers or
the same code paths that validate user values, and raise/record the same
validation error if the default fails type, numeric range, enum membership, or
regex checks; apply the same change to the other model-validation block(s) that
perform cross-field checks referenced in the file so all default fields are
validated consistently.
In `@src/synthorg/settings/service.py`:
- Around line 605-613: In the except block that catches Exception (around the
notification failure handling), update the logger.warning call for
SETTINGS_NOTIFICATION_FAILED (the block using namespace, key, error=str(exc),
error_type=type(exc).__name__) to include exc_info=True so the full stack trace
is logged for debugging while preserving the current behavior that notification
failures don't break settings writes.
In `@tests/unit/api/controllers/test_settings.py`:
- Around line 44-51: Consolidate the repeated 404 tests by parameterizing them:
replace individual tests like test_get_unknown_setting_returns_404 and the
similar tests at the other ranges with a single parameterized test (e.g.,
test_unknown_resource_returns_404) that uses `@pytest.mark.parametrize` over the
endpoint strings ("/api/v1/settings/budget/nonexistent",
"/api/v1/settings/nonexistent_ns", "/api/v1/settings/_schema/nonexistent_ns",
etc.), accept parameters (self, test_client: TestClient[Any], endpoint: str),
call test_client.get(endpoint, headers=_HEADERS) and assert resp.status_code ==
404; update or remove the original test functions
(test_get_unknown_setting_returns_404 and the ones referenced at 73-81, 99-106,
151-156, 158-165) to avoid duplication.
---
Outside diff comments:
In `@src/synthorg/persistence/sqlite/backend.py`:
- Around line 411-473: get_setting and set_setting duplicate SQL, namespace and
timestamp logic; instead delegate to the SQLiteSettingsRepository exposed by the
settings property. Replace the SQL and direct DB access in get_setting and
set_setting with calls into self.settings (e.g., use methods like
SQLiteSettingsRepository.get(key) and SQLiteSettingsRepository.set(key, value)
or the repository's equivalent names), keep the same PersistenceConnectionError
behavior by using self._require_connected or the existing connection check, and
preserve exception translation (raise QueryError) around the repository calls so
the public API and error semantics remain unchanged.
---
Duplicate comments:
In `@tests/unit/api/fakes.py`:
- Around line 549-553: The delete_namespace implementation repeatedly rebuilds
self._store inside the loop; instead compute the list of matching keys once
(already in keys) and then rebuild self._store a single time by filtering out
entries whose key tuple first element equals the given namespace (use the same
namespace check used to collect keys), finally return len(keys); update the
function delete_namespace to perform one comprehension pass rather than one per
key.
In `@tests/unit/persistence/sqlite/test_migrations_v9.py`:
- Around line 150-172: The test currently seeds settings_old and settings with
the same value so it can't prove authoritative-source selection; change the
inserted values so settings_old and settings differ (e.g. settings_old has
"old_val" and settings has "new_val"), run _apply_v9, then assert the settings
table contains the exact tuple migrated from settings_old (namespace "_system",
key "k1", value matching settings_old) to prove the migration used the
authoritative source.
🪄 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: 6c5b6973-6bd7-4630-a8e4-0cddcaf39a41
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
CLAUDE.mddocs/design/memory.mddocs/design/operations.mdpyproject.tomlsrc/synthorg/api/app.pysrc/synthorg/observability/events/settings.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/persistence/sqlite/migrations.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/encryption.pysrc/synthorg/settings/models.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/service.pytests/unit/api/controllers/test_settings.pytests/unit/api/fakes.pytests/unit/persistence/sqlite/test_migrations_v9.pytests/unit/settings/test_service.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 Sandbox
- GitHub Check: Build Web
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update the relevant
docs/design/page to reflect approved deviations from the spec
Files:
docs/design/operations.mddocs/design/memory.md
**/*.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) per PEP 758 — ruff enforces this on Python 3.14
Add type hints to all public functions and classes; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules
Keep line length to 88 characters (ruff enforced)
Files:
tests/unit/persistence/sqlite/test_migrations_v9.pysrc/synthorg/observability/events/settings.pysrc/synthorg/settings/models.pytests/unit/settings/test_service.pysrc/synthorg/api/app.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/persistence/sqlite/migrations.pytests/unit/api/controllers/test_settings.pysrc/synthorg/settings/encryption.pysrc/synthorg/settings/service.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/registry.pytests/unit/api/fakes.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
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set 30-second timeout per test
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python (@given+@settings); useciprofile (200 examples, default) anddevprofile (1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
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/persistence/sqlite/test_migrations_v9.pytests/unit/settings/test_service.pytests/unit/api/controllers/test_settings.pytests/unit/api/fakes.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Create new objects for immutability instead of mutating existing ones; 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/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 (BaseModel,model_validator,computed_field,ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; useNotBlankStrfor all identifier/name fields
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate at system boundaries (user input, external APIs, config files)
Every module with business logic must importfrom synthorg.observability import get_loggerand createlogger = get_logger(__name__); never useimport loggingorlogging.getLogger()orprint()in application code
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget); import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Pure data models, enums, and re-e...
Files:
src/synthorg/observability/events/settings.pysrc/synthorg/settings/models.pysrc/synthorg/api/app.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/persistence/sqlite/migrations.pysrc/synthorg/settings/encryption.pysrc/synthorg/settings/service.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/registry.py
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: Use Pydantic v2 inpyproject.tomlwithmem0ai(Mem0 memory backend) pinned as a required dependency with==version specifier
Pin all dependency versions using==inpyproject.toml; organize into groups:test(pytest + plugins, hypothesis),dev(includes test + ruff, mypy, pre-commit, commitizen, pip-audit)
Files:
pyproject.toml
🧠 Learnings (20)
📚 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/observability/events/settings.pyCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/settings.pyCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to pyproject.toml : Pin all dependency versions using `==` in `pyproject.toml`; organize into groups: `test` (pytest + plugins, hypothesis), `dev` (includes test + ruff, mypy, pre-commit, commitizen, pip-audit)
Applied to files:
pyproject.tomlCLAUDE.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 pyproject.toml : Dependencies: all versions use == in pyproject.toml. Groups: test (pytest + plugins, hypothesis), dev (includes test + ruff, mypy, pre-commit, commitizen, pip-audit). Required: mem0ai (Mem0 memory backend — the default and currently only backend). Install: uv sync installs everything (dev group is default).
Applied to files:
pyproject.tomlCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using `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/models.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`); use `computed_field` for derived values instead of storing redundant fields; use `NotBlankStr` for all identifier/name fields
Applied to files:
src/synthorg/settings/models.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.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/models.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 : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.mdsrc/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 : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic must import `from synthorg.observability import get_logger` and create `logger = get_logger(__name__)`; never use `import logging` or `logging.getLogger()` or `print()` in application code
Applied to files:
CLAUDE.mdsrc/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
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 src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.mdsrc/synthorg/settings/encryption.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 state transitions must log at INFO.
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 src/synthorg/**/*.py : DEBUG logging for object creation, internal flow, entry/exit of key functions.
Applied to files:
CLAUDE.mdsrc/synthorg/settings/config_bridge.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Pure data models, enums, and re-exports do NOT need logging
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to pyproject.toml : Use Pydantic v2 in `pyproject.toml` with `mem0ai` (Mem0 memory backend) pinned as a required dependency with `==` version specifier
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to web/package.json : Web dashboard requires Node.js 20+; dependencies in `web/package.json` (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc)
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 web/** : Web dashboard: Node.js 20+, dependencies in web/package.json (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc).
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 docker/{Dockerfile*,compose.yml} : Docker: Backend uses 3-stage build (builder → setup → distroless runtime), Chainguard Python, non-root (UID 65532), CIS-hardened. Web uses nginxinc/nginx-unprivileged, Vue 3 SPA with PrimeVue + Tailwind CSS, SPA routing, API/WebSocket proxy to backend.
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: Property-based testing: Python uses Hypothesis (given + settings). Hypothesis profiles: ci (200 examples, default) and dev (1000 examples), controlled via HYPOTHESIS_PROFILE env var. Run dev profile: HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties.
Applied to files:
CLAUDE.md
🧬 Code graph analysis (8)
tests/unit/persistence/sqlite/test_migrations_v9.py (1)
src/synthorg/persistence/sqlite/migrations.py (3)
get_user_version(283-287)run_migrations(533-601)set_user_version(290-309)
src/synthorg/settings/models.py (1)
src/synthorg/settings/enums.py (4)
SettingLevel(38-46)SettingNamespace(6-20)SettingSource(49-60)SettingType(23-35)
src/synthorg/api/app.py (2)
src/synthorg/settings/service.py (1)
SettingsService(151-613)src/synthorg/api/state.py (1)
settings_service(225-227)
src/synthorg/persistence/sqlite/settings_repo.py (3)
src/synthorg/persistence/sqlite/backend.py (1)
settings(412-418)src/synthorg/persistence/protocol.py (1)
settings(165-167)tests/unit/persistence/test_protocol.py (3)
settings(358-359)get_namespace(268-269)get_all(271-272)
tests/unit/api/controllers/test_settings.py (5)
tests/unit/api/conftest.py (2)
make_auth_headers(100-126)test_client(182-213)src/synthorg/persistence/sqlite/settings_repo.py (2)
get(34-58)delete(131-159)src/synthorg/settings/registry.py (1)
get(52-62)src/synthorg/settings/service.py (2)
get(188-256)delete(525-551)src/synthorg/persistence/repositories.py (13)
get(62-74)get(221-233)get(349-361)get(429-441)get(554-566)get(621-633)delete(98-110)delete(263-275)delete(399-411)delete(471-483)delete(586-598)delete(649-661)delete(740-757)
src/synthorg/settings/service.py (6)
src/synthorg/persistence/protocol.py (1)
settings(165-167)src/synthorg/settings/config_bridge.py (1)
extract_from_config(13-42)src/synthorg/settings/enums.py (2)
SettingSource(49-60)SettingType(23-35)src/synthorg/settings/models.py (3)
SettingDefinition(16-117)SettingEntry(143-164)SettingValue(120-140)src/synthorg/persistence/repositories.py (17)
SettingsRepository(665-771)get(62-74)get(221-233)get(349-361)get(429-441)get(554-566)get(621-633)get(672-689)get_namespace(691-706)get_all(708-718)delete(98-110)delete(263-275)delete(399-411)delete(471-483)delete(586-598)delete(649-661)delete(740-757)src/synthorg/settings/encryption.py (3)
SettingsEncryptor(21-125)decrypt(56-88)encrypt(31-54)
src/synthorg/persistence/sqlite/backend.py (2)
src/synthorg/persistence/sqlite/settings_repo.py (1)
SQLiteSettingsRepository(21-183)src/synthorg/persistence/protocol.py (3)
settings(165-167)get_setting(169-181)set_setting(183-195)
tests/unit/api/fakes.py (4)
src/synthorg/persistence/sqlite/backend.py (3)
settings(412-418)get_setting(420-444)set_setting(446-473)src/synthorg/persistence/protocol.py (3)
settings(165-167)get_setting(169-181)set_setting(183-195)src/synthorg/persistence/sqlite/settings_repo.py (4)
get(34-58)get_namespace(60-80)get_all(82-97)delete(131-159)src/synthorg/settings/service.py (4)
get(188-256)get_namespace(283-308)get_all(310-333)delete(525-551)
🔇 Additional comments (20)
pyproject.toml (1)
19-19:cryptographyis correctly pinned as a runtime dependency.Good addition for the Fernet-based settings encryption path.
CLAUDE.md (1)
116-130: Settings-related documentation updates are aligned and helpful.The package-structure, logging-event, and required-dependencies updates are consistent with the new settings subsystem.
Also applies to: 196-196, 289-290
tests/unit/api/fakes.py (1)
425-428: Nice clarification of namespaced vs legacy settings storage.The comment and
settingsproperty make test intent clearer.Also applies to: 503-505
docs/design/operations.md (1)
1014-1014: Good spec alignment for the new Settings UI/API capability.This update clearly captures the DB-backed resolution chain and runtime editability.
docs/design/memory.md (1)
394-395: Persistence design doc updates are consistent with V9/settings integration.The protocol and entities sections now correctly include settings persistence as a first-class concern.
Also applies to: 452-452
src/synthorg/settings/config_bridge.py (1)
13-42: Clean and guideline-aligned config-path bridge implementation.Typed API, Google-style docstring, and structured debug logging with
SETTINGS_CONFIG_PATH_MISSall look good.src/synthorg/api/app.py (1)
445-446: SettingsService wiring through app factory and AppState looks correct.The optional injection path and startup warning messaging are coherent with existing 503-on-missing-service behavior.
Also applies to: 470-471, 506-512, 541-542
tests/unit/api/controllers/test_settings.py (3)
10-11: Import-time auth header generation may cause test isolation issues.This was previously flagged.
make_auth_headersis called at module import time rather than per-test or in a fixture. Consider moving these to module-scoped fixtures for better test isolation and control.
83-97: Consider verifying post-delete fallback behavior.The test confirms the delete succeeds but doesn't verify that the setting falls back to the next resolution source (env/YAML/default). Adding a follow-up GET assertion would strengthen the test and validate the core resolution chain behavior.
14-165: LGTM!The test suite provides good coverage of the Settings API controller endpoints including:
- CRUD operations (list, get, update, delete)
- Schema endpoints
- Access control (observer read-only permissions)
- Error handling (404 for unknown resources, 422 for validation failures)
The test organization is clear and follows the
@pytest.mark.unitconvention.src/synthorg/settings/service.py (10)
141-148: Consider masking numeric values in range errors for sensitive settings.For consistency with other validators that mask sensitive values in error messages, range check errors could also mask the actual value for sensitive settings. While the security risk is lower since only numeric values are exposed, it would maintain consistency across all validators.
151-186: LGTM!The class initialization is well-documented with a clear docstring explaining the resolution order and caching behavior. The constructor uses keyword-only arguments and proper type hints. Cache updates follow the immutability pattern by creating new dict instances.
188-256: LGTM!The
get()method correctly implements the 4-layer resolution chain with proper handling of:
- Registry validation with
SettingNotFoundError- Cache checking (skipped for sensitive values)
- DB lookup with encryption handling that raises
SettingsEncryptionErrorwhen no encryptor is available for sensitive values (addressing the previously flagged ciphertext leak issue)- Fallback chain (env > YAML > default)
258-281: LGTM!The
get_entry()method correctly combines the resolved value with its definition and masks sensitive values for display. The assertion on line 274 is appropriate as an internal invariant sinceget()guarantees the definition exists.
283-333: LGTM!The batch methods
get_namespace()andget_all()efficiently avoid N+1 queries by pre-fetching all DB values in a single query before resolving each setting. The implementation correctly delegates to_resolve_with_db_lookup()for consistent resolution logic.
335-374: LGTM!The
_resolve_fallback()method correctly implements the env > YAML > code default resolution chain with appropriate DEBUG-level logging for each source. The fallback to empty string whendefaultisNoneis a reasonable sentinel value.
376-437: LGTM!The
_resolve_with_db_lookup()helper correctly handles batch resolution with graceful degradation for encryption issues:
- Returns masked value (not ciphertext) when encryptor is missing
- Returns masked value when decryption fails
- This prevents a single bad setting from failing the entire batch while avoiding ciphertext exposure
445-523: LGTM!The
set()method implements a complete write operation with:
- Registry validation
- Type and constraint validation
- Encryption for sensitive settings
- Persistence with timestamp
- Cache invalidation
- INFO-level logging of state transition
- Change notification publishing
- Masked response for sensitive values
525-551: LGTM!The
delete()method correctly implements idempotent deletion with registry validation, cache invalidation, logging, and change notification. The repository's return value (whether something was actually deleted) is intentionally ignored to maintain idempotent behavior.
553-565: LGTM!Simple and clean delegation to the registry for schema introspection with optional namespace filtering.
Add a full settings persistence layer enabling runtime-editable configuration via REST API with resolution order DB > env > YAML > code defaults. Core components: - SettingsRepository protocol + SQLiteSettingsRepository implementation - V9 migration: evolve settings table to (namespace, key) composite PK with crash-safe rename pattern, preserving existing KV data in _system namespace - SettingsService with 4-layer resolution chain, invalidate-on-write cache, type validation, and change notifications via message bus - SettingsRegistry for metadata-driven schema introspection - Fernet encryption for sensitive settings (SYNTHORG_SETTINGS_KEY) - Config bridge for lazy YAML extraction via dotted paths - 8 namespace definition modules (budget, security, providers, company, memory, coordination, observability, backup) - REST controller with GET/PUT/DELETE + schema endpoints - 93 new unit tests across models, registry, repo, encryption, service, config bridge, and API controller Closes #450
Security fixes: - Mask sensitive values in validation error messages (#2) - Sanitize SettingsEncryptionError in API response (#3) - Don't cache decrypted plaintext secrets (#4) - Validate namespace against SettingNamespace enum at controller (#8) - Error on empty SYNTHORG_SETTINGS_KEY instead of silently ignoring (#9) Persistence fixes: - V9 migration: prefer settings_old as copy source when both exist (#1) - Remove redundant idx_settings_namespace index (PK covers it) (#15) - Fix updated_at format consistency (ISO 8601 with timezone) (#17) - Align get_setting/set_setting types with protocol (NotBlankStr) (#10) Performance: - Use batch repo methods in get_all/get_namespace (eliminate N+1) (#6) Code quality: - Fix Any type to SettingsService on create_app parameter (#5) - Add SETTINGS_NOTIFICATION_FAILED + SETTINGS_FETCH_FAILED events (#7,#16) - Fix wrong event constants in repo error paths and notification (#7,#16) - Remove duplicate SettingNotFoundError guard in get_entry (#21) - Log SETTINGS_VALUE_RESOLVED for all resolution sources (#22) Tests: - Add V9 migration crash-safety tests (#11) - Add tests for sensitive read without encryptor (#12) - Add tests for get_all/get_namespace service methods (#13) - Add tests for notification exception handling (#14) - Use pydantic.ValidationError instead of broad Exception (#18) - Add INTEGER, JSON, and sensitive-masking validation tests (#19) - Use actual Unicode characters in encryption roundtrip test (#20)
Critical fixes: - Guard against ciphertext leak on read when encryptor absent - Replace assert with if/raise in V9 migration (python -O safe) - Add CLAUDE.md docs for settings package, events, cryptography dep - Create follow-up issue #497 for criterion 8 (config migration) Major fixes: - Fix logger.exception outside except block in encryption.py - Add SETTINGS_SET_FAILED/DELETE_FAILED error event constants - Fix JSON validation error leaking sensitive values - Add batch decryption failure handling (mask instead of crash) - Add logger + MappingProxyType to SettingsRegistry - Add exception details to _publish_change warning - Add logging before SettingNotFoundError raises (3 locations) - Add logging to config_bridge AttributeError catch - Refactor get()/set() under 50 lines via helpers - Fix naive timestamp in backend set_setting - Add settings_service to degraded-services startup warning - Update design spec (memory.md entities, operations.md) Medium fixes: - Remove dead v8_db fixture, tighten assertion to == 1 - Add spec=SettingsRepository to mock_repo - Add model_validator for cross-field constraints - Narrow except Exception to specific crypto error types - Add tests: ciphertext leak guard, batch decrypt, validator_pattern, invalid namespace 404
Fixes SECT curve subgroup attack vulnerability. Not exploitable in our Fernet-only usage but clears the Dependency Review advisory.
- Pin bandit==1.9.4 (was floating >=1.9.4) - V9 migration: detect v9 schema to skip copy/rename when settings table is already migrated (prevents data clobber) - Validate SettingDefinition.default against type/range/enum/pattern at construction time via model_validator - Add exc_info=True to notification failure warning for stack traces - Consolidate 404 GET tests with @pytest.mark.parametrize - Delegate backend get_setting/set_setting to SettingsRepository (eliminates duplicate SQL, fixes timestamp inconsistency) - Fix fakes delete_namespace to rebuild dict once instead of per-key - Differentiate values in crash-with-both-tables test to prove authoritative source selection; add v9-schema regression test
05a6f40 to
a948ec2
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (8)
tests/unit/settings/test_models.py (1)
1-16:⚠️ Potential issue | 🟡 MinorAdd module-level 30s timeout marker for this test file.
@pytest.mark.unitis present, but the timeout marker convention is still missing.Proposed fix
import pytest from pydantic import ValidationError from synthorg.settings.enums import ( @@ ) from synthorg.settings.models import SettingDefinition, SettingEntry, SettingValue +pytestmark = pytest.mark.timeout(30) + `@pytest.mark.unit` class TestSettingDefinition:Based on learnings: Applies to tests/**/*.py : Test timeout: 30 seconds per test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/test_models.py` around lines 1 - 16, The file is missing the module-level 30s timeout marker; add a module-level pytestmark that includes the existing pytest.mark.unit plus pytest.mark.timeout(30) (e.g. pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)]) near the top of the module so tests in this file use the 30-second timeout.tests/unit/persistence/sqlite/test_migrations_v9.py (1)
171-176:⚠️ Potential issue | 🟠 MajorStrengthen crash-safety assertion to include migrated value.
The current check can still pass if the row value is wrong. Assert the full tuple so precedence behavior is actually validated.
✅ Proposed assertion hardening
- cursor = await memory_db.execute("SELECT namespace, key, value FROM settings") + cursor = await memory_db.execute( + "SELECT namespace, key, value FROM settings ORDER BY namespace, key" + ) rows = list(await cursor.fetchall()) assert len(rows) == 1 assert rows[0][0] == "_system" assert rows[0][1] == "k1" + assert rows[0][2] == "new_val"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/persistence/sqlite/test_migrations_v9.py` around lines 171 - 176, The test currently only asserts namespace and key, which misses validating the migrated value; update the assertion after fetching rows (variables cursor and rows) to assert the full tuple equals ("_system", "k1", "<expected_value>") so the test validates the migrated value from the settings table in memory_db and ensures correct precedence behavior.src/synthorg/settings/definitions/__init__.py (1)
7-16: 🧹 Nitpick | 🔵 TrivialConsider explicit
__all__for intentional re-exports.This clarifies public API intent and avoids false-positive “unused import” tooling noise for side-effect imports.
🧩 Proposed addition
from synthorg.settings.definitions import ( backup, budget, company, coordination, memory, observability, providers, security, ) + +__all__ = [ + "backup", + "budget", + "company", + "coordination", + "memory", + "observability", + "providers", + "security", +]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/settings/definitions/__init__.py` around lines 7 - 16, Add an explicit __all__ to clarify the package's public API and prevent tooling from flagging the module-level imports as unused: keep the existing imports (backup, budget, company, coordination, memory, observability, providers, security) and add a top-level __all__ = ['backup','budget','company','coordination','memory','observability','providers','security'] so these names are intentionally re-exported from this package.tests/unit/settings/test_definitions.py (2)
14-15:⚠️ Potential issue | 🟡 MinorAdd a 30-second timeout marker to this test class.
Line [14] currently applies only
@pytest.mark.unit; add a timeout marker so hangs/regressions fail fast.🧪 Proposed fix
+@pytest.mark.timeout(30) `@pytest.mark.unit` class TestDefinitionsLoading:Based on learnings: "Applies to tests/**/*.py : Test timeout: 30 seconds per test."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/test_definitions.py` around lines 14 - 15, Add a 30‑second pytest timeout marker to the TestDefinitionsLoading test class by applying `@pytest.mark.timeout`(30) alongside the existing `@pytest.mark.unit` decorator (i.e., stack the timeout decorator above or below `@pytest.mark.unit` on the class definition) so each test in TestDefinitionsLoading fails fast if it exceeds 30 seconds; ensure pytest is imported so the marker is available.
18-31: 🧹 Nitpick | 🔵 TrivialConsolidate the first two tests to avoid duplicated assertions.
Line [18] through Line [31] currently has two tests validating the same condition (
registry.size > 0), which weakens intent clarity.♻️ Proposed refactor
- def test_all_definitions_load_without_error(self) -> None: - """Importing the definitions package must not raise.""" - registry = get_registry() - assert registry.size > 0 - - def test_no_duplicate_registrations(self) -> None: - """The global registry must have no duplicates. - - Duplicates would have raised ValueError during import, - so if we get here the registry is duplicate-free. - Verify it has a reasonable number of definitions. - """ + def test_definitions_load_and_registry_is_populated(self) -> None: + """Definitions import should succeed and populate the global registry.""" registry = get_registry() assert registry.size > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/test_definitions.py` around lines 18 - 31, Combine the two duplicate tests test_all_definitions_load_without_error and test_no_duplicate_registrations into a single test (e.g., test_definitions_registry_valid) that calls get_registry() once and asserts registry.size > 0; update the docstring to state both intents (“importing definitions must not raise and the registry must contain entries; duplicates would have raised during import”). Replace the two existing test functions with this single consolidated test and keep the assertion against registry.size to avoid duplicated assertions.tests/unit/settings/test_config_bridge.py (1)
22-23:⚠️ Potential issue | 🟡 MinorAdd class-level timeout marker to enforce test runtime bounds.
Line 22 has
@pytest.mark.unit, but no timeout marker is present for this class.Based on learnings, tests should use a 30-second timeout per test to catch performance regressions early.
🤖 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 22 - 23, Add a class-level timeout marker to the test class TestExtractFromConfig by annotating it with `@pytest.mark.timeout`(30) alongside the existing `@pytest.mark.unit` so each test in the class has a 30-second limit; ensure pytest-timeout is available in tests and place the marker directly above the class definition near the existing `@pytest.mark.unit` decorator.tests/unit/api/controllers/test_settings.py (1)
10-11:⚠️ Potential issue | 🟡 MinorMove auth header generation into fixtures to avoid import-time coupling.
make_auth_headers()is still executed at module import, which can hurt isolation and increase flake risk if auth/token setup changes.♻️ Suggested adjustment
-_HEADERS = make_auth_headers("ceo") -_OBSERVER_HEADERS = make_auth_headers("observer") +@pytest.fixture(scope="module") +def ceo_headers() -> dict[str, str]: + return make_auth_headers("ceo") + + +@pytest.fixture(scope="module") +def observer_headers() -> dict[str, str]: + return make_auth_headers("observer")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/controllers/test_settings.py` around lines 10 - 11, The module-level calls to make_auth_headers that assign _HEADERS and _OBSERVER_HEADERS run at import-time and should be moved into fixtures; remove the top-level assignments (_HEADERS, _OBSERVER_HEADERS) from tests/unit/api/controllers/test_settings.py and instead create pytest fixtures (e.g., auth_headers_ceo and auth_headers_observer) that call make_auth_headers("ceo") and make_auth_headers("observer") at runtime, register them in the test file or conftest.py, and update each test to accept the appropriate fixture name so headers are generated per-test rather than at import.src/synthorg/settings/service.py (1)
141-148:⚠️ Potential issue | 🟡 MinorRange-validation errors still expose raw numeric values for sensitive settings.
_check_range()formats the actual numeric value even when the definition is sensitive, which is inconsistent with masking used elsewhere in this module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/settings/service.py` around lines 141 - 148, The range-check currently includes the raw numeric value in SettingValidationError messages even when the setting is sensitive; update _check_range(definition: SettingDefinition, value: float) to mask the displayed value when definition.sensitive is True by using the same masking approach used elsewhere in this module (use the module's existing mask/obfuscation helper or logic) so that the error messages reference the masked value instead of the raw numeric; preserve the descriptive context (min/max and that the value is below/above) while only changing how the offending value is rendered.
🤖 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/persistence/sqlite/backend.py`:
- Around line 424-437: Replace direct calls to
_require_connected(self._settings, "settings") in get_setting and set_setting
with the canonical accessor self.settings so the centralized connection-check
logic is used; specifically, in get_setting assign repo = self.settings (then
call repo.get(... ) and return as before) and in set_setting assign repo =
self.settings (then use repo.set/ upsert logic and updated_at as currently
implemented), removing the explicit _require_connected(...) calls.
In `@src/synthorg/settings/definitions/memory.py`:
- Around line 22-46: The MEMORY SettingDefinition entries for key
"default_level" (SettingDefinition named default_level) and
"consolidation_interval" are missing enum members and one yaml_path is
incorrect: update the enum_values on default_level to include all MemoryLevel
members (add "project" alongside "none","session","persistent"), update
enum_values on consolidation_interval to include "never" alongside
"hourly","daily","weekly", and change yaml_path on the default_level
SettingDefinition from "memory.default_level" to "memory.level" so it matches
CompanyMemoryConfig.level for proper YAML deserialization.
In `@src/synthorg/settings/models.py`:
- Around line 63-66: The enum_values Field currently uses tuple[str, ...] and
allows blank/whitespace entries; change its annotation to tuple[NotBlankStr,
...] and import NotBlankStr from core.types, update the Field declaration to use
that typed tuple (keeping default=() and the description), and ensure any
necessary typing imports remain; this enforces the "not blank" constraint for
each enum literal referenced by the enum_values field.
- Around line 93-112: The validation currently allows non-finite floats
(nan/inf) to be used for min_value, max_value, and numeric defaults; update the
validation in the Settings model (the block checking self.min_value,
self.max_value and the block that validates default values) to explicitly reject
non-finite numbers by using math.isfinite (or equivalent) for any numeric
constraint or numeric default when self.type is SettingType.FLOAT or
SettingType.INTEGER and raise a ValueError with a clear message (e.g. "min_value
must be finite", "default value must be finite") referencing the offending
attribute (self.min_value, self.max_value, self.default). Ensure these checks
run before min/max comparisons so nan/inf cannot bypass range checks.
In `@src/synthorg/settings/service.py`:
- Around line 223-236: Wrap the call to self._encryptor.decrypt(raw_value) in a
try/except that catches Exception, logs SETTINGS_ENCRYPTION_ERROR (include
namespace and key and a failure reason like "decrypt_failed" and the exception
message/str(err)), then re-raise the exception; keep the rest of the flow that
constructs SettingValue unchanged (refer to self._encryptor.decrypt,
SETTINGS_ENCRYPTION_ERROR, SettingsEncryptionError and SettingValue to find the
exact location).
In `@tests/unit/settings/test_encryption.py`:
- Around line 22-36: Consolidate the three duplicate tests into a single
parametrized test: replace test_roundtrip, test_roundtrip_empty_string, and
test_roundtrip_unicode with one function (e.g., test_roundtrip_parametrized)
decorated with `@pytest.mark.parametrize` supplying the three plaintext values
("my-secret-api-key", "", and "p\u00e4ssw\u00f6rd-\U0001f512-\u6d4b\u8bd5");
inside the test call encryptor.encrypt(plaintext) and assert ciphertext !=
plaintext only for non-empty inputs (or adjust expectation accordingly), then
assert encryptor.decrypt(ciphertext) == plaintext, keeping use of the
SettingsEncryptor fixture and the encrypt/decrypt methods unchanged so the test
logic is identical but consolidated.
In `@tests/unit/settings/test_service.py`:
- Around line 232-283: Replace the duplicated validation tests
(test_rejects_non_float, test_rejects_below_min, test_rejects_above_max,
test_rejects_invalid_enum, test_rejects_invalid_bool) with a single
`@pytest.mark.parametrize-driven` test that iterates cases containing the setting
key, a factory/configuration lambda for registry.register (using
_make_definition and SettingType/enum_values/min_value/max_value/yaml_path as
needed), the input string to set, and the expected error substring; inside the
parametrized test create a SettingsRegistry, call registry.register(...) using
the provided factory, instantiate SettingsService(repository=mock_repo,
registry=registry, config=config), and assert with
pytest.raises(SettingValidationError, match=expected_message) when awaiting
svc.set(key_group, key_name, input_value). Ensure each case's unique parameters
map to the original behaviors and error messages.
- Around line 118-119: The TestResolutionOrder test class is marked
`@pytest.mark.unit` but lacks a timeout; add a 30-second pytest timeout to the
class (or decorate each test) so every test in TestResolutionOrder enforces a
30s cap—apply pytest.mark.timeout(30) alongside pytest.mark.unit on the
TestResolutionOrder class declaration to enforce the limit.
---
Duplicate comments:
In `@src/synthorg/settings/definitions/__init__.py`:
- Around line 7-16: Add an explicit __all__ to clarify the package's public API
and prevent tooling from flagging the module-level imports as unused: keep the
existing imports (backup, budget, company, coordination, memory, observability,
providers, security) and add a top-level __all__ =
['backup','budget','company','coordination','memory','observability','providers','security']
so these names are intentionally re-exported from this package.
In `@src/synthorg/settings/service.py`:
- Around line 141-148: The range-check currently includes the raw numeric value
in SettingValidationError messages even when the setting is sensitive; update
_check_range(definition: SettingDefinition, value: float) to mask the displayed
value when definition.sensitive is True by using the same masking approach used
elsewhere in this module (use the module's existing mask/obfuscation helper or
logic) so that the error messages reference the masked value instead of the raw
numeric; preserve the descriptive context (min/max and that the value is
below/above) while only changing how the offending value is rendered.
In `@tests/unit/api/controllers/test_settings.py`:
- Around line 10-11: The module-level calls to make_auth_headers that assign
_HEADERS and _OBSERVER_HEADERS run at import-time and should be moved into
fixtures; remove the top-level assignments (_HEADERS, _OBSERVER_HEADERS) from
tests/unit/api/controllers/test_settings.py and instead create pytest fixtures
(e.g., auth_headers_ceo and auth_headers_observer) that call
make_auth_headers("ceo") and make_auth_headers("observer") at runtime, register
them in the test file or conftest.py, and update each test to accept the
appropriate fixture name so headers are generated per-test rather than at
import.
In `@tests/unit/persistence/sqlite/test_migrations_v9.py`:
- Around line 171-176: The test currently only asserts namespace and key, which
misses validating the migrated value; update the assertion after fetching rows
(variables cursor and rows) to assert the full tuple equals ("_system", "k1",
"<expected_value>") so the test validates the migrated value from the settings
table in memory_db and ensures correct precedence behavior.
In `@tests/unit/settings/test_config_bridge.py`:
- Around line 22-23: Add a class-level timeout marker to the test class
TestExtractFromConfig by annotating it with `@pytest.mark.timeout`(30) alongside
the existing `@pytest.mark.unit` so each test in the class has a 30-second limit;
ensure pytest-timeout is available in tests and place the marker directly above
the class definition near the existing `@pytest.mark.unit` decorator.
In `@tests/unit/settings/test_definitions.py`:
- Around line 14-15: Add a 30‑second pytest timeout marker to the
TestDefinitionsLoading test class by applying `@pytest.mark.timeout`(30) alongside
the existing `@pytest.mark.unit` decorator (i.e., stack the timeout decorator
above or below `@pytest.mark.unit` on the class definition) so each test in
TestDefinitionsLoading fails fast if it exceeds 30 seconds; ensure pytest is
imported so the marker is available.
- Around line 18-31: Combine the two duplicate tests
test_all_definitions_load_without_error and test_no_duplicate_registrations into
a single test (e.g., test_definitions_registry_valid) that calls get_registry()
once and asserts registry.size > 0; update the docstring to state both intents
(“importing definitions must not raise and the registry must contain entries;
duplicates would have raised during import”). Replace the two existing test
functions with this single consolidated test and keep the assertion against
registry.size to avoid duplicated assertions.
In `@tests/unit/settings/test_models.py`:
- Around line 1-16: The file is missing the module-level 30s timeout marker; add
a module-level pytestmark that includes the existing pytest.mark.unit plus
pytest.mark.timeout(30) (e.g. pytestmark = [pytest.mark.unit,
pytest.mark.timeout(30)]) near the top of the module so tests in this file use
the 30-second timeout.
🪄 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: 12277e6f-6d0c-4120-9bc1-9561824071c8
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
CLAUDE.mddocs/design/memory.mddocs/design/operations.mdpyproject.tomlsrc/synthorg/api/app.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/state.pysrc/synthorg/observability/events/settings.pysrc/synthorg/persistence/protocol.pysrc/synthorg/persistence/repositories.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/persistence/sqlite/migrations.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/settings/__init__.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/settings/definitions/budget.pysrc/synthorg/settings/definitions/company.pysrc/synthorg/settings/definitions/coordination.pysrc/synthorg/settings/definitions/memory.pysrc/synthorg/settings/definitions/observability.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/encryption.pysrc/synthorg/settings/enums.pysrc/synthorg/settings/errors.pysrc/synthorg/settings/models.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/service.pytests/unit/api/conftest.pytests/unit/api/controllers/test_settings.pytests/unit/api/fakes.pytests/unit/config/conftest.pytests/unit/observability/test_events.pytests/unit/persistence/sqlite/test_migrations_v6.pytests/unit/persistence/sqlite/test_migrations_v9.pytests/unit/persistence/sqlite/test_settings_repo.pytests/unit/persistence/test_migrations_v2.pytests/unit/persistence/test_protocol.pytests/unit/settings/__init__.pytests/unit/settings/test_config_bridge.pytests/unit/settings/test_definitions.pytests/unit/settings/test_encryption.pytests/unit/settings/test_models.pytests/unit/settings/test_registry.pytests/unit/settings/test_service.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: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __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
Files:
tests/unit/persistence/sqlite/test_migrations_v9.pysrc/synthorg/settings/definitions/company.pytests/unit/settings/test_service.pytests/unit/persistence/test_protocol.pysrc/synthorg/persistence/sqlite/migrations.pysrc/synthorg/settings/registry.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/settings/definitions/coordination.pytests/unit/observability/test_events.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/enums.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/api/state.pytests/unit/config/conftest.pysrc/synthorg/persistence/protocol.pysrc/synthorg/observability/events/settings.pytests/unit/settings/test_definitions.pytests/unit/persistence/sqlite/test_migrations_v6.pytests/unit/settings/test_registry.pysrc/synthorg/settings/__init__.pytests/unit/api/conftest.pysrc/synthorg/settings/encryption.pytests/unit/api/fakes.pytests/unit/persistence/test_migrations_v2.pytests/unit/settings/test_models.pysrc/synthorg/settings/definitions/memory.pysrc/synthorg/settings/service.pytests/unit/settings/test_config_bridge.pytests/unit/persistence/sqlite/test_settings_repo.pysrc/synthorg/persistence/repositories.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/settings/definitions/budget.pytests/unit/settings/test_encryption.pysrc/synthorg/settings/definitions/observability.pysrc/synthorg/api/app.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/settings/models.pysrc/synthorg/settings/errors.pytests/unit/api/controllers/test_settings.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: All tests must be marked with@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, or@pytest.mark.slow
Maintain 80% minimum code coverage (enforced in CI)
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned test code — use generic names:test-provider,test-small-001,test-medium-001,test-large-001
Use@pytest.mark.parametrizefor testing similar cases instead of duplicating test code
Use Hypothesis (@given+@settings) for property-based testing; run dev profile withHYPOTHESIS_PROFILE=devfor 1000 examples
Never skip, dismiss, or ignore flaky tests — always fix them fundamentally by mocking time functions instead of widening timing margins
Files:
tests/unit/persistence/sqlite/test_migrations_v9.pytests/unit/settings/test_service.pytests/unit/persistence/test_protocol.pytests/unit/observability/test_events.pytests/unit/config/conftest.pytests/unit/settings/test_definitions.pytests/unit/persistence/sqlite/test_migrations_v6.pytests/unit/settings/test_registry.pytests/unit/api/conftest.pytests/unit/api/fakes.pytests/unit/persistence/test_migrations_v2.pytests/unit/settings/test_models.pytests/unit/settings/test_config_bridge.pytests/unit/persistence/sqlite/test_settings_repo.pytests/unit/settings/test_encryption.pytests/unit/api/controllers/test_settings.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: All public functions and classes must have type hints with mypy strict mode enabled
Use Google-style docstrings (required on all public classes and functions) — enforced by ruff D rules
Every module with business logic MUST import logger viafrom synthorg.observability import get_loggerand assign to variable namedlogger
Never useimport logging,logging.getLogger(), orprint()in application code — use the logger from observability module instead
Create new objects instead of mutating existing ones; for non-Pydantic internal collections usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves (viamodel_copy(update=...)) — never mix static config with mutable runtime fields
UseNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Use@computed_fieldfor derived values instead of storing and validating redundant fields
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g., multiple tool invocations, parallel agent calls) — use structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate input at system boundaries: user input, external APIs, and config files
All provider calls must go throughBaseCompletionProviderwhich applies retry and rate limiting automatically — never implement retry logic in driver subclasses or calling code
Use event constants fromsynthorg.observability.events.<domain>modules for all structured logging instead of string literals
Log all error paths at WARNING or ERROR level with context before raising, and log all state transitions at INFO level
Use DEBUG logging for object creation, internal flow, and entry/exit of key function...
Files:
src/synthorg/settings/definitions/company.pysrc/synthorg/persistence/sqlite/migrations.pysrc/synthorg/settings/registry.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/settings/definitions/coordination.pysrc/synthorg/settings/definitions/providers.pysrc/synthorg/settings/enums.pysrc/synthorg/settings/config_bridge.pysrc/synthorg/api/state.pysrc/synthorg/persistence/protocol.pysrc/synthorg/observability/events/settings.pysrc/synthorg/settings/__init__.pysrc/synthorg/settings/encryption.pysrc/synthorg/settings/definitions/memory.pysrc/synthorg/settings/service.pysrc/synthorg/persistence/repositories.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/settings/definitions/budget.pysrc/synthorg/settings/definitions/observability.pysrc/synthorg/api/app.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/settings/models.pysrc/synthorg/settings/errors.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS read the relevant
docs/design/page before implementing any feature — design spec is the starting point for architecture, data models, and behavior
Files:
docs/design/operations.mddocs/design/memory.md
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
docs/design/**/*.md: When a spec topic is referenced (e.g., 'the Agents page' or 'the Engine page's Crash Recovery section'), read the relevant design page before coding
When approved deviations from design spec occur, update the relevantdocs/design/page to reflect the new reality
Files:
docs/design/operations.mddocs/design/memory.md
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: All Python dependencies must use pinned versions with==— Dependabot auto-updates daily via grouped minor/patch updates
Useuvfor Python dependency management with groups:test,dev(includes test),docs
Files:
pyproject.toml
🧠 Learnings (29)
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` modules for all structured logging instead of string literals
Applied to files:
tests/unit/observability/test_events.pysrc/synthorg/observability/events/settings.pyCLAUDE.mdsrc/synthorg/settings/definitions/observability.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.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 : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
src/synthorg/settings/config_bridge.pysrc/synthorg/api/state.pyCLAUDE.md
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Never use `import logging`, `logging.getLogger()`, or `print()` in application code — use the logger from observability module instead
Applied to files:
src/synthorg/settings/config_bridge.pysrc/synthorg/api/state.pyCLAUDE.mdsrc/synthorg/settings/definitions/observability.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST import logger via `from synthorg.observability import get_logger` and assign to variable named `logger`
Applied to files:
src/synthorg/settings/config_bridge.pysrc/synthorg/api/state.pyCLAUDE.md
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Use DEBUG logging for object creation, internal flow, and entry/exit of key functions
Applied to files:
src/synthorg/settings/config_bridge.pyCLAUDE.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/**/*.py : DEBUG logging for object creation, internal flow, entry/exit of key functions.
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 : 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/observability/events/settings.pyCLAUDE.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 tests/**/*.py : Test timeout: 30 seconds per test.
Applied to files:
tests/unit/settings/test_definitions.pytests/unit/settings/test_models.pytests/unit/settings/test_config_bridge.py
📚 Learning: 2026-03-16T19:51:10.862Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.862Z
Learning: Use 30-second timeout per test to catch performance regressions early
Applied to files:
tests/unit/settings/test_definitions.pytests/unit/settings/test_models.pytests/unit/settings/test_config_bridge.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to tests/**/*.py : Never skip, dismiss, or ignore flaky tests — always fix them fundamentally by mocking time functions instead of widening timing margins
Applied to files:
tests/unit/settings/test_definitions.pytests/unit/settings/test_models.pytests/unit/settings/test_config_bridge.pytests/unit/api/controllers/test_settings.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 : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
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 src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.mdsrc/synthorg/settings/encryption.pysrc/synthorg/settings/service.pysrc/synthorg/settings/errors.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 state transitions must log at INFO.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Log all error paths at WARNING or ERROR level with context before raising, and log all state transitions at INFO level
Applied to files:
CLAUDE.mdsrc/synthorg/settings/errors.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 pyproject.toml : Dependencies: all versions use == in pyproject.toml. Groups: test (pytest + plugins, hypothesis), dev (includes test + ruff, mypy, pre-commit, commitizen, pip-audit). Required: mem0ai (Mem0 memory backend — the default and currently only backend). Install: uv sync installs everything (dev group is default).
Applied to files:
CLAUDE.mdpyproject.toml
📚 Learning: 2026-03-16T19:51:10.862Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.862Z
Learning: Applies to pyproject.toml : Use `uv` for Python dependency management with groups: `test`, `dev` (includes test), `docs`
Applied to files:
CLAUDE.mdpyproject.toml
📚 Learning: 2026-03-16T19:51:10.862Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.862Z
Learning: Applies to web/package.json : Web dashboard dependencies: Node.js 20+, Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, Vitest, fast-check, ESLint, vue-tsc
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T19:51:10.862Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.862Z
Learning: Applies to pyproject.toml : All Python dependencies must use pinned versions with `==` — Dependabot auto-updates daily via grouped minor/patch updates
Applied to files:
CLAUDE.mdpyproject.toml
📚 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 web/** : Web dashboard: Node.js 20+, dependencies in web/package.json (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc).
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 docker/{Dockerfile*,compose.yml} : Docker: Backend uses 3-stage build (builder → setup → distroless runtime), Chainguard Python, non-root (UID 65532), CIS-hardened. Web uses nginxinc/nginx-unprivileged, Vue 3 SPA with PrimeVue + Tailwind CSS, SPA routing, API/WebSocket proxy to backend.
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: Property-based testing: Python uses Hypothesis (given + settings). Hypothesis profiles: ci (200 examples, default) and dev (1000 examples), controlled via HYPOTHESIS_PROFILE env var. Run dev profile: HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties.
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 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/settings/test_models.pytests/unit/settings/test_config_bridge.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow exceptions
Applied to files:
src/synthorg/settings/service.pysrc/synthorg/settings/errors.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves (via `model_copy(update=...)`) — never mix static config with mutable runtime fields
Applied to files:
src/synthorg/settings/models.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 : Validate: at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/settings/models.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Validate input at system boundaries: user input, external APIs, and config files
Applied to files:
src/synthorg/settings/models.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to tests/**/*.py : Use `pytest.mark.parametrize` for testing similar cases instead of duplicating test code
Applied to files:
tests/unit/api/controllers/test_settings.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/api/controllers/test_settings.py
🧬 Code graph analysis (28)
tests/unit/persistence/sqlite/test_migrations_v9.py (1)
src/synthorg/persistence/sqlite/migrations.py (3)
_apply_v9(476-531)get_user_version(283-287)set_user_version(290-309)
src/synthorg/settings/definitions/company.py (3)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(17-133)src/synthorg/settings/registry.py (2)
get_registry(111-113)register(30-50)
tests/unit/persistence/test_protocol.py (2)
src/synthorg/persistence/repositories.py (18)
SettingsRepository(665-771)get(62-74)get(221-233)get(349-361)get(429-441)get(554-566)get(621-633)get(672-689)get_namespace(691-706)get_all(708-718)delete(98-110)delete(263-275)delete(399-411)delete(471-483)delete(586-598)delete(649-661)delete(740-757)delete_namespace(759-771)src/synthorg/persistence/sqlite/backend.py (1)
settings(408-414)
src/synthorg/settings/registry.py (3)
src/synthorg/settings/models.py (1)
SettingDefinition(17-133)src/synthorg/settings/service.py (1)
get(188-256)src/synthorg/persistence/sqlite/settings_repo.py (1)
get(34-58)
src/synthorg/api/controllers/settings.py (4)
src/synthorg/settings/service.py (5)
delete(525-551)get(188-256)get_all(310-333)get_namespace(283-308)get_entry(258-281)src/synthorg/settings/enums.py (1)
SettingNamespace(6-20)src/synthorg/settings/errors.py (3)
SettingNotFoundError(8-9)SettingsEncryptionError(16-17)SettingValidationError(12-13)src/synthorg/settings/models.py (2)
SettingDefinition(17-133)SettingEntry(225-246)
src/synthorg/settings/definitions/coordination.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(17-133)src/synthorg/settings/registry.py (2)
get_registry(111-113)register(30-50)
src/synthorg/api/state.py (3)
src/synthorg/persistence/sqlite/backend.py (1)
settings(408-414)src/synthorg/persistence/protocol.py (1)
settings(165-167)src/synthorg/settings/service.py (1)
SettingsService(151-614)
src/synthorg/persistence/protocol.py (4)
src/synthorg/persistence/repositories.py (1)
SettingsRepository(665-771)src/synthorg/persistence/sqlite/backend.py (1)
settings(408-414)tests/unit/api/fakes.py (1)
settings(504-505)tests/unit/persistence/test_protocol.py (1)
settings(358-359)
tests/unit/settings/test_definitions.py (2)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/registry.py (5)
SettingsRegistry(12-104)get_registry(111-113)size(102-104)namespaces(93-99)list_all(80-91)
tests/unit/persistence/sqlite/test_migrations_v6.py (1)
tests/unit/persistence/test_migrations_v2.py (1)
test_schema_version_is_nine(32-33)
tests/unit/settings/test_registry.py (3)
src/synthorg/settings/enums.py (2)
SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(17-133)src/synthorg/settings/registry.py (7)
SettingsRegistry(12-104)register(30-50)get(52-62)list_namespace(64-78)list_all(80-91)namespaces(93-99)size(102-104)
tests/unit/api/conftest.py (4)
src/synthorg/persistence/protocol.py (1)
settings(165-167)tests/unit/settings/test_service.py (3)
registry(81-84)service(105-112)config(100-101)src/synthorg/settings/registry.py (1)
get_registry(111-113)src/synthorg/settings/service.py (1)
SettingsService(151-614)
src/synthorg/settings/encryption.py (3)
src/synthorg/settings/errors.py (1)
SettingsEncryptionError(16-17)tests/unit/settings/test_encryption.py (1)
key(15-16)src/synthorg/settings/service.py (1)
get(188-256)
tests/unit/persistence/test_migrations_v2.py (1)
tests/unit/persistence/sqlite/test_migrations_v6.py (1)
test_schema_version_is_nine(20-21)
tests/unit/settings/test_models.py (2)
src/synthorg/settings/enums.py (4)
SettingLevel(38-46)SettingNamespace(6-20)SettingSource(49-60)SettingType(23-35)src/synthorg/settings/models.py (3)
SettingDefinition(17-133)SettingEntry(225-246)SettingValue(202-222)
tests/unit/settings/test_config_bridge.py (1)
src/synthorg/settings/config_bridge.py (1)
extract_from_config(13-42)
tests/unit/persistence/sqlite/test_settings_repo.py (1)
src/synthorg/persistence/sqlite/settings_repo.py (5)
get(34-58)delete(131-159)get_namespace(60-80)get_all(82-97)delete_namespace(161-183)
src/synthorg/persistence/sqlite/backend.py (2)
src/synthorg/persistence/sqlite/settings_repo.py (2)
SQLiteSettingsRepository(21-183)get(34-58)src/synthorg/persistence/protocol.py (3)
settings(165-167)get_setting(169-181)set_setting(183-195)
src/synthorg/settings/definitions/security.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(17-133)src/synthorg/settings/registry.py (2)
get_registry(111-113)register(30-50)
src/synthorg/settings/definitions/backup.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(17-133)src/synthorg/settings/registry.py (2)
get_registry(111-113)register(30-50)
src/synthorg/settings/definitions/budget.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(17-133)src/synthorg/settings/registry.py (2)
get_registry(111-113)register(30-50)
tests/unit/settings/test_encryption.py (2)
src/synthorg/settings/encryption.py (4)
SettingsEncryptor(21-125)encrypt(31-54)decrypt(56-88)from_env(91-125)src/synthorg/settings/errors.py (1)
SettingsEncryptionError(16-17)
src/synthorg/settings/definitions/observability.py (3)
src/synthorg/settings/enums.py (3)
SettingLevel(38-46)SettingNamespace(6-20)SettingType(23-35)src/synthorg/settings/models.py (1)
SettingDefinition(17-133)src/synthorg/settings/registry.py (2)
get_registry(111-113)register(30-50)
src/synthorg/api/app.py (2)
src/synthorg/settings/service.py (1)
SettingsService(151-614)src/synthorg/api/state.py (1)
settings_service(225-227)
src/synthorg/api/controllers/__init__.py (1)
src/synthorg/api/controllers/settings.py (1)
SettingsController(45-220)
src/synthorg/persistence/sqlite/settings_repo.py (3)
src/synthorg/persistence/sqlite/backend.py (1)
settings(408-414)src/synthorg/persistence/protocol.py (1)
settings(165-167)tests/unit/persistence/test_protocol.py (1)
settings(358-359)
src/synthorg/settings/models.py (1)
src/synthorg/settings/enums.py (4)
SettingLevel(38-46)SettingNamespace(6-20)SettingSource(49-60)SettingType(23-35)
tests/unit/api/controllers/test_settings.py (4)
tests/unit/api/conftest.py (2)
make_auth_headers(111-137)test_client(193-224)src/synthorg/settings/service.py (2)
get(188-256)delete(525-551)src/synthorg/persistence/sqlite/settings_repo.py (2)
get(34-58)delete(131-159)src/synthorg/settings/registry.py (1)
get(52-62)
Comment on lines
+424
to
+437
| repo = self._require_connected(self._settings, "settings") | ||
| result = await repo.get(NotBlankStr("_system"), key) | ||
| return result[0] if result is not None else None | ||
|
|
||
| async def set_setting(self, key: NotBlankStr, value: str) -> None: | ||
| """Store a setting value (upsert) in the ``_system`` namespace. | ||
|
|
||
| Delegates to ``self.settings`` (the ``SettingsRepository``). | ||
|
|
||
| Raises: | ||
| PersistenceConnectionError: If not connected. | ||
| """ | ||
| repo = self._require_connected(self._settings, "settings") | ||
| updated_at = datetime.now(UTC).isoformat() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer using self.settings inside legacy wrappers for one canonical access path.
get_setting() and set_setting() currently call _require_connected(self._settings, "settings") directly. Using self.settings avoids duplicated connection-check logic and keeps behavior centralized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/persistence/sqlite/backend.py` around lines 424 - 437, Replace
direct calls to _require_connected(self._settings, "settings") in get_setting
and set_setting with the canonical accessor self.settings so the centralized
connection-check logic is used; specifically, in get_setting assign repo =
self.settings (then call repo.get(... ) and return as before) and in set_setting
assign repo = self.settings (then use repo.set/ upsert logic and updated_at as
currently implemented), removing the explicit _require_connected(...) calls.
Comment on lines
+22
to
+46
| _r.register( | ||
| SettingDefinition( | ||
| namespace=SettingNamespace.MEMORY, | ||
| key="default_level", | ||
| type=SettingType.ENUM, | ||
| default="persistent", | ||
| description="Default memory persistence level for agents", | ||
| group="General", | ||
| enum_values=("none", "session", "persistent"), | ||
| yaml_path="memory.default_level", | ||
| ) | ||
| ) | ||
|
|
||
| _r.register( | ||
| SettingDefinition( | ||
| namespace=SettingNamespace.MEMORY, | ||
| key="consolidation_interval", | ||
| type=SettingType.ENUM, | ||
| default="daily", | ||
| description="How often to consolidate and archive memories", | ||
| group="Maintenance", | ||
| level=SettingLevel.ADVANCED, | ||
| enum_values=("hourly", "daily", "weekly"), | ||
| yaml_path="memory.consolidation_interval", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Memory-related enums =="
rg -nP --type=py 'class\s+MemoryLevel|class\s+ConsolidationInterval' -C4
echo
echo "== Memory config model fields (level/default_level/consolidation_interval) =="
rg -nP --type=py '\b(default_level|level|consolidation_interval)\b' -C2 src/synthorg
echo
echo "== Current settings definition values =="
rg -nP --type=py 'key="default_level"|key="consolidation_interval"|yaml_path="memory\.' -C2 src/synthorg/settings/definitionsRepository: Aureliolo/synthorg
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Complete MemoryLevel enum =="
sed -n '92,105p' src/synthorg/core/enums.py
echo
echo "== Complete ConsolidationInterval enum =="
sed -n '111,120p' src/synthorg/core/enums.py
echo
echo "== Memory config model (search for class and fields) =="
fd -t f memory\.py src/synthorg --exec wc -l {} \; | sort -rn | head -5
echo
echo "== Memory config model =="
rg -A 20 'class.*Memory.*Config|class.*MemoryConfig' --type=py src/synthorgRepository: Aureliolo/synthorg
Length of output: 10016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Settings definition for default_level and consolidation_interval =="
sed -n '22,46p' src/synthorg/settings/definitions/memory.py
echo
echo "== CompanyMemoryConfig fields (complete) =="
grep -A 50 'class CompanyMemoryConfig' src/synthorg/memory/config.py | head -70Repository: Aureliolo/synthorg
Length of output: 2785
Fix enum_values mismatch and yaml_path field name in MEMORY settings.
Lines 30 and 44 exclude valid enum members: enum_values for default_level omits "project" (MemoryLevel has PERSISTENT, PROJECT, SESSION, NONE), and consolidation_interval omits "never" (ConsolidationInterval has HOURLY, DAILY, WEEKLY, NEVER).
Additionally, line 31's yaml_path="memory.default_level" does not match the actual config field name level in CompanyMemoryConfig. This mismatch will prevent YAML deserialization from resolving to the correct field.
Include all enum members in enum_values and correct the yaml_path to "memory.level".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/settings/definitions/memory.py` around lines 22 - 46, The MEMORY
SettingDefinition entries for key "default_level" (SettingDefinition named
default_level) and "consolidation_interval" are missing enum members and one
yaml_path is incorrect: update the enum_values on default_level to include all
MemoryLevel members (add "project" alongside "none","session","persistent"),
update enum_values on consolidation_interval to include "never" alongside
"hourly","daily","weekly", and change yaml_path on the default_level
SettingDefinition from "memory.default_level" to "memory.level" so it matches
CompanyMemoryConfig.level for proper YAML deserialization.
src/synthorg/settings/models.py
Outdated
Comment on lines
+63
to
+66
| enum_values: tuple[str, ...] = Field( | ||
| default=(), | ||
| description="Allowed values for ENUM type", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use NotBlankStr for enum_values entries.
Line [63] currently allows blank enum literals, which weakens definition integrity for UI/schema consumers.
♻️ Proposed refactor
- enum_values: tuple[str, ...] = Field(
+ enum_values: tuple[NotBlankStr, ...] = Field(
default=(),
description="Allowed values for ENUM type",
)As per coding guidelines: "Use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/settings/models.py` around lines 63 - 66, The enum_values Field
currently uses tuple[str, ...] and allows blank/whitespace entries; change its
annotation to tuple[NotBlankStr, ...] and import NotBlankStr from core.types,
update the Field declaration to use that typed tuple (keeping default=() and the
description), and ensure any necessary typing imports remain; this enforces the
"not blank" constraint for each enum literal referenced by the enum_values
field.
src/synthorg/settings/models.py
Outdated
Comment on lines
+93
to
+112
| if self.min_value is not None and self.type not in ( | ||
| SettingType.INTEGER, | ||
| SettingType.FLOAT, | ||
| ): | ||
| msg = f"min_value is only valid for INTEGER/FLOAT, not {self.type}" | ||
| raise ValueError(msg) | ||
| if self.max_value is not None and self.type not in ( | ||
| SettingType.INTEGER, | ||
| SettingType.FLOAT, | ||
| ): | ||
| msg = f"max_value is only valid for INTEGER/FLOAT, not {self.type}" | ||
| raise ValueError(msg) | ||
| if ( | ||
| self.min_value is not None | ||
| and self.max_value is not None | ||
| and self.min_value > self.max_value | ||
| ): | ||
| msg = f"min_value ({self.min_value}) exceeds max_value ({self.max_value})" | ||
| raise ValueError(msg) | ||
| if self.validator_pattern is not None: |
There was a problem hiding this comment.
Reject non-finite numeric values (nan, inf) in definition constraints and defaults.
Line [93] through Line [199] accepts non-finite floats today; nan can silently bypass min/max comparisons.
🐛 Proposed fix
import json
+import math
import re
@@
if self.max_value is not None and self.type not in (
SettingType.INTEGER,
SettingType.FLOAT,
):
msg = f"max_value is only valid for INTEGER/FLOAT, not {self.type}"
raise ValueError(msg)
+ if self.min_value is not None and not math.isfinite(self.min_value):
+ msg = f"min_value must be finite, got {self.min_value}"
+ raise ValueError(msg)
+ if self.max_value is not None and not math.isfinite(self.max_value):
+ msg = f"max_value must be finite, got {self.max_value}"
+ raise ValueError(msg)
@@
def _check_default_float(default: str) -> None:
try:
- float(default)
+ val = float(default)
except ValueError:
msg = f"default {default!r} is not a valid float"
raise ValueError(msg) from None
+ if not math.isfinite(val):
+ msg = f"default {default!r} must be a finite float"
+ raise ValueError(msg)
@@
if defn.type not in (SettingType.INTEGER, SettingType.FLOAT):
return
val = float(default)
+ if not math.isfinite(val):
+ msg = f"default {default!r} must be finite"
+ raise ValueError(msg)
if defn.min_value is not None and val < defn.min_value:
msg = f"default {val} below min_value {defn.min_value}"
raise ValueError(msg)Based on learnings: "Applies to src/synthorg/**/*.py : Validate input at system boundaries: user input, external APIs, and config files".
Also applies to: 164-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/settings/models.py` around lines 93 - 112, The validation
currently allows non-finite floats (nan/inf) to be used for min_value,
max_value, and numeric defaults; update the validation in the Settings model
(the block checking self.min_value, self.max_value and the block that validates
default values) to explicitly reject non-finite numbers by using math.isfinite
(or equivalent) for any numeric constraint or numeric default when self.type is
SettingType.FLOAT or SettingType.INTEGER and raise a ValueError with a clear
message (e.g. "min_value must be finite", "default value must be finite")
referencing the offending attribute (self.min_value, self.max_value,
self.default). Ensure these checks run before min/max comparisons so nan/inf
cannot bypass range checks.
Comment on lines
+223
to
+236
| if self._encryptor is None: | ||
| logger.error( | ||
| SETTINGS_ENCRYPTION_ERROR, | ||
| namespace=namespace, | ||
| key=key, | ||
| reason="no_encryptor_on_read", | ||
| ) | ||
| msg = ( | ||
| f"Cannot decrypt sensitive setting" | ||
| f" {namespace}/{key}: no encryptor" | ||
| ) | ||
| raise SettingsEncryptionError(msg) | ||
| value = self._encryptor.decrypt(raw_value) | ||
| setting_value = SettingValue( |
There was a problem hiding this comment.
Log decrypt failures with context before propagating.
If self._encryptor.decrypt(raw_value) fails, the exception currently propagates without a service-level log entry containing namespace/key. Add a guarded decrypt block that logs SETTINGS_ENCRYPTION_ERROR with failure reason before re-raising.
🛠️ Proposed patch
if definition.sensitive:
if self._encryptor is None:
logger.error(
SETTINGS_ENCRYPTION_ERROR,
namespace=namespace,
key=key,
reason="no_encryptor_on_read",
)
msg = (
f"Cannot decrypt sensitive setting"
f" {namespace}/{key}: no encryptor"
)
raise SettingsEncryptionError(msg)
- value = self._encryptor.decrypt(raw_value)
+ try:
+ value = self._encryptor.decrypt(raw_value)
+ except SettingsEncryptionError:
+ logger.error(
+ SETTINGS_ENCRYPTION_ERROR,
+ namespace=namespace,
+ key=key,
+ reason="decrypt_failed_on_read",
+ )
+ raiseBased on learnings: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/settings/service.py` around lines 223 - 236, Wrap the call to
self._encryptor.decrypt(raw_value) in a try/except that catches Exception, logs
SETTINGS_ENCRYPTION_ERROR (include namespace and key and a failure reason like
"decrypt_failed" and the exception message/str(err)), then re-raise the
exception; keep the rest of the flow that constructs SettingValue unchanged
(refer to self._encryptor.decrypt, SETTINGS_ENCRYPTION_ERROR,
SettingsEncryptionError and SettingValue to find the exact location).
Comment on lines
+26
to
+53
| def test_top_level_field(self) -> None: | ||
| config = _FakeConfig() | ||
| assert extract_from_config(config, "company_name") == "TestCo" | ||
|
|
||
| def test_nested_field(self) -> None: | ||
| config = _FakeConfig() | ||
| assert extract_from_config(config, "budget.daily_limit") == "10.0" | ||
|
|
||
| def test_nested_bool(self) -> None: | ||
| config = _FakeConfig() | ||
| assert extract_from_config(config, "budget.enabled") == "True" | ||
|
|
||
| def test_missing_top_level(self) -> None: | ||
| config = _FakeConfig() | ||
| assert extract_from_config(config, "nonexistent") is None | ||
|
|
||
| def test_missing_nested(self) -> None: | ||
| config = _FakeConfig() | ||
| assert extract_from_config(config, "budget.nonexistent") is None | ||
|
|
||
| def test_none_field(self) -> None: | ||
| config = _FakeConfig() | ||
| assert extract_from_config(config, "optional_field") is None | ||
|
|
||
| def test_empty_path(self) -> None: | ||
| config = _FakeConfig() | ||
| # Empty string splits to [''] — getattr('') fails | ||
| assert extract_from_config(config, "") is None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate similar extraction cases with parametrization.
Lines 26-53 repeat the same setup/assert pattern and should be collapsed into a @pytest.mark.parametrize table to reduce duplication and improve maintainability.
♻️ Suggested refactor
`@pytest.mark.unit`
+@pytest.mark.timeout(30)
class TestExtractFromConfig:
"""Tests for dotted-path config extraction."""
- def test_top_level_field(self) -> None:
- config = _FakeConfig()
- assert extract_from_config(config, "company_name") == "TestCo"
-
- def test_nested_field(self) -> None:
- config = _FakeConfig()
- assert extract_from_config(config, "budget.daily_limit") == "10.0"
-
- def test_nested_bool(self) -> None:
- config = _FakeConfig()
- assert extract_from_config(config, "budget.enabled") == "True"
-
- def test_missing_top_level(self) -> None:
- config = _FakeConfig()
- assert extract_from_config(config, "nonexistent") is None
-
- def test_missing_nested(self) -> None:
- config = _FakeConfig()
- assert extract_from_config(config, "budget.nonexistent") is None
-
- def test_none_field(self) -> None:
- config = _FakeConfig()
- assert extract_from_config(config, "optional_field") is None
-
- def test_empty_path(self) -> None:
+ `@pytest.mark.parametrize`(
+ ("path", "expected"),
+ [
+ ("company_name", "TestCo"),
+ ("budget.daily_limit", "10.0"),
+ ("budget.enabled", "True"),
+ ("nonexistent", None),
+ ("budget.nonexistent", None),
+ ("optional_field", None),
+ ("", None),
+ ],
+ )
+ def test_extract_from_config(self, path: str, expected: str | None) -> None:
config = _FakeConfig()
- # Empty string splits to [''] — getattr('') fails
- assert extract_from_config(config, "") is None
+ assert extract_from_config(config, path) == expectedAs per coding guidelines, “Use @pytest.mark.parametrize for testing similar cases instead of duplicating test code”.
Comment on lines
+22
to
+36
| def test_roundtrip(self, encryptor: SettingsEncryptor) -> None: | ||
| plaintext = "my-secret-api-key" | ||
| ciphertext = encryptor.encrypt(plaintext) | ||
| assert ciphertext != plaintext | ||
| assert encryptor.decrypt(ciphertext) == plaintext | ||
|
|
||
| def test_roundtrip_empty_string(self, encryptor: SettingsEncryptor) -> None: | ||
| ciphertext = encryptor.encrypt("") | ||
| assert encryptor.decrypt(ciphertext) == "" | ||
|
|
||
| def test_roundtrip_unicode(self, encryptor: SettingsEncryptor) -> None: | ||
| plaintext = "p\u00e4ssw\u00f6rd-\U0001f512-\u6d4b\u8bd5" | ||
| ciphertext = encryptor.encrypt(plaintext) | ||
| assert encryptor.decrypt(ciphertext) == plaintext | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Parametrize roundtrip variants instead of duplicating test bodies.
These three tests have identical flow and only vary input values; consolidate with @pytest.mark.parametrize to match repo testing standards.
♻️ Proposed refactor
- def test_roundtrip(self, encryptor: SettingsEncryptor) -> None:
- plaintext = "my-secret-api-key"
- ciphertext = encryptor.encrypt(plaintext)
- assert ciphertext != plaintext
- assert encryptor.decrypt(ciphertext) == plaintext
-
- def test_roundtrip_empty_string(self, encryptor: SettingsEncryptor) -> None:
- ciphertext = encryptor.encrypt("")
- assert encryptor.decrypt(ciphertext) == ""
-
- def test_roundtrip_unicode(self, encryptor: SettingsEncryptor) -> None:
- plaintext = "p\u00e4ssw\u00f6rd-\U0001f512-\u6d4b\u8bd5"
+ `@pytest.mark.parametrize`(
+ "plaintext",
+ [
+ "my-secret-api-key",
+ "",
+ "p\u00e4ssw\u00f6rd-\U0001f512-\u6d4b\u8bd5",
+ ],
+ )
+ def test_roundtrip(self, encryptor: SettingsEncryptor, plaintext: str) -> None:
ciphertext = encryptor.encrypt(plaintext)
+ assert ciphertext != plaintext or plaintext == ""
assert encryptor.decrypt(ciphertext) == plaintextAs per coding guidelines, "Use @pytest.mark.parametrize for testing similar cases instead of duplicating test code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/test_encryption.py` around lines 22 - 36, Consolidate the
three duplicate tests into a single parametrized test: replace test_roundtrip,
test_roundtrip_empty_string, and test_roundtrip_unicode with one function (e.g.,
test_roundtrip_parametrized) decorated with `@pytest.mark.parametrize` supplying
the three plaintext values ("my-secret-api-key", "", and
"p\u00e4ssw\u00f6rd-\U0001f512-\u6d4b\u8bd5"); inside the test call
encryptor.encrypt(plaintext) and assert ciphertext != plaintext only for
non-empty inputs (or adjust expectation accordingly), then assert
encryptor.decrypt(ciphertext) == plaintext, keeping use of the SettingsEncryptor
fixture and the encrypt/decrypt methods unchanged so the test logic is identical
but consolidated.
Comment on lines
+118
to
+119
| @pytest.mark.unit | ||
| class TestResolutionOrder: |
There was a problem hiding this comment.
Add timeout enforcement for unit test classes.
Line 118 marks the class as unit, but there is no timeout marker to cap execution time.
Based on learnings, tests should use a 30-second timeout per test to catch performance regressions early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/test_service.py` around lines 118 - 119, The
TestResolutionOrder test class is marked `@pytest.mark.unit` but lacks a timeout;
add a 30-second pytest timeout to the class (or decorate each test) so every
test in TestResolutionOrder enforces a 30s cap—apply pytest.mark.timeout(30)
alongside pytest.mark.unit on the TestResolutionOrder class declaration to
enforce the limit.
tests/unit/settings/test_service.py
Outdated
Comment on lines
+232
to
+283
| async def test_rejects_non_float(self, service: SettingsService) -> None: | ||
| with pytest.raises(SettingValidationError, match="Expected float"): | ||
| await service.set("budget", "total_monthly", "not-a-number") | ||
|
|
||
| async def test_rejects_below_min( | ||
| self, mock_repo: AsyncMock, config: _FakeConfig | ||
| ) -> None: | ||
| registry = SettingsRegistry() | ||
| registry.register(_make_definition(min_value=0.0)) | ||
| svc = SettingsService(repository=mock_repo, registry=registry, config=config) | ||
| with pytest.raises(SettingValidationError, match="below minimum"): | ||
| await svc.set("budget", "total_monthly", "-1.0") | ||
|
|
||
| async def test_rejects_above_max( | ||
| self, mock_repo: AsyncMock, config: _FakeConfig | ||
| ) -> None: | ||
| registry = SettingsRegistry() | ||
| registry.register(_make_definition(max_value=1000.0)) | ||
| svc = SettingsService(repository=mock_repo, registry=registry, config=config) | ||
| with pytest.raises(SettingValidationError, match="above maximum"): | ||
| await svc.set("budget", "total_monthly", "9999.0") | ||
|
|
||
| async def test_rejects_invalid_enum( | ||
| self, mock_repo: AsyncMock, config: _FakeConfig | ||
| ) -> None: | ||
| registry = SettingsRegistry() | ||
| registry.register( | ||
| _make_definition( | ||
| key="strategy", | ||
| setting_type=SettingType.ENUM, | ||
| enum_values=("a", "b"), | ||
| yaml_path=None, | ||
| ) | ||
| ) | ||
| svc = SettingsService(repository=mock_repo, registry=registry, config=config) | ||
| with pytest.raises(SettingValidationError, match="Invalid enum"): | ||
| await svc.set("budget", "strategy", "c") | ||
|
|
||
| async def test_rejects_invalid_bool( | ||
| self, mock_repo: AsyncMock, config: _FakeConfig | ||
| ) -> None: | ||
| registry = SettingsRegistry() | ||
| registry.register( | ||
| _make_definition( | ||
| key="enabled", | ||
| setting_type=SettingType.BOOLEAN, | ||
| yaml_path=None, | ||
| ) | ||
| ) | ||
| svc = SettingsService(repository=mock_repo, registry=registry, config=config) | ||
| with pytest.raises(SettingValidationError, match="Expected boolean"): | ||
| await svc.set("budget", "enabled", "maybe") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Refactor repetitive validation cases into parameterized tests.
This block repeats the same arrange/act/assert shape for type validation paths and should be table-driven with @pytest.mark.parametrize.
As per coding guidelines, “Use @pytest.mark.parametrize for testing similar cases instead of duplicating test code”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/test_service.py` around lines 232 - 283, Replace the
duplicated validation tests (test_rejects_non_float, test_rejects_below_min,
test_rejects_above_max, test_rejects_invalid_enum, test_rejects_invalid_bool)
with a single `@pytest.mark.parametrize-driven` test that iterates cases
containing the setting key, a factory/configuration lambda for registry.register
(using _make_definition and
SettingType/enum_values/min_value/max_value/yaml_path as needed), the input
string to set, and the expected error substring; inside the parametrized test
create a SettingsRegistry, call registry.register(...) using the provided
factory, instantiate SettingsService(repository=mock_repo, registry=registry,
config=config), and assert with pytest.raises(SettingValidationError,
match=expected_message) when awaiting svc.set(key_group, key_name, input_value).
Ensure each case's unique parameters map to the original behaviors and error
messages.
Source fixes: - Use self.settings property instead of _require_connected (backend.py) - Fix memory definitions: add missing enum members (project, never), correct yaml_path memory.default_level → memory.level - Add non-finite float rejection (math.isfinite) for min/max/defaults - enum_values typed as tuple[NotBlankStr, ...] for blank prevention - Wrap single-get decrypt in try/except with logger.exception - Mask numeric values in range errors for sensitive settings - Add __all__ to definitions/__init__.py Test improvements: - Parametrize encryption roundtrip tests (3 → 1) - Parametrize validation rejection tests (5 → 1) - Move auth headers from module-level to fixtures - Add pytest.mark.timeout(30) to config_bridge, definitions, models, resolution-order test classes - Consolidate duplicate definitions tests (2 → 1) - Assert full tuple in crash-with-both-tables migration test - Fix fakes delete_namespace single-pass rebuild
This was referenced Mar 16, 2026
This was referenced Mar 17, 2026
Aureliolo
added a commit
that referenced
this pull request
Mar 17, 2026
🤖 I have created a release *beep* *boop* --- ## [0.3.1](v0.3.0...v0.3.1) (2026-03-17) ### Features * **api:** RFC 9457 Phase 2 — ProblemDetail and content negotiation ([#496](#496)) ([30f7c49](30f7c49)) * **cli:** verify container image signatures and SLSA provenance on pull ([#492](#492)) ([bef272d](bef272d)), closes [#491](#491) * **engine:** implement context budget management in execution loops ([#520](#520)) ([181eb8a](181eb8a)), closes [#416](#416) * implement settings persistence layer (DB-backed config) ([#495](#495)) ([4bd99f7](4bd99f7)), closes [#450](#450) * **memory:** implement dual-mode archival in memory consolidation ([#524](#524)) ([4603c9e](4603c9e)), closes [#418](#418) * migrate config consumers to read through SettingsService ([#510](#510)) ([32f553d](32f553d)) * **settings:** implement settings change subscriptions for service hot-reload ([#526](#526)) ([53f908e](53f908e)), closes [#503](#503) * **settings:** register API config in SettingsService with 2-phase init ([#518](#518)) ([29f7481](29f7481)) * **tools:** add SSRF prevention for git clone URLs ([#505](#505)) ([492dd0d](492dd0d)) * **tools:** wire RootConfig.git_clone to GitCloneTool instantiation ([#519](#519)) ([b7d8172](b7d8172)) ### Bug Fixes * **api:** replace JWT query parameter with one-time ticket for WebSocket auth ([#493](#493)) ([22a25f6](22a25f6)), closes [#343](#343) ### Documentation * add uv cache lock contention handling to worktree skill ([#500](#500)) ([bd85a8d](bd85a8d)) * document RFC 9457 dual response formats in OpenAPI schema ([#506](#506)) ([8dd2524](8dd2524)) ### Maintenance * upgrade jsdom from 28 to 29 ([#499](#499)) ([1ea2249](1ea2249)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Mar 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SettingsRepositoryprotocol +SQLiteSettingsRepositorywith V9 migration (crash-safe schema evolution from simple KV to namespaced composite PK)SettingsServicewith invalidate-on-write cache, type validation, Fernet encryption for sensitive settings, and change notifications via message busSettingsRegistrymetadata catalog with 8 namespace definition modules (budget, security, providers, company, memory, coordination, observability, backup)cryptographydependency for Fernet symmetric encryptionReview coverage
Pre-reviewed by 6 agents (code-reviewer, python-reviewer, security-reviewer, test-analyzer, persistence-reviewer, issue-verifier). 22 findings addressed:
Test plan
Follow-up
Closes #450