feat(backup): implement automated backup and restore system#541
feat(backup): implement automated backup and restore system#541
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🧰 Additional context used📓 Path-based instructions (4)src/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,ts,py,go}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-03-18T09:27:32.266ZApplied to files:
📚 Learning: 2026-03-15T11:48:14.867ZApplied to files:
🔇 Additional comments (11)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a complete backup and restore subsystem: BackupService orchestration, component handlers (persistence, memory, config), scheduler, retention manager, API controller and state wiring, settings subscriber, observability event constants, configuration/schema additions, factory builders, extensive unit tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as BackupService
participant Handlers as ComponentHandlers
participant FS as FileSystem
participant Retention as RetentionManager
participant Manifest as ManifestStore
Client->>Service: create_backup(trigger, components)
activate Service
Service->>Service: acquire _backup_lock
loop per component
Service->>Handlers: handler.backup(target_dir)
activate Handlers
Handlers->>FS: snapshot / copy files
Handlers-->>Service: bytes_written
deactivate Handlers
end
Service->>Service: compute checksum + write manifest
Service->>Manifest: persist manifest.json
alt compression enabled
Service->>FS: create tar.gz archive, remove temp dir
end
Service->>Retention: prune()
activate Retention
Retention->>FS: load manifests
Retention->>FS: delete prunable backups
Retention-->>Service: pruned_ids
deactivate Retention
Service-->>Client: BackupManifest
deactivate Service
sequenceDiagram
participant Client as Client
participant Service as BackupService
participant Handlers as ComponentHandlers
participant FS as FileSystem
Client->>Service: restore_from_backup(backup_id, components)
activate Service
Service->>Service: acquire _backup_lock
Service->>FS: locate & validate backup + manifest
Service->>Service: create safety backup
Service->>FS: extract/prepare component sources
loop per component
Service->>Handlers: handler.validate_source(source_dir)
Service->>Handlers: handler.restore(source_dir)
activate Handlers
Handlers->>FS: atomic swap restore
Handlers-->>Service: restore_ok
deactivate Handlers
end
alt failure during restore
Service->>Service: rollback from safety backup
Service-->>Client: RestoreError
else success
Service-->>Client: RestoreResponse
end
deactivate Service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
|
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 backup and restore system to the application, covering persistence data, agent memory, and company configurations. It provides both automated and manual backup options, integrates with the application lifecycle, and includes REST endpoints for administration. The system emphasizes data integrity and safety through checksums, compression, and schema validation. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive backup and restore system for the SynthOrg application, covering persistence DB, agent memory, and company config. It includes a BackupService orchestrator, BackupScheduler, RetentionManager, and ComponentHandler protocol. REST endpoints for backup management are also implemented. The code changes are extensive, spanning multiple files and introducing new modules. The review focuses on correctness, maintainability, and adherence to the established code conventions, particularly around logging and error handling.
src/synthorg/api/app.py
Outdated
| if backup_service._config.on_startup: # noqa: SLF001 | ||
| try: | ||
| await backup_service.create_backup( | ||
| BackupTrigger.STARTUP, | ||
| ) | ||
| except Exception: | ||
| logger.warning( | ||
| API_APP_STARTUP, | ||
| error="Startup backup failed (non-fatal)", | ||
| exc_info=True, | ||
| ) |
There was a problem hiding this comment.
The backup_service._config is accessing a protected member. This could lead to unexpected behavior if the internal implementation of BackupService changes. Consider using a public method or property to access this configuration value. This is a high severity issue because it violates encapsulation and increases the risk of future bugs.
src/synthorg/api/app.py
Outdated
| if backup_service._config.on_shutdown: # noqa: SLF001 | ||
| try: | ||
| await backup_service.create_backup( | ||
| BackupTrigger.SHUTDOWN, | ||
| ) | ||
| except Exception: | ||
| logger.warning( | ||
| API_APP_SHUTDOWN, | ||
| error="Shutdown backup failed (non-fatal)", | ||
| exc_info=True, | ||
| ) |
There was a problem hiding this comment.
The backup_service._config is accessing a protected member. This could lead to unexpected behavior if the internal implementation of BackupService changes. Consider using a public method or property to access this configuration value. This is a high severity issue because it violates encapsulation and increases the risk of future bugs.
| - **Never** use `import logging` / `logging.getLogger()` / `print()` in application code | ||
| - **Variable name**: always `logger` (not `_logger`, not `log`) | ||
| - **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`, `GIT_COMMAND_START` from `events.git`, `CONTEXT_BUDGET_FILL_UPDATED` from `events.context_budget`). Each domain has its own module — see `src/synthorg/observability/events/` for the full inventory of constants. Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT` | ||
| - **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`, `GIT_COMMAND_START` from `events.git`, `CONTEXT_BUDGET_FILL_UPDATED` from `events.context_budget`, `BACKUP_STARTED` from `events.backup`). Each domain has its own module -- see `src/synthorg/observability/events/` for the full inventory of constants. Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT` |
There was a problem hiding this comment.
Per the style guide, the event names should use double dashes instead of single dashes. Please update BACKUP_STARTED to BACKUP--STARTED.
| - **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`, `GIT_COMMAND_START` from `events.git`, `CONTEXT_BUDGET_FILL_UPDATED` from `events.context_budget`, `BACKUP_STARTED` from `events.backup`). Each domain has its own module -- see `src/synthorg/observability/events/` for the full inventory of constants. Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT` | |
| - **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`, `GIT_COMMAND_START` from `events.git`, `CONTEXT_BUDGET_FILL_UPDATED` from `events.context_budget`, `BACKUP_STARTED` from `events.backup`). Each domain has its own module -- see `src/synthorg/observability/events/` for the full inventory of constants. Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT` | |
| + **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`, `GIT_COMMAND_START` from `events.git`, `CONTEXT_BUDGET_FILL_UPDATED` from `events.context_budget`, `BACKUP--STARTED` from `events.backup`). Each domain has its own module -- see `src/synthorg/observability/events/` for the full inventory of constants. Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT` |
| if ".." in parts: | ||
| msg = "Backup path must not contain parent-directory traversal (..)" | ||
| logger.warning( | ||
| CONFIG_VALIDATION_FAILED, | ||
| field="path", | ||
| value=self.path, | ||
| reason=msg, | ||
| ) |
There was a problem hiding this comment.
The logging message should include the exception information for better debugging. Consider adding exc_info=True to the logger.warning call.
logger.warning(
CONFIG_VALIDATION_FAILED,
field="path",
value=self.path,
reason=msg,
)
+ exc_info=TrueReferences
- All error paths must log at WARNING or ERROR with context before raising
| logger.debug(BACKUP_SCHEDULER_TICK) | ||
| try: | ||
| await self._service.create_backup(BackupTrigger.SCHEDULED) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
The except clause should be except (MemoryError, RecursionError): to adhere to PEP 758 syntax.
try:
await self._service.create_backup(BackupTrigger.SCHEDULED)
except MemoryError, RecursionError:
+ except (MemoryError, RecursionError):
raise
except Exception:References
- PEP 758 except syntax: use
except A, B:(no parentheses) — ruff enforces this on Python 3.14
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
==========================================
- Coverage 93.01% 92.49% -0.52%
==========================================
Files 526 542 +16
Lines 25687 26655 +968
Branches 2437 2544 +107
==========================================
+ Hits 23893 24655 +762
- Misses 1429 1598 +169
- Partials 365 402 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthorg/config/schema.py (1)
388-424: 🧹 Nitpick | 🔵 TrivialUpdate docstring to include the new
backupattribute.The
RootConfigdocstring lists all attributes but is missing the newly addedbackupfield.📝 Suggested docstring update
Add to the Attributes section (around line 423):
coordination: Multi-agent coordination configuration. git_clone: Git clone SSRF prevention network policy. + backup: Backup and restore configuration. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/config/schema.py` around lines 388 - 424, The RootConfig docstring's Attributes list is missing the newly added backup field; update the RootConfig class docstring to include a brief entry for backup (e.g., "backup: Backup/restore configuration.") so the Attributes section documents the new field alongside other attributes; locate the RootConfig class definition and add the backup attribute description to the Attributes block in the same style as existing entries.src/synthorg/settings/definitions/backup.py (1)
20-30:⚠️ Potential issue | 🟡 MinorRemove the unused
frequencysetting (lines 20-30) or document its intended purpose.The
frequencysetting is registered in the definitions file but is not used anywhere in the backup system:
BackupSettingsSubscriber._WATCHEDdoes not include it (only watchesenabled,schedule_hours,compression,on_shutdown,on_startup)BackupConfigdoes not define afrequencyfield; it usesschedule_hoursas the sole scheduling mechanism- No code in the codebase references this setting
Either remove this orphaned setting or clarify if it is planned for future use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/settings/definitions/backup.py` around lines 20 - 30, The registered SettingDefinition with namespace=SettingNamespace.BACKUP and key="frequency" is orphaned; remove the registration block (the SettingDefinition that sets key "frequency") if it's unused, or alternatively add it to BackupSettingsSubscriber._WATCHED and to BackupConfig (and any parsing/validation) and update code that schedules backups (which currently uses schedule_hours) to accept/interpret the new frequency enum ("hourly","daily","weekly") so the setting is actually consumed; pick one approach and make corresponding changes to the symbols BackupSettingsSubscriber._WATCHED, BackupConfig, and any scheduling logic that uses schedule_hours to ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/app.py`:
- Around line 719-725: The backup service is being constructed from guessed
config paths instead of the actual runtime paths the app started with; update
the call sites (including create_app and the other occurrences around the
852-889 region) to pass the resolved runtime paths into _build_backup_service()
rather than letting it recompute from RootConfig or environment fallbacks.
Concretely, locate where create_app wires the DB (where SYNTHORG_DB_PATH may be
applied) and where PersistenceComponentHandler/config.persistence.sqlite.path is
referenced, capture the resolved sqlite path and the resolved company.yaml path
the app actually uses at startup, and pass those concrete paths as parameters
into _build_backup_service(effective_config, resolved_db_path,
resolved_company_path) (or an equivalent signature) so backups/restores operate
on the real runtime files. Ensure all other callers of _build_backup_service in
the file are updated to supply the resolved runtime paths similarly.
In `@src/synthorg/api/controllers/backup.py`:
- Around line 58-64: Update the exception handlers in
src/synthorg/api/controllers/backup.py so they log the caught backup errors with
a backup-domain event at WARNING/ERROR before re-raising and stop echoing raw
exception text for 5xx responses: in the except BackupInProgressError as exc
block and the except BackupError as exc block (and the other similar blocks you
noted) call the controller/module logger (e.g., logger.error or logger.warning)
with context including the backup id/operation and the exception, then raise
ClientException with the same 409 status for backup-in-progress but keep the
log; for BackupError replace raising InternalServerException(str(exc)) with
raising InternalServerException("internal-backup-error") (or a generic internal
error detail) while logging exc (use logger.exception or logger.error with
exc_info) so internals like filesystem paths are not returned to clients.
In `@src/synthorg/backup/handlers/config_handler.py`:
- Around line 99-118: The restore path raises ComponentBackupError without
logging; update the restore method in the ConfigHandler to log an ERROR or
WARNING before each raise: log the missing directory case (use config_dir and
source_dir context) before raising the "Backup config directory not found"
error, log the empty backup_files case (include config_dir) before raising the
"No config files found" error, and in the exception handler log the failure
(include source_file, self._config_path and exc) before re-raising
ComponentBackupError; use the existing logger instance (e.g., self._logger or
module logger) and include clear contextual messages.
In `@src/synthorg/backup/handlers/memory.py`:
- Around line 101-104: Replace the blocking call to source.exists() with an
async-friendly call using asyncio.to_thread so the filesystem check runs off the
event loop; specifically, in the memory backup handler where 'source =
source_dir / _MEMORY_SUBDIR' is computed, change the existence check to await
asyncio.to_thread(source.exists) and keep the same error path that raises
ComponentBackupError with the same message; mirror the pattern used in
validate_source and backup to ensure non-blocking behavior.
In `@src/synthorg/backup/handlers/persistence.py`:
- Around line 94-109: In restore(), add a WARNING/ERROR log call that includes
the component/source context and exception details before raising
ComponentBackupError: when source_file is missing log a warning including self
(or component id) and source_file before raising the ComponentBackupError, and
inside the except Exception as exc block log the error (including self,
source_file, bak_path, and exc) prior to raising the wrapped
ComponentBackupError; ensure the logs are emitted from the same logger used by
this class (e.g., self.logger or component logger) so context is preserved for
_atomic_swap, _db_path, source_file and bak_path failures.
- Around line 166-188: The _atomic_swap() restore currently only moves
synthorg.db (db_path) and ignores SQLite sidecars which can cause WAL replay;
update the function to also remove or move
db_path.with_name(f"{db_path.name}-wal") and
db_path.with_name(f"{db_path.name}-shm") (and their bak equivalents) before
opening the restored DB for the integrity check, and ensure the same sidecar
files are restored during the rollback path if a bak exists; also delete any
remaining sidecars on successful cleanup so no stale -wal/-shm files remain
alongside the new db file.
In `@src/synthorg/backup/retention.py`:
- Around line 151-168: The method _load_dir_manifest does not use instance state
and should be made a `@staticmethod` for consistency with _load_archive_manifest;
change the definition of _load_dir_manifest to be a staticmethod and update any
internal references accordingly (it currently only reads manifest_path, json,
and appends to the manifests list), and ensure call sites (where
_load_dir_manifest is invoked) call it on the class or instance without relying
on self, keeping parameter list (entry, manifests) unchanged; alternatively, if
you prefer to keep it as an instance method, explicitly use self._backup_path or
instance attributes in logging to justify non-static usage—pick one approach and
make the signatures/calls consistent with _load_archive_manifest.
In `@src/synthorg/backup/scheduler.py`:
- Around line 69-80: reschedule currently only updates _interval_seconds so an
ongoing asyncio.sleep in the scheduler loop (e.g., in the background run/_run
method) keeps the old delay; change reschedule to interrupt the current sleep
immediately by either (A) introducing an asyncio.Event (e.g., self._wake_event)
that the run loop waits on with timeout=self._interval_seconds (clear the event
after waking) and having reschedule set the event, or (B) cancelling and
restarting the background task (e.g., self._task) from reschedule; update the
run/_run loop to use asyncio.wait_for or event.wait with the interval so the new
interval takes effect immediately when reschedule sets the event or restarts the
task, and ensure proper cleanup in stop/shutdown paths.
In `@src/synthorg/backup/service.py`:
- Around line 266-273: The code performs blocking I/O by calling
shutil.rmtree(backup_dir) directly inside the async method after calling
self._compress_dir; change this to run in a thread by awaiting
asyncio.to_thread(shutil.rmtree, backup_dir) so the deletion is non-blocking and
consistent with the prior asyncio.to_thread usage; update the block that uses
use_compression / archive_path / backup_dir / self._compress_dir to call await
asyncio.to_thread for shutil.rmtree instead of the synchronous call.
- Around line 541-558: The _try_delete_backup function performs blocking
filesystem ops from an async context; change its signature to async def
_try_delete_backup(self, backup_id: str) -> bool and wrap the blocking calls
with await asyncio.to_thread(...): use await asyncio.to_thread(shutil.rmtree,
entry) for directory removal and await asyncio.to_thread(entry.unlink) for file
deletion; update callers (notably delete_backup) to await _try_delete_backup so
all blocking I/O runs off the event loop.
- Around line 196-199: The call to backup_dir.mkdir(...) inside the async backup
routine is performing blocking I/O; change it to run on a thread (use
asyncio.to_thread) so it matches other async-safe helpers like _compute_checksum
and _compress_dir and remove the ASYNC240 exception. Specifically, replace the
direct call to backup_dir.mkdir(parents=True, exist_ok=True) (and similarly
self._backup_path.mkdir if needed) with an awaited asyncio.to_thread invocation,
and add the asyncio import if it's not already present so the async method
performs directory creation without blocking the event loop.
- Around line 370-372: The finally block calls synchronous
shutil.rmtree(backup_dir) which blocks the event loop; change it to run in a
thread by awaiting asyncio.to_thread(shutil.rmtree, backup_dir) (and ensure
asyncio is imported), keeping the same guards (temp_extracted and backup_dir is
not None and backup_dir.exists()); reference the identifiers temp_extracted,
backup_dir, and shutil.rmtree in your change so you replace the blocking call
with an await asyncio.to_thread(...) call.
In `@src/synthorg/settings/subscribers/backup_subscriber.py`:
- Around line 69-73: The on_settings_changed handler currently triggers side
effects based only on key; guard on the namespace first by checking the
namespace equals "backup" before handling keys so that non-backup namespaces
cannot call _toggle_scheduler or _reschedule; update the on_settings_changed
function to return early or skip side-effect branches when namespace !=
"backup", then proceed to inspect key ("enabled", "schedule_hours") and call
self._toggle_scheduler() or self._reschedule() only when namespace is "backup".
- Around line 107-112: In _reschedule, wrap the conversion of result.value and
the call to scheduler.reschedule in a try/except that catches
ValueError/TypeError (and handle a missing result by checking for None), log a
warning or error via logger including the setting name
("backup","schedule_hours") and the raw result.value (and result object) before
re-raising the exception, so failures to parse schedule_hours are recorded with
context prior to propagation; keep references to _settings_service.get,
result.value, scheduler.reschedule and logger in your fix.
In `@tests/unit/api/controllers/test_backup.py`:
- Around line 11-17: Add a module-level pytest timeout marker to enforce the 30s
limit by declaring a pytestmark variable at the top of the test module (e.g.,
pytestmark = pytest.mark.timeout(30)); update
tests/unit/api/controllers/test_backup.py to include this module-level marker so
all tests in the file (including those referenced around functions in this
module) inherit the 30-second timeout constraint.
In `@tests/unit/backup/test_config.py`:
- Around line 150-174: Add Windows-style traversal test cases to exercise
BackupConfig._reject_path_traversal's PureWindowsPath branch: update
test_rejects_path_with_dotdot to include inputs like "..\\secrets" and
"C:\\backup\\..\\escape" (and any other backslash-based traversal forms), and
add corresponding safe Windows-style paths to test_accepts_safe_paths as needed
so the Windows parsing branch is covered and cannot regress.
In `@tests/unit/settings/test_backup_subscriber.py`:
- Around line 1-11: Add a module-level pytest timeout marker to the test file to
enforce the 30s per-test limit: after the imports in
tests/unit/settings/test_backup_subscriber.py, define pytestmark =
pytest.mark.timeout(30). This ensures the module-level marker applies to tests
in this file; locate the file where SettingsSubscriber and
BackupSettingsSubscriber are imported and add the pytestmark assignment
immediately following those imports.
- Around line 127-146: The parametrize list in
test_advisory_key_does_not_start_scheduler wrongly includes "schedule_hours"
even though backup_subscriber.py's _reschedule path calls
scheduler.reschedule(), so update the test to remove "schedule_hours" from the
parameter list (or move it into its own test asserting that
service.scheduler.reschedule() is called when sub.on_settings_changed("backup",
"schedule_hours") is invoked) so this advisory-keys test only contains true
advisory keys ("compression", "on_shutdown", "on_startup") and no longer asserts
on the behavior of _reschedule/scheduler.reschedule.
- Around line 148-155: The test's name and docstring incorrectly call
schedule_hours "advisory" even though BackupSubscriber._reschedule triggers
scheduler.reschedule; rename the test from test_schedule_hours_is_advisory to
test_schedule_hours_does_not_toggle_scheduler and update the docstring
accordingly, then add an assertion that
service.scheduler.reschedule.assert_called_once() (or assert_awaited if it's
async) after awaiting sub.on_settings_changed("backup", "schedule_hours") while
keeping the existing assertions that start/stop are not called; reference
BackupSubscriber._reschedule, the on_settings_changed call, and
service.scheduler.reschedule in your changes.
---
Outside diff comments:
In `@src/synthorg/config/schema.py`:
- Around line 388-424: The RootConfig docstring's Attributes list is missing the
newly added backup field; update the RootConfig class docstring to include a
brief entry for backup (e.g., "backup: Backup/restore configuration.") so the
Attributes section documents the new field alongside other attributes; locate
the RootConfig class definition and add the backup attribute description to the
Attributes block in the same style as existing entries.
In `@src/synthorg/settings/definitions/backup.py`:
- Around line 20-30: The registered SettingDefinition with
namespace=SettingNamespace.BACKUP and key="frequency" is orphaned; remove the
registration block (the SettingDefinition that sets key "frequency") if it's
unused, or alternatively add it to BackupSettingsSubscriber._WATCHED and to
BackupConfig (and any parsing/validation) and update code that schedules backups
(which currently uses schedule_hours) to accept/interpret the new frequency enum
("hourly","daily","weekly") so the setting is actually consumed; pick one
approach and make corresponding changes to the symbols
BackupSettingsSubscriber._WATCHED, BackupConfig, and any scheduling logic that
uses schedule_hours to ensure consistency.
🪄 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: ec68a184-3dc9-4f41-8497-11811edd5920
📒 Files selected for processing (40)
CLAUDE.mdsrc/synthorg/api/app.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/state.pysrc/synthorg/backup/__init__.pysrc/synthorg/backup/config.pysrc/synthorg/backup/errors.pysrc/synthorg/backup/handlers/__init__.pysrc/synthorg/backup/handlers/config_handler.pysrc/synthorg/backup/handlers/memory.pysrc/synthorg/backup/handlers/persistence.pysrc/synthorg/backup/handlers/protocol.pysrc/synthorg/backup/models.pysrc/synthorg/backup/retention.pysrc/synthorg/backup/scheduler.pysrc/synthorg/backup/service.pysrc/synthorg/config/defaults.pysrc/synthorg/config/schema.pysrc/synthorg/observability/events/backup.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/settings/subscribers/__init__.pysrc/synthorg/settings/subscribers/backup_subscriber.pytests/unit/api/controllers/test_backup.pytests/unit/api/test_app.pytests/unit/backup/__init__.pytests/unit/backup/conftest.pytests/unit/backup/test_config.pytests/unit/backup/test_errors.pytests/unit/backup/test_handlers/__init__.pytests/unit/backup/test_handlers/test_config_handler.pytests/unit/backup/test_handlers/test_memory_handler.pytests/unit/backup/test_handlers/test_persistence_handler.pytests/unit/backup/test_models.pytests/unit/backup/test_retention.pytests/unit/backup/test_scheduler.pytests/unit/backup/test_service.pytests/unit/config/conftest.pytests/unit/observability/test_events.pytests/unit/settings/test_backup_subscriber.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). (4)
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow.
Coverage: 80% minimum — enforced in CI.
Async:asyncio_mode = "auto"— no manual@pytest.mark.asyncioneeded.
Timeout: 30 seconds per test.
Parallelism:pytest-xdistvia-n auto— ALWAYS include-n autowhen running pytest, never run tests sequentially.
Parametrize: Prefer@pytest.mark.parametrizefor testing similar cases.
Tests must usetest-provider,test-small-001, etc. instead of vendor names.
Property-based testing: Python uses Hypothesis (@given+@settings). Profiles:ci(200 examples, default) anddev(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/observability/test_events.pytests/unit/api/controllers/test_backup.pytests/unit/backup/test_handlers/test_persistence_handler.pytests/unit/backup/conftest.pytests/unit/backup/test_handlers/test_memory_handler.pytests/unit/backup/test_config.pytests/unit/backup/test_errors.pytests/unit/backup/test_models.pytests/unit/backup/test_retention.pytests/unit/backup/test_handlers/test_config_handler.pytests/unit/backup/test_scheduler.pytests/unit/backup/test_service.pytests/unit/config/conftest.pytests/unit/settings/test_backup_subscriber.pytests/unit/api/test_app.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations.
Useexcept A, B:syntax (no parentheses) for exception handling — PEP 758 except syntax enforced by ruff on Python 3.14.
Type hints: all public functions required, mypy strict mode enforced.
Docstrings: Google style, required on public classes/functions — enforced by ruff D rules.
Immutability: create new objects, never mutate existing ones. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).
Config vs runtime state: frozen Pydantic models for config/identity; separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Models: Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict). Use@computed_fieldfor derived values instead of storing redundant fields. UseNotBlankStrfor all identifier/name fields — including optional (NotBlankStr | None) and tuple variants — instead of manual whitespace validators.
Async concurrency: preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g., multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length: 88 characters — enforced by ruff.
Functions: < 50 lines, files < 800 lines.
Errors: handle explicitly, never silently swallow.
Validate: at system boundaries (user input, external APIs, config files).
Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config ...
Files:
src/synthorg/api/controllers/__init__.pysrc/synthorg/config/schema.pysrc/synthorg/backup/__init__.pysrc/synthorg/backup/handlers/protocol.pysrc/synthorg/backup/config.pysrc/synthorg/backup/errors.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/api/app.pysrc/synthorg/backup/handlers/persistence.pysrc/synthorg/config/defaults.pysrc/synthorg/backup/handlers/memory.pysrc/synthorg/api/state.pysrc/synthorg/backup/retention.pysrc/synthorg/backup/models.pysrc/synthorg/backup/scheduler.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/backup/service.pysrc/synthorg/settings/subscribers/__init__.pysrc/synthorg/observability/events/backup.pysrc/synthorg/backup/handlers/config_handler.pysrc/synthorg/backup/handlers/__init__.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport logging/logging.getLogger()/print()in application code.
Logging variable name: alwayslogger(not_logger, notlog).
Event names: always use constants from the domain-specific module undersynthorg.observability.events(e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Each domain has its own module.
Structured logging kwargs: alwayslogger.info(EVENT, key=value)— neverlogger.info("msg %s", val).
All error paths must log at WARNING or ERROR with context before raising.
All state transitions must log at INFO.
DEBUG logging for object creation, internal flow, entry/exit of key functions.
Pure data models, enums, and re-exports do NOT need logging.
All provider calls go throughBaseCompletionProviderwhich applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig.
Retryable errors (is_retryable=True):RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError. Non-retryable errors raise immediately.
RetryExhaustedErrorsignals that all retries failed — the engine layer catches this to trigger fallback chains.
Rate limiter respectsRateLimitError.retry_afterfrom providers — automatically pauses future requests.
Files:
src/synthorg/api/controllers/__init__.pysrc/synthorg/config/schema.pysrc/synthorg/backup/__init__.pysrc/synthorg/backup/handlers/protocol.pysrc/synthorg/backup/config.pysrc/synthorg/backup/errors.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/api/app.pysrc/synthorg/backup/handlers/persistence.pysrc/synthorg/config/defaults.pysrc/synthorg/backup/handlers/memory.pysrc/synthorg/api/state.pysrc/synthorg/backup/retention.pysrc/synthorg/backup/models.pysrc/synthorg/backup/scheduler.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/backup/service.pysrc/synthorg/settings/subscribers/__init__.pysrc/synthorg/observability/events/backup.pysrc/synthorg/backup/handlers/config_handler.pysrc/synthorg/backup/handlers/__init__.py
🧠 Learnings (14)
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Each domain has its own module.
Applied to files:
tests/unit/observability/test_events.pysrc/synthorg/observability/events/backup.pyCLAUDE.md
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
Learning: Applies to src/**/*.py : Models: 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 — including optional (`NotBlankStr | None`) and tuple variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/config/schema.py
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
Learning: Applies to src/**/*.py : Config vs runtime state: frozen Pydantic models for config/identity; 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/backup/config.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 : Config vs runtime state: frozen Pydantic models for config/identity; 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/backup/config.py
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
Learning: Applies to src/synthorg/**/*.py : Retryable errors (`is_retryable=True`): `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`. Non-retryable errors raise immediately.
Applied to files:
tests/unit/backup/test_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/backup.pyCLAUDE.md
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
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.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
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-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.md
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
Learning: Applies to src/synthorg/**/*.py : Structured logging kwargs: 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 : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
Learning: Applies to src/synthorg/**/*.py : Logging variable name: always `logger` (not `_logger`, not `log`).
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.md
📚 Learning: 2026-03-18T07:33:37.988Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T07:33:37.988Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO.
Applied to files:
CLAUDE.md
🧬 Code graph analysis (27)
src/synthorg/api/controllers/__init__.py (5)
src/synthorg/backup/handlers/config_handler.py (1)
backup(41-88)src/synthorg/backup/handlers/memory.py (1)
backup(42-86)src/synthorg/backup/handlers/persistence.py (1)
backup(41-79)src/synthorg/backup/handlers/protocol.py (1)
backup(28-37)src/synthorg/api/controllers/backup.py (1)
BackupController(33-163)
tests/unit/api/controllers/test_backup.py (8)
src/synthorg/backup/handlers/config_handler.py (1)
backup(41-88)src/synthorg/backup/handlers/memory.py (1)
backup(42-86)src/synthorg/backup/handlers/persistence.py (1)
backup(41-79)src/synthorg/api/controllers/backup.py (6)
BackupController(33-163)create_backup(41-65)list_backups(68-82)get_backup(85-104)delete_backup(107-126)restore_backup(129-163)src/synthorg/backup/errors.py (4)
BackupInProgressError(12-13)BackupNotFoundError(32-33)ManifestError(20-21)RestoreError(16-17)src/synthorg/backup/models.py (5)
BackupComponent(24-29)BackupManifest(32-60)BackupTrigger(14-21)RestoreRequest(85-98)RestoreResponse(101-116)src/synthorg/api/state.py (1)
backup_service(374-376)src/synthorg/backup/service.py (5)
create_backup(101-128)list_backups(429-449)get_backup(509-521)delete_backup(523-539)restore_from_backup(294-315)
src/synthorg/config/schema.py (1)
src/synthorg/backup/config.py (1)
BackupConfig(34-99)
tests/unit/backup/test_handlers/test_persistence_handler.py (3)
src/synthorg/backup/handlers/persistence.py (6)
backup(41-79)PersistenceComponentHandler(23-188)component(37-39)restore(81-109)validate_source(111-135)_check_integrity(153-157)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(24-29)
tests/unit/backup/conftest.py (2)
src/synthorg/backup/config.py (2)
BackupConfig(34-99)RetentionConfig(20-31)src/synthorg/backup/models.py (3)
BackupComponent(24-29)BackupManifest(32-60)BackupTrigger(14-21)
src/synthorg/backup/__init__.py (2)
src/synthorg/backup/config.py (2)
BackupConfig(34-99)RetentionConfig(20-31)src/synthorg/backup/service.py (1)
BackupService(58-739)
src/synthorg/backup/handlers/protocol.py (4)
src/synthorg/backup/handlers/config_handler.py (4)
backup(41-88)component(37-39)restore(90-118)validate_source(120-133)src/synthorg/backup/handlers/memory.py (4)
backup(42-86)component(38-40)restore(88-119)validate_source(121-131)src/synthorg/backup/handlers/persistence.py (4)
backup(41-79)component(37-39)restore(81-109)validate_source(111-135)src/synthorg/backup/models.py (1)
BackupComponent(24-29)
src/synthorg/backup/config.py (1)
src/synthorg/backup/models.py (1)
BackupComponent(24-29)
tests/unit/backup/test_handlers/test_memory_handler.py (1)
src/synthorg/backup/handlers/memory.py (5)
backup(42-86)MemoryComponentHandler(24-163)component(38-40)restore(88-119)validate_source(121-131)
src/synthorg/settings/definitions/backup.py (3)
src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)src/synthorg/settings/enums.py (3)
SettingNamespace(6-21)SettingType(24-36)SettingLevel(39-47)
src/synthorg/api/app.py (5)
src/synthorg/backup/handlers/protocol.py (1)
ComponentHandler(16-56)src/synthorg/backup/models.py (2)
BackupComponent(24-29)BackupTrigger(14-21)src/synthorg/backup/service.py (4)
BackupService(58-739)stop(97-99)start(92-95)create_backup(101-128)src/synthorg/api/state.py (3)
persistence(145-147)backup_service(374-376)AppState(37-394)src/synthorg/settings/subscriber.py (1)
SettingsSubscriber(11-55)
src/synthorg/backup/handlers/persistence.py (4)
src/synthorg/backup/handlers/memory.py (4)
backup(42-86)component(38-40)restore(88-119)_atomic_swap(144-163)src/synthorg/backup/handlers/protocol.py (3)
backup(28-37)component(24-26)restore(39-45)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(24-29)
tests/unit/backup/test_config.py (2)
src/synthorg/backup/config.py (2)
BackupConfig(34-99)RetentionConfig(20-31)src/synthorg/backup/models.py (1)
BackupComponent(24-29)
tests/unit/backup/test_errors.py (1)
src/synthorg/backup/errors.py (7)
BackupError(8-9)BackupInProgressError(12-13)BackupNotFoundError(32-33)ComponentBackupError(24-25)ManifestError(20-21)RestoreError(16-17)RetentionError(28-29)
src/synthorg/backup/handlers/memory.py (3)
src/synthorg/backup/handlers/protocol.py (4)
backup(28-37)component(24-26)restore(39-45)validate_source(47-56)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(24-29)
src/synthorg/api/state.py (1)
src/synthorg/backup/service.py (1)
BackupService(58-739)
src/synthorg/backup/retention.py (3)
src/synthorg/backup/errors.py (1)
RetentionError(28-29)src/synthorg/backup/models.py (2)
BackupManifest(32-60)BackupTrigger(14-21)src/synthorg/backup/config.py (1)
RetentionConfig(20-31)
tests/unit/backup/test_models.py (2)
src/synthorg/backup/models.py (6)
BackupComponent(24-29)BackupInfo(63-82)BackupManifest(32-60)BackupTrigger(14-21)RestoreRequest(85-98)RestoreResponse(101-116)tests/unit/backup/conftest.py (1)
sample_manifest(40-56)
tests/unit/backup/test_retention.py (4)
src/synthorg/backup/config.py (1)
RetentionConfig(20-31)src/synthorg/backup/errors.py (1)
RetentionError(28-29)src/synthorg/backup/models.py (3)
BackupComponent(24-29)BackupManifest(32-60)BackupTrigger(14-21)src/synthorg/backup/retention.py (2)
RetentionManager(27-221)prune(42-68)
src/synthorg/settings/subscribers/backup_subscriber.py (3)
src/synthorg/backup/service.py (4)
BackupService(58-739)scheduler(88-90)start(92-95)stop(97-99)src/synthorg/settings/service.py (1)
SettingsService(152-637)src/synthorg/backup/scheduler.py (4)
is_running(38-40)start(42-57)stop(59-67)reschedule(69-80)
tests/unit/backup/test_handlers/test_config_handler.py (2)
src/synthorg/backup/handlers/config_handler.py (5)
backup(41-88)ConfigComponentHandler(24-141)component(37-39)restore(90-118)validate_source(120-133)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)
tests/unit/backup/test_scheduler.py (2)
src/synthorg/backup/models.py (1)
BackupTrigger(14-21)src/synthorg/backup/scheduler.py (6)
BackupScheduler(22-96)is_running(38-40)start(42-57)stop(59-67)reschedule(69-80)_run_loop(82-96)
src/synthorg/settings/subscribers/__init__.py (1)
src/synthorg/settings/subscribers/backup_subscriber.py (1)
BackupSettingsSubscriber(25-118)
tests/unit/backup/test_service.py (3)
src/synthorg/backup/config.py (2)
BackupConfig(34-99)RetentionConfig(20-31)src/synthorg/backup/errors.py (2)
BackupInProgressError(12-13)BackupNotFoundError(32-33)src/synthorg/backup/models.py (3)
BackupComponent(24-29)BackupManifest(32-60)BackupTrigger(14-21)
tests/unit/config/conftest.py (1)
src/synthorg/backup/config.py (1)
BackupConfig(34-99)
tests/unit/settings/test_backup_subscriber.py (2)
src/synthorg/settings/subscriber.py (1)
SettingsSubscriber(11-55)src/synthorg/settings/subscribers/backup_subscriber.py (4)
BackupSettingsSubscriber(25-118)watched_keys(46-48)subscriber_name(51-53)on_settings_changed(55-80)
tests/unit/api/test_app.py (1)
src/synthorg/api/app.py (2)
_safe_startup(380-508)_safe_shutdown(511-580)
🪛 LanguageTool
CLAUDE.md
[style] ~202-~202: Since ownership is already implied, this phrasing may be redundant.
Context: ... from events.backup). Each domain has its own module -- see `src/synthorg/observabili...
(PRP_OWN)
🔇 Additional comments (51)
src/synthorg/backup/errors.py (1)
1-33: LGTM!Clean exception hierarchy with proper inheritance chain. All exceptions derive from
BackupError, enabling unified catch handling. Docstrings are concise and descriptive.src/synthorg/backup/models.py (1)
1-116: LGTM!Well-structured domain models with proper immutability patterns:
- Frozen Pydantic models with
allow_inf_nan=False.NotBlankStrfor all identifier fields.- Tuple types for immutable collections.
- Field constraints (
ge=0) for size and version fields.src/synthorg/api/controllers/__init__.py (1)
11-11: LGTM!Clean integration of
BackupControllerfollowing the established pattern for controller registration. Import, tuple entry, and__all__export are all properly alphabetized.Also applies to: 45-45, 56-56
tests/unit/config/conftest.py (1)
9-9: LGTM!Correct integration of
BackupConfiginto the test factory, ensuring test fixtures align with the updatedRootConfigschema that now includes backup configuration.Also applies to: 103-103
tests/unit/observability/test_events.py (1)
215-215: LGTM!Correctly adds "backup" to the expected domain modules set, aligning with the new
synthorg.observability.events.backupmodule introduced in this PR.src/synthorg/config/defaults.py (1)
44-44: LGTM!Correctly adds the
backupkey to the base configuration defaults, following the established pattern for other configuration sections. This ensures the backup config section exists with sensible defaults even if omitted from user config files.tests/unit/backup/test_handlers/test_config_handler.py (1)
1-179: LGTM!Comprehensive test coverage for
ConfigComponentHandler:
- Component property verification.
- Backup behavior: successful copy, missing source handling, error propagation.
- Restore behavior: successful restore, missing/empty backup dir errors, copy failure handling.
- Validate source: positive and negative cases.
Tests are well-organized by functionality with clear naming conventions.
src/synthorg/settings/subscribers/__init__.py (1)
3-5: LGTM!The new
BackupSettingsSubscriberexport follows the existing pattern and maintains alphabetical ordering in__all__.Also applies to: 14-14
src/synthorg/backup/__init__.py (1)
1-15: LGTM!Clean package init with appropriate docstring and minimal public API surface. The exports align with what consumers need from the backup subsystem.
src/synthorg/backup/handlers/__init__.py (1)
1-13: LGTM!The handlers package properly exports the
ComponentHandlerprotocol alongside its concrete implementations, enabling consumers to type-hint against the protocol.tests/unit/backup/test_handlers/test_memory_handler.py (1)
1-188: LGTM!Comprehensive test coverage for
MemoryComponentHandlerincluding:
- Component property verification
- Backup success, missing source, and copy failure scenarios
- Restore with atomic swap, rollback on failure, and missing live directory handling
- Source validation checks
The tests properly use
tmp_pathfor isolation and mock filesystem errors to verify error handling.src/synthorg/settings/definitions/backup.py (1)
94-103: LGTM for new settings definitions.The new settings (
schedule_hours,path,compression,on_shutdown,on_startup) are properly defined with appropriate types, defaults, descriptions, and constraints that align withBackupConfigfields.src/synthorg/config/schema.py (1)
543-546: LGTM for the backup configuration field.The
backupfield follows the established pattern for sub-configurations inRootConfig.tests/unit/backup/test_handlers/test_persistence_handler.py (1)
1-239: LGTM!Thorough test coverage for
PersistenceComponentHandlerincluding:
- VACUUM INTO backup creation and validation
- Atomic swap restore with rollback on corrupt backup
- SQLite integrity checking edge cases (valid, corrupt, empty file)
All database connections are properly closed in
finallyblocks, and the tests appropriately verify both filesystem state and database content after operations.tests/unit/backup/test_errors.py (1)
1-103: LGTM!Well-structured tests for the backup error hierarchy using
@pytest.mark.parametrizeas recommended. The tests verify:
- All specific errors inherit from
BackupErrorBackupErrorinherits fromException- Error messages are preserved
- Errors are catchable both as their own type and as the base
BackupErrorThe module-level
pytestmarkfor timeout aligns with the 30-second test timeout guideline.tests/unit/api/controllers/test_backup.py (1)
205-341: Restore endpoint tests are well-covered.Good coverage of confirm-gate short-circuiting and exception-to-HTTP mapping (400/404/409/422/500), including the “service not called” guard path.
tests/unit/api/test_app.py (1)
110-119: Startup/shutdown helper call updates are consistent with new lifecycle arity.These call-site changes correctly insert the backup-service parameter and preserve the intended cleanup assertions.
Also applies to: 137-137, 167-176, 219-220, 237-237, 273-274, 278-278
src/synthorg/backup/handlers/protocol.py (1)
15-56: Protocol contract is clean and complete.The protocol shape aligns with the handlers used by
BackupServiceand is clearly typed/documented.tests/unit/backup/test_retention.py (2)
84-243: Retention policy behavior is thoroughly exercised.Nice coverage for count/age pruning and explicit protection rules (newest + pre-migration).
249-285: Error-path assertions are solid.The corrupt-manifest skip and load-failure wrapping into
RetentionErrorare both validated clearly.tests/unit/backup/conftest.py (1)
17-64: Fixture setup looks good and reusable.These fixtures provide a clean baseline for backup-domain unit tests without coupling to test-specific internals.
tests/unit/backup/test_models.py (1)
71-137: Model validation and immutability coverage is strong.The suite checks defaults, negative-boundary validation, serialization roundtrips, and frozen semantics across all key backup models.
Also applies to: 142-179, 185-251
src/synthorg/api/state.py (1)
12-12: Backup service integration inAppStateis consistent and safe.The deferred-init pattern (
has_*, guarded accessor, single-assignment setter) matches existing state-container conventions and keeps 503 semantics intact.Also applies to: 54-59, 101-101, 366-394
tests/unit/backup/test_scheduler.py (7)
1-11: LGTM!Module setup is correct: proper docstring, imports,
pytest.mark.timeout(30)at module level as per guidelines, and no manual@pytest.mark.asyncio(asyncio_mode=auto is configured).
14-18: LGTM!Helper function cleanly creates a mock service with proper
AsyncMockfor the asynccreate_backupmethod.
21-55: LGTM!
TestSchedulerStartcovers the key behaviors: task creation and idempotent start calls. Mockingasyncio.create_taskis appropriate for testing task creation without running the actual loop.
57-86: LGTM!
TestSchedulerStopcorrectly tests task cancellation with a real task (fortest_stop_cancels_task) and verifies no-op behavior when not running.
88-121: LGTM!
TestSchedulerIsRunningcomprehensively tests the property in all states: initial, after start, and when task is done.
123-148: LGTM!Good use of
@pytest.mark.parametrizefor testing multiple interval conversion scenarios as per guidelines.
150-237: LGTM!
TestSchedulerLooptests cover the critical behaviors:
- Correct trigger type (
BackupTrigger.SCHEDULED)- Error resilience (continues after exceptions)
- MemoryError propagation (not swallowed)
- Correct sleep interval
The
_counting_sleepand_recording_sleeppatterns effectively control loop iterations for testing.tests/unit/settings/test_backup_subscriber.py (3)
13-58: LGTM!The
_make_subscriberhelper is well-structured with clear parameter documentation and proper mock setup for async methods.
60-84: LGTM!Protocol conformance tests correctly verify
isinstancecheck,watched_keys, andsubscriber_name.
86-122: LGTM!
TestBackupSubscriberEnabledcorrectly tests scheduler toggle behavior including start, stop, and idempotency.src/synthorg/backup/handlers/memory.py (4)
1-41: LGTM!Module setup is correct: proper imports,
get_logger(__name__), structured event constants from the backup domain module, and clean class initialization.
42-86: LGTM!The
backupmethod properly:
- Logs lifecycle events with structured kwargs
- Handles missing source directory gracefully
- Wraps blocking I/O in
asyncio.to_thread- Wraps exceptions in
ComponentBackupErrorwith context
121-131: LGTM!
validate_sourcecorrectly usesasyncio.to_threadfor the filesystem check.
133-163: LGTM!Static helpers
_copy_treeand_atomic_swapimplement proper atomic restore semantics with rollback on failure. These run in a thread viaasyncio.to_thread, so synchronous filesystem operations are appropriate.src/synthorg/backup/retention.py (5)
1-25: LGTM!Module setup is correct with proper imports,
get_logger(__name__), and structured event constants from the backup domain module.
27-68: LGTM!
RetentionManagerclass andprunemethod correctly:
- Document behavior (protects newest + PRE_MIGRATION)
- Handle empty manifests
- Sort by timestamp descending before pruning
- Wrap manifest loading errors in
RetentionError
70-111: LGTM!Pruning logic correctly:
- Skips index 0 (newest)
- Skips PRE_MIGRATION trigger
- Prunes by count (
index >= max_count)- Prunes by age with proper timezone-aware comparison
- Handles invalid timestamps gracefully with warning
112-149: LGTM!
_execute_pruneand_load_manifestsare well-implemented:
- Individual delete failures don't abort the entire prune operation
- Supports both directory and archive manifest loading
194-221: LGTM!
_delete_backupimplements a reasonable two-phase lookup:
- Fast path: match by filename prefix
- Fallback: read manifests to find by
backup_idThis handles cases where directory names don't match the backup_id format.
src/synthorg/observability/events/backup.py (1)
1-43: LGTM!Well-organized event constants module following the
backup.<entity>.<action>naming convention. Constants are properly typed withFinal[str]and grouped by category (lifecycle, component, manifest, retention, scheduler, restore, management).src/synthorg/backup/service.py (8)
71-86: LGTM!Constructor correctly implements immutability pattern using
MappingProxyType(deepcopy(handlers))as per guidelines. Lock, retention manager, and scheduler are properly initialized.
122-128: Intentional fast-fail pattern acknowledged.The
locked()check before acquiring is a deliberate design choice to immediately reject concurrent backup requests withBackupInProgressErrorrather than queuing them. The subsequentasync with self._backup_lockprovides the actual synchronization guarantee.
684-695: LGTM!
_compute_checksumcorrectly:
- Includes relative file paths in the hash (guards against renaming attacks)
- Uses streaming reads with
_CHECKSUM_CHUNK_SIZEchunks- Excludes
manifest.jsonto avoid circular dependency- Sorts files for deterministic ordering
704-714: LGTM!
_extract_tarimplements proper security measures:
- Validates against absolute paths and path traversal (
..)- Uses
filter="data"(Python 3.12+) for safe extraction
716-739: LGTM!
_read_manifest_from_archivecorrectly:
- Handles missing
manifest.jsongracefully- Re-raises
MemoryErrorandRecursionError(not swallowed)- Logs invalid manifests at DEBUG level
101-128: LGTM!
create_backupproperly documents the API contract, including:
- Default component selection from config
- Compression behavior (forced off for shutdown)
BackupInProgressErrorfor concurrent attempts
294-374: LGTM!
restore_from_backupand_do_restoreimplement robust restore logic:
- Schema version compatibility check (newer → error)
- Safety backup before restore (PRE_MIGRATION trigger)
- Proper cleanup of temp extracted archives
- Calls
_do_backupdirectly to avoid recursive lock acquisition
429-508: LGTM!
list_backupscorrectly handles both directory and archive-based backups, sorts by timestamp descending, and logs the count.
Add comprehensive backup/restore for persistence DB, agent memory, and company config with scheduled, lifecycle-triggered, and manual backups plus validated restore with atomic rollback. New module: src/synthorg/backup/ - BackupConfig, RetentionConfig (frozen Pydantic models) - BackupManifest, BackupInfo, RestoreRequest/Response models - BackupError hierarchy (7 error types) - ComponentHandler protocol with 3 concrete handlers: - PersistenceComponentHandler (SQLite VACUUM INTO) - MemoryComponentHandler (shutil.copytree) - ConfigComponentHandler (shutil.copy2) - BackupService orchestrator (create, restore, list, get, delete) - BackupScheduler (asyncio background task) - RetentionManager (count + age pruning, pre_migration protection) - BackupController (5 REST endpoints under /admin/backup) - BackupSettingsSubscriber (react to runtime setting changes) - Observability events (backup.py event constants) - 5 new settings (schedule_hours, path, compression, on_shutdown, on_startup) Wiring: - BackupConfig added to RootConfig - AppState: _backup_service slot, property, set_backup_service() - BackupController added to ALL_CONTROLLERS - BackupSettingsSubscriber added to subscribers __init__ Tests: 221 unit tests covering all components.
Fix type annotations and add type-ignore comments for test-specific patterns (mock objects, controller owner, unreachable assertions).
Pre-reviewed by 8 agents, 22 findings addressed: Source fixes: - Retention manager now prunes compressed (.tar.gz) backups - Checksum includes file paths + streaming reads for large files - _handlers dict uses deepcopy + MappingProxyType (immutability) - BackupConfig.include typed as tuple[BackupComponent, ...] not strings - restore_from_backup protected by backup lock, calls _do_backup directly - Schema version compatibility check on restore - Settings subscriber reads actual setting values (not toggling blindly) - _reschedule actually calls scheduler.reschedule() with new value - sqlite3 connections use contextlib.closing context managers - Silent except Exception blocks now log at WARNING with exc_info - Handler constructors accept Path instead of str - Removed str(Path) wrappers throughout - Controller catches BackupError base class for clean 500 responses - Scheduler uses BACKUP_FAILED event constant for error logging - allow_inf_nan=False added to all model ConfigDicts - Functions split to stay under 50-line limit Wiring: - BackupService wired into app.py lifecycle (startup/shutdown) - BackupSettingsSubscriber added to settings dispatcher - CLAUDE.md updated with backup module documentation Tests updated for interface changes (221 backup tests pass).
Update test_config, conftest, and test_service to use BackupComponent enum values instead of strings for BackupConfig.include, and replace mock attribute assertions with manifest-based verification for deepcopied handlers.
…eviewers
Security hardening:
- Verify SHA-256 checksum on restore (was stored but never checked)
- Validate backup_id format (^[0-9a-f]{12}$) at service/model boundary
- Reject symlink targets in tar archives (zip-slip via linkname)
- Handle SQLite WAL/SHM sidecar files during restore
- Pass symlinks=True to copytree (prevent data exfiltration)
- Bound manifest.json reads to 64KB (prevent OOM)
- Sanitize error messages in controller (no internal path leaks)
- Fix startswith prefix matching to use backup_id_ separator
Correctness:
- _extract_archive raises ManifestError instead of returning None
- _archive_matches_backup logs warnings instead of silent swallow
- _dir_matches_backup logs at WARNING instead of DEBUG
- Fix wrong log event (BACKUP_RETENTION_FAILED not BACKUP_FAILED)
- Remove dead except BackupInProgressError re-raise
- restore_from_backup fails fast when lock is held
- Use runtime-resolved paths in _build_backup_service
- ConfigComponentHandler.restore looks up by filename, not [0]
Type/model hardening:
- Add timestamp ISO 8601 validator on BackupManifest
- Add checksum sha256:<hex> format validator
- Add duplicate component validator on BackupConfig
- Add BackupInfo.from_manifest() factory method
- Add RestoreRequest backup_id format validator
- Add BackupScheduler.reschedule bounds validation (>= 1)
Async/concurrency:
- Wrap sync I/O in asyncio.to_thread (rmtree, mkdir, write_text)
- Scheduler reschedule() now interrupts sleep via asyncio.Event
- ConfigComponentHandler.validate_source uses to_thread
- MemoryComponentHandler.restore uses to_thread for exists()
- delete_backup runs _try_delete_backup via to_thread
Logging:
- Add controller logging on all error paths
- Add logging before raise in all handler restore() methods
- Add BACKUP_SCHEDULER_RESCHEDULED event constant
- BackupSettingsSubscriber namespace guard + int() error handling
- _read_manifest_from_archive logs at WARNING not DEBUG
Code quality:
- Expose on_startup/on_shutdown as public properties (no _config access)
- retention._load_dir_manifest returns value instead of mutating arg
- retention._delete_backup returns bool for accurate prune reporting
- retention._load_archive_manifest adds MemoryError/RecursionError guard
- _load_dir_manifest is now @staticmethod
- _compute_checksum skips symlinks
Tests (337 pass):
- Add pytestmark timeout(30) to 5 test files
- Fix schedule_hours test: add reschedule() assertion
- Add model validator tests (timestamp, checksum, backup_id)
- Add duplicate components rejection test
- Add scheduler reschedule bounds validation test
- Add Windows path traversal test cases
- Fix assertion precision (size_bytes == 1536, result == ())
- Update scheduler tests for asyncio.Event wake pattern
- Update fixtures for new model validators
Docs:
- Add Backup and Restore section to operations design page
- Add BackupSettingsSubscriber to concrete subscribers list
- Add backup attribute to RootConfig docstring
52ab853 to
98f42e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
src/synthorg/api/app.py (1)
719-719:⚠️ Potential issue | 🟠 MajorResolved runtime paths not passed to backup service builder.
_build_backup_serviceacceptsresolved_db_pathandresolved_config_pathparameters (lines 855-856) to ensure backups target the actual files the app opened, butcreate_appdoesn't pass these values. WhenSYNTHORG_DB_PATHis used (line 655), the backup handler will still useconfig.persistence.sqlite.pathinstead of the actual path.Pass the resolved paths from the auto-wiring logic:
🐛 Proposed fix
+ # Track the actual DB path used (for backup service) + actual_db_path: Path | None = None + if persistence is None: + db_path = (os.environ.get("SYNTHORG_DB_PATH") or "").strip() + if db_path: + actual_db_path = Path(db_path) try: persistence = create_backend( - PersistenceConfig(sqlite=SQLiteConfig(path=db_path)), + PersistenceConfig(sqlite=SQLiteConfig(path=str(actual_db_path))), ) ... + else: + # If persistence was provided, use its configured path + actual_db_path = Path(effective_config.persistence.sqlite.path) ... bridge = _build_bridge(message_bus, channels_plugin) - backup_service = _build_backup_service(effective_config) + backup_service = _build_backup_service( + effective_config, + resolved_db_path=actual_db_path, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/app.py` at line 719, create_app calls _build_backup_service(effective_config) but does not pass the actual resolved_db_path and resolved_config_path produced by the auto-wiring logic, so the backup handler may use the original config values instead of the files the app opened; update the call in create_app to pass the resolved paths (resolved_db_path and resolved_config_path) returned/available from the auto-wiring routine to _build_backup_service, and ensure the signature used matches _build_backup_service(resolved_db_path, resolved_config_path, effective_config) so backups target the real opened files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/operations.md`:
- Around line 1091-1092: Reword the dense phrase "agent memory data directory"
in the docs entry for MemoryComponentHandler to make it more readable and
scannable; for example, replace it with "the directory that stores an agent's
memory data" (or "agent's memory directory") in the list item referencing
MemoryComponentHandler (alongside the existing ConfigComponentHandler entry) so
the line reads clearly while preserving the mention of shutil.copytree and
symlinks=True.
In `@src/synthorg/backup/config.py`:
- Around line 101-107: The _reject_duplicate_components model_validator
currently raises ValueError when duplicate entries are found but doesn't log;
update the function _reject_duplicate_components to log the validation failure
(including the offending self.include contents and a clear message) at WARNING
or ERROR level before raising; use the module-level logger (e.g., logger) or the
existing logger instance to emit the log entry with context and then raise
ValueError(msg) as before.
In `@src/synthorg/backup/handlers/config_handler.py`:
- Around line 115-125: The code does blocking filesystem I/O by calling
source_file.exists() and list(config_dir.iterdir()) directly in an async
context; change these to run on a thread (e.g., wrap both calls with
asyncio.to_thread or the event loop's run_in_executor) so they do not block the
event loop — locate the block that references source_file, config_dir,
BACKUP_COMPONENT_FAILED, logger and ComponentBackupError and replace the
synchronous exists() and iterdir() usage with asyncio.to_thread calls that
return the boolean/extracted list before proceeding with the existing warning
and exception logic.
In `@src/synthorg/backup/models.py`:
- Around line 66-67: BackupManifest.backup_id and
RestoreResponse.safety_backup_id are only typed as NotBlankStr and need format
validation at model boundaries to prevent malformed IDs from propagating; add a
stricter type or validator (e.g., create/replace NotBlankStr with BackupIDStr or
attach a pydantic/validator function) that enforces the backup-id regex/format
and use it for both BackupManifest.backup_id and
RestoreResponse.safety_backup_id (also update the other occurrence mentioned
around lines 174-175) so any bad ID is rejected early at model construction.
- Around line 68-77: The timestamp validator _validate_timestamp currently
accepts naive ISO 8601 strings causing a TypeError later in
RetentionManager._should_prune when subtracting from datetime.now(UTC); update
_validate_timestamp (the field_validator for "timestamp") to parse the string
with datetime.fromisoformat (or equivalent) and verify that the resulting
datetime.tzinfo is not None (i.e., timezone-aware), and if it is naive raise a
ValueError with a clear message like "Timestamp must include timezone/offset";
keep the existing ValueError handling for invalid formats so callers see a
single manifest-validation error for both format and timezone issues.
In `@src/synthorg/backup/retention.py`:
- Around line 68-70: Manifests are being sorted by their ISO string timestamps
which fails when timezone offsets vary; change the sort to parse each
manifest.timestamp into a timezone-aware datetime and sort by that datetime
(newest first) before calling self._identify_prunable(manifests). Locate the
sort around the manifests variable in retention.py and replace the lexical key
with a parsed datetime key (e.g., using datetime.fromisoformat or
dateutil.parser.parse) so comparisons use actual UTC-aware datetimes; ensure the
parsed values are timezone-aware and comparable so the ordering is correct
across offsets.
In `@src/synthorg/backup/scheduler.py`:
- Around line 82-84: Before raising the ValueError for interval_hours in the
backup scheduler, add a log call that includes context (the invalid
interval_hours value and the function/operation) at WARNING or ERROR level; for
example, call the module/logger used in this file (e.g., logger.warning or
logger.error) with a message like "invalid backup interval_hours=%s: must be
>=1" and then raise ValueError(msg) as currently done, ensuring the log appears
immediately before the raise in the function that checks interval_hours.
In `@src/synthorg/backup/service.py`:
- Around line 650-663: The helper _try_load_entry_manifest performs blocking
filesystem calls (entry.is_dir, manifest_path.exists, manifest_path.read_text)
but is left async with a noqa; instead make it a synchronous helper (or keep as
is) and ensure callers (notably _load_manifest) invoke it via asyncio.to_thread
like _try_delete_backup does so the blocking I/O runs off the event loop; update
references to _try_load_entry_manifest in _load_manifest to call it via
asyncio.to_thread(..., args=(entry, backup_id)) and remove the ASYNC240
suppression, keeping BackupManifest and backup_id checks and the existing
warning handling intact.
- Around line 504-522: The async method list_backups performs blocking
filesystem iteration via self._backup_path.iterdir() and entry.is_dir(); wrap
these blocking ops in asyncio.to_thread to avoid blocking the event loop:
replace direct iteration with something like entries = await
asyncio.to_thread(list, self._backup_path.iterdir()) and use await
asyncio.to_thread(entry.is_dir) (or batch directory checks in the same to_thread
call) when deciding to call the synchronous _try_load_dir_info, while keeping
the existing await call to the async _try_load_archive_info for .tar.gz files;
update list_backups to collect entries from the thread and then proceed with the
same filtering, sorting, logging, and return logic.
In `@src/synthorg/settings/subscribers/backup_subscriber.py`:
- Around line 159-167: The call to scheduler.reschedule(hours) can raise
ValueError for invalid interval_hours (e.g., 0 or negative) and currently would
propagate without context; wrap the call to scheduler.reschedule(hours) in a
try/except that catches ValueError, log a warning or error (using
logger.warn/logger.error) including SETTINGS_SUBSCRIBER_NOTIFIED,
self.subscriber_name, namespace="backup", key="schedule_hours", the offending
hours value and a clear note, then re-raise the exception so callers still see
it; keep the successful logger.info(...) unchanged when no exception occurs.
---
Duplicate comments:
In `@src/synthorg/api/app.py`:
- Line 719: create_app calls _build_backup_service(effective_config) but does
not pass the actual resolved_db_path and resolved_config_path produced by the
auto-wiring logic, so the backup handler may use the original config values
instead of the files the app opened; update the call in create_app to pass the
resolved paths (resolved_db_path and resolved_config_path) returned/available
from the auto-wiring routine to _build_backup_service, and ensure the signature
used matches _build_backup_service(resolved_db_path, resolved_config_path,
effective_config) so backups target the real opened files.
🪄 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: ef11cc73-b35c-497d-98f8-36a29cd536f7
📒 Files selected for processing (41)
CLAUDE.mddocs/design/operations.mdsrc/synthorg/api/app.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/state.pysrc/synthorg/backup/__init__.pysrc/synthorg/backup/config.pysrc/synthorg/backup/errors.pysrc/synthorg/backup/handlers/__init__.pysrc/synthorg/backup/handlers/config_handler.pysrc/synthorg/backup/handlers/memory.pysrc/synthorg/backup/handlers/persistence.pysrc/synthorg/backup/handlers/protocol.pysrc/synthorg/backup/models.pysrc/synthorg/backup/retention.pysrc/synthorg/backup/scheduler.pysrc/synthorg/backup/service.pysrc/synthorg/config/defaults.pysrc/synthorg/config/schema.pysrc/synthorg/observability/events/backup.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/settings/subscribers/__init__.pysrc/synthorg/settings/subscribers/backup_subscriber.pytests/unit/api/controllers/test_backup.pytests/unit/api/test_app.pytests/unit/backup/__init__.pytests/unit/backup/conftest.pytests/unit/backup/test_config.pytests/unit/backup/test_errors.pytests/unit/backup/test_handlers/__init__.pytests/unit/backup/test_handlers/test_config_handler.pytests/unit/backup/test_handlers/test_memory_handler.pytests/unit/backup/test_handlers/test_persistence_handler.pytests/unit/backup/test_models.pytests/unit/backup/test_retention.pytests/unit/backup/test_scheduler.pytests/unit/backup/test_service.pytests/unit/config/conftest.pytests/unit/observability/test_events.pytests/unit/settings/test_backup_subscriber.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 Web
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Always read the relevant
docs/design/page before implementing any feature or planning any issue. The design spec (DESIGN_SPEC.md) is a pointer file linking to 7 design pages and is the starting point for architecture, data models, and behavior.
Files:
docs/design/operations.mdCLAUDE.md
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Pre-commit hooks enforce: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint (Dockerfile linting)
Files:
docs/design/operations.mdsrc/synthorg/backup/handlers/memory.pytests/unit/backup/test_models.pysrc/synthorg/config/defaults.pysrc/synthorg/backup/config.pysrc/synthorg/settings/subscribers/backup_subscriber.pytests/unit/backup/test_errors.pysrc/synthorg/api/controllers/backup.pytests/unit/api/test_app.pysrc/synthorg/backup/handlers/persistence.pysrc/synthorg/config/schema.pysrc/synthorg/backup/scheduler.pysrc/synthorg/settings/definitions/backup.pytests/unit/backup/test_service.pysrc/synthorg/backup/retention.pysrc/synthorg/backup/service.pytests/unit/backup/test_handlers/test_persistence_handler.pyCLAUDE.mdsrc/synthorg/api/controllers/__init__.pytests/unit/observability/test_events.pytests/unit/api/controllers/test_backup.pysrc/synthorg/observability/events/backup.pysrc/synthorg/settings/subscribers/__init__.pysrc/synthorg/backup/errors.pytests/unit/settings/test_backup_subscriber.pytests/unit/backup/test_config.pysrc/synthorg/backup/handlers/protocol.pysrc/synthorg/backup/handlers/config_handler.pytests/unit/backup/test_handlers/test_config_handler.pysrc/synthorg/backup/handlers/__init__.pysrc/synthorg/api/state.pytests/unit/backup/conftest.pysrc/synthorg/backup/models.pysrc/synthorg/backup/__init__.pysrc/synthorg/api/app.pytests/unit/backup/test_retention.pytests/unit/backup/test_scheduler.pytests/unit/config/conftest.pytests/unit/backup/test_handlers/test_memory_handler.py
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Design spec pages are the mandatory starting point for architecture, data models, and behavior. The 7 design pages cover: index, agents, organization, communication, engine, memory, operations.
Files:
docs/design/operations.md
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Use PEP 758 except syntax: useexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
All public functions must have type hints; mypy strict mode is enforced
Docstrings must be in Google style and are required on all public classes and functions (enforced by ruff D rules)
Create new objects instead of mutating existing ones. For non-Pydantic internal collections (registries, BaseTool), usecopy.deepcopy()at construction and wrap withMappingProxyTypefor read-only enforcement. For dict/list fields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.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 conventions:BaseModel,model_validator,computed_field,ConfigDict. Use@computed_fieldfor derived values instead of storing redundant fields. UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional and tuple variants — instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations (e.g. multiple tool invocations, parallel agent calls). Use structured concurrency over barecreate_task.
Line length must be 88 characters (enforced by ruff)
Functions must be < 50 lines, files < 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 have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport logging/logging.getLogger()/print()in ap...
Files:
src/synthorg/backup/handlers/memory.pysrc/synthorg/config/defaults.pysrc/synthorg/backup/config.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/backup/handlers/persistence.pysrc/synthorg/config/schema.pysrc/synthorg/backup/scheduler.pysrc/synthorg/settings/definitions/backup.pysrc/synthorg/backup/retention.pysrc/synthorg/backup/service.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/observability/events/backup.pysrc/synthorg/settings/subscribers/__init__.pysrc/synthorg/backup/errors.pysrc/synthorg/backup/handlers/protocol.pysrc/synthorg/backup/handlers/config_handler.pysrc/synthorg/backup/handlers/__init__.pysrc/synthorg/api/state.pysrc/synthorg/backup/models.pysrc/synthorg/backup/__init__.pysrc/synthorg/api/app.py
**/*.{js,ts,py,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Commits must follow format:
<type>: <description>with types: feat, fix, refactor, docs, test, chore, perf, ci. Enforced by commitizen (commit-msg hook). Signed commits are required on main via branch protection — all commits must be GPG/SSH signed.
Files:
src/synthorg/backup/handlers/memory.pytests/unit/backup/test_models.pysrc/synthorg/config/defaults.pysrc/synthorg/backup/config.pysrc/synthorg/settings/subscribers/backup_subscriber.pytests/unit/backup/test_errors.pysrc/synthorg/api/controllers/backup.pytests/unit/api/test_app.pysrc/synthorg/backup/handlers/persistence.pysrc/synthorg/config/schema.pysrc/synthorg/backup/scheduler.pysrc/synthorg/settings/definitions/backup.pytests/unit/backup/test_service.pysrc/synthorg/backup/retention.pysrc/synthorg/backup/service.pytests/unit/backup/test_handlers/test_persistence_handler.pysrc/synthorg/api/controllers/__init__.pytests/unit/observability/test_events.pytests/unit/api/controllers/test_backup.pysrc/synthorg/observability/events/backup.pysrc/synthorg/settings/subscribers/__init__.pysrc/synthorg/backup/errors.pytests/unit/settings/test_backup_subscriber.pytests/unit/backup/test_config.pysrc/synthorg/backup/handlers/protocol.pysrc/synthorg/backup/handlers/config_handler.pytests/unit/backup/test_handlers/test_config_handler.pysrc/synthorg/backup/handlers/__init__.pysrc/synthorg/api/state.pytests/unit/backup/conftest.pysrc/synthorg/backup/models.pysrc/synthorg/backup/__init__.pysrc/synthorg/api/app.pytests/unit/backup/test_retention.pytests/unit/backup/test_scheduler.pytests/unit/config/conftest.pytests/unit/backup/test_handlers/test_memory_handler.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Scripts in
scripts/directory have relaxed ruff rules:print()and deferred imports are allowed
Files:
src/synthorg/backup/handlers/memory.pytests/unit/backup/test_models.pysrc/synthorg/config/defaults.pysrc/synthorg/backup/config.pysrc/synthorg/settings/subscribers/backup_subscriber.pytests/unit/backup/test_errors.pysrc/synthorg/api/controllers/backup.pytests/unit/api/test_app.pysrc/synthorg/backup/handlers/persistence.pysrc/synthorg/config/schema.pysrc/synthorg/backup/scheduler.pysrc/synthorg/settings/definitions/backup.pytests/unit/backup/test_service.pysrc/synthorg/backup/retention.pysrc/synthorg/backup/service.pytests/unit/backup/test_handlers/test_persistence_handler.pysrc/synthorg/api/controllers/__init__.pytests/unit/observability/test_events.pytests/unit/api/controllers/test_backup.pysrc/synthorg/observability/events/backup.pysrc/synthorg/settings/subscribers/__init__.pysrc/synthorg/backup/errors.pytests/unit/settings/test_backup_subscriber.pytests/unit/backup/test_config.pysrc/synthorg/backup/handlers/protocol.pysrc/synthorg/backup/handlers/config_handler.pytests/unit/backup/test_handlers/test_config_handler.pysrc/synthorg/backup/handlers/__init__.pysrc/synthorg/api/state.pytests/unit/backup/conftest.pysrc/synthorg/backup/models.pysrc/synthorg/backup/__init__.pysrc/synthorg/api/app.pytests/unit/backup/test_retention.pytests/unit/backup/test_scheduler.pytests/unit/config/conftest.pytests/unit/backup/test_handlers/test_memory_handler.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Coverage minimum is 80% (enforced in CI). Async usesasyncio_mode = "auto"— no manual@pytest.mark.asyncioneeded. Test timeout is 30 seconds per test. Usepytest-xdistvia-n auto— ALWAYS include-n autowhen running pytest, never run tests sequentially.
Prefer@pytest.mark.parametrizefor testing similar cases
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned test code, docstrings, or config examples. Use generic names:example-provider,example-large-001,example-medium-001,example-small-001. Tests must usetest-provider,test-small-001, etc. Vendor names may only appear in third-party import paths.
Use Hypothesis for property-based testing with@given+@settings. Profiles:ci(200 examples, default) anddev(1000 examples), controlled viaHYPOTHESIS_PROFILEenv var. Run dev profile:HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties..hypothesis/is gitignored.
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins.
Files:
tests/unit/backup/test_models.pytests/unit/backup/test_errors.pytests/unit/api/test_app.pytests/unit/backup/test_service.pytests/unit/backup/test_handlers/test_persistence_handler.pytests/unit/observability/test_events.pytests/unit/api/controllers/test_backup.pytests/unit/settings/test_backup_subscriber.pytests/unit/backup/test_config.pytests/unit/backup/test_handlers/test_config_handler.pytests/unit/backup/conftest.pytests/unit/backup/test_retention.pytests/unit/backup/test_scheduler.pytests/unit/config/conftest.pytests/unit/backup/test_handlers/test_memory_handler.py
🧠 Learnings (19)
📚 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:
docs/design/operations.md
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to src/**/*.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/backup/config.pysrc/synthorg/backup/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 : Config vs runtime state: frozen Pydantic models for config/identity; 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/backup/config.py
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to src/**/*.py : All error paths must log at WARNING or ERROR with context before raising. All state transitions must log at INFO. DEBUG for object creation, internal flow, entry/exit of key functions.
Applied to files:
src/synthorg/backup/config.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/backup/handlers/persistence.pyCLAUDE.mdsrc/synthorg/backup/handlers/config_handler.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/backup/config.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/backup/handlers/persistence.pyCLAUDE.mdsrc/synthorg/backup/handlers/config_handler.py
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to src/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. Use `computed_field` for derived values instead of storing redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional and tuple variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/config/schema.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare create_task.
Applied to files:
src/synthorg/backup/service.py
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to src/**/*.py : Use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`. Use structured logging: `logger.info(EVENT, key=value)` — never string formatting.
Applied to files:
CLAUDE.mdsrc/synthorg/observability/events/backup.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:
CLAUDE.mdsrc/synthorg/observability/events/backup.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.md
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to src/**/*.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 must be `logger` (not `_logger`, not `log`).
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 : 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 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.md
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to src/**/*.py : Pure data models, enums, and re-exports do NOT need logging
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 timeout: 30 seconds per test.
Applied to files:
tests/unit/api/controllers/test_backup.pytests/unit/settings/test_backup_subscriber.py
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to tests/**/*.py : Coverage minimum is 80% (enforced in CI). Async uses `asyncio_mode = "auto"` — no manual `pytest.mark.asyncio` needed. Test timeout is 30 seconds per test. Use `pytest-xdist` via `-n auto` — ALWAYS include `-n auto` when running pytest, never run tests sequentially.
Applied to files:
tests/unit/api/controllers/test_backup.pytests/unit/settings/test_backup_subscriber.pytests/unit/backup/test_scheduler.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.e2e, pytest.mark.slow. Coverage: 80% minimum (enforced in CI).
Applied to files:
tests/unit/api/controllers/test_backup.pytests/unit/settings/test_backup_subscriber.py
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to tests/**/*.py : NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins.
Applied to files:
tests/unit/api/controllers/test_backup.pytests/unit/settings/test_backup_subscriber.py
🧬 Code graph analysis (25)
src/synthorg/backup/handlers/memory.py (5)
src/synthorg/backup/handlers/config_handler.py (4)
backup(41-88)component(37-39)restore(90-141)validate_source(143-153)src/synthorg/backup/handlers/persistence.py (5)
backup(41-79)component(37-39)restore(81-122)_atomic_swap(181-218)validate_source(124-148)src/synthorg/backup/handlers/protocol.py (4)
backup(28-37)component(24-26)restore(39-45)validate_source(47-56)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(30-35)
src/synthorg/backup/config.py (5)
src/synthorg/backup/handlers/config_handler.py (1)
backup(41-88)src/synthorg/backup/handlers/memory.py (1)
backup(43-87)src/synthorg/backup/handlers/persistence.py (1)
backup(41-79)src/synthorg/backup/handlers/protocol.py (1)
backup(28-37)src/synthorg/backup/models.py (1)
BackupComponent(30-35)
src/synthorg/settings/subscribers/backup_subscriber.py (2)
src/synthorg/backup/service.py (3)
scheduler(103-105)start(117-120)stop(122-124)src/synthorg/backup/scheduler.py (4)
is_running(38-40)start(42-58)stop(60-68)reschedule(70-90)
tests/unit/backup/test_errors.py (1)
src/synthorg/backup/errors.py (7)
BackupError(8-9)BackupInProgressError(12-13)BackupNotFoundError(32-33)ComponentBackupError(24-25)ManifestError(20-21)RestoreError(16-17)RetentionError(28-29)
src/synthorg/api/controllers/backup.py (4)
src/synthorg/backup/handlers/protocol.py (1)
backup(28-37)src/synthorg/backup/errors.py (5)
BackupError(8-9)BackupInProgressError(12-13)BackupNotFoundError(32-33)ManifestError(20-21)RestoreError(16-17)src/synthorg/backup/models.py (5)
BackupInfo(89-128)BackupManifest(38-86)BackupTrigger(20-27)RestoreRequest(131-157)RestoreResponse(160-175)src/synthorg/backup/service.py (5)
create_backup(126-153)list_backups(498-522)get_backup(564-577)delete_backup(579-596)restore_from_backup(314-343)
tests/unit/api/test_app.py (1)
src/synthorg/api/app.py (2)
_safe_startup(380-508)_safe_shutdown(511-580)
src/synthorg/backup/handlers/persistence.py (4)
src/synthorg/backup/handlers/memory.py (5)
backup(43-87)component(39-41)restore(89-133)_atomic_swap(158-177)validate_source(135-145)src/synthorg/backup/handlers/protocol.py (4)
backup(28-37)component(24-26)restore(39-45)validate_source(47-56)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(30-35)
src/synthorg/backup/scheduler.py (2)
src/synthorg/backup/models.py (1)
BackupTrigger(20-27)src/synthorg/backup/service.py (4)
BackupService(71-838)start(117-120)stop(122-124)create_backup(126-153)
src/synthorg/settings/definitions/backup.py (3)
src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)src/synthorg/settings/enums.py (3)
SettingNamespace(6-21)SettingType(24-36)SettingLevel(39-47)
tests/unit/backup/test_service.py (8)
src/synthorg/backup/handlers/memory.py (4)
backup(43-87)component(39-41)restore(89-133)validate_source(135-145)src/synthorg/backup/handlers/persistence.py (4)
backup(41-79)component(37-39)restore(81-122)validate_source(124-148)src/synthorg/backup/handlers/protocol.py (4)
backup(28-37)component(24-26)restore(39-45)validate_source(47-56)src/synthorg/backup/config.py (2)
BackupConfig(34-107)RetentionConfig(20-31)src/synthorg/backup/errors.py (2)
BackupInProgressError(12-13)BackupNotFoundError(32-33)src/synthorg/backup/models.py (3)
BackupComponent(30-35)BackupManifest(38-86)BackupTrigger(20-27)tests/unit/backup/conftest.py (1)
backup_path(60-64)src/synthorg/backup/scheduler.py (1)
start(42-58)
src/synthorg/backup/retention.py (3)
src/synthorg/backup/errors.py (1)
RetentionError(28-29)src/synthorg/backup/models.py (2)
BackupManifest(38-86)BackupTrigger(20-27)src/synthorg/backup/config.py (1)
RetentionConfig(20-31)
src/synthorg/backup/service.py (10)
src/synthorg/backup/handlers/config_handler.py (4)
backup(41-88)component(37-39)restore(90-141)validate_source(143-153)src/synthorg/backup/handlers/memory.py (4)
backup(43-87)component(39-41)restore(89-133)validate_source(135-145)src/synthorg/backup/handlers/persistence.py (4)
backup(41-79)component(37-39)restore(81-122)validate_source(124-148)src/synthorg/backup/handlers/protocol.py (5)
backup(28-37)ComponentHandler(16-56)component(24-26)restore(39-45)validate_source(47-56)src/synthorg/backup/errors.py (4)
BackupInProgressError(12-13)BackupNotFoundError(32-33)ManifestError(20-21)RestoreError(16-17)src/synthorg/backup/models.py (5)
BackupComponent(30-35)BackupInfo(89-128)BackupManifest(38-86)BackupTrigger(20-27)from_manifest(111-128)src/synthorg/backup/retention.py (2)
RetentionManager(27-242)prune(42-71)src/synthorg/backup/scheduler.py (3)
BackupScheduler(23-115)start(42-58)stop(60-68)src/synthorg/api/app.py (2)
on_startup(217-234)on_shutdown(236-252)src/synthorg/api/controllers/backup.py (4)
create_backup(52-86)list_backups(89-112)get_backup(115-138)delete_backup(141-164)
tests/unit/backup/test_handlers/test_persistence_handler.py (3)
src/synthorg/backup/handlers/persistence.py (6)
backup(41-79)PersistenceComponentHandler(23-218)component(37-39)restore(81-122)validate_source(124-148)_check_integrity(166-170)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(30-35)
src/synthorg/api/controllers/__init__.py (5)
src/synthorg/backup/handlers/config_handler.py (1)
backup(41-88)src/synthorg/backup/handlers/memory.py (1)
backup(43-87)src/synthorg/backup/handlers/persistence.py (1)
backup(41-79)src/synthorg/backup/handlers/protocol.py (1)
backup(28-37)src/synthorg/api/controllers/backup.py (1)
BackupController(41-228)
src/synthorg/settings/subscribers/__init__.py (1)
src/synthorg/settings/subscribers/backup_subscriber.py (1)
BackupSettingsSubscriber(27-167)
tests/unit/settings/test_backup_subscriber.py (4)
src/synthorg/settings/subscriber.py (1)
SettingsSubscriber(11-55)src/synthorg/settings/subscribers/backup_subscriber.py (4)
BackupSettingsSubscriber(27-167)watched_keys(48-50)subscriber_name(53-55)on_settings_changed(57-92)src/synthorg/backup/service.py (3)
scheduler(103-105)start(117-120)stop(122-124)src/synthorg/backup/scheduler.py (4)
is_running(38-40)start(42-58)stop(60-68)reschedule(70-90)
src/synthorg/backup/handlers/protocol.py (4)
src/synthorg/backup/handlers/config_handler.py (4)
backup(41-88)component(37-39)restore(90-141)validate_source(143-153)src/synthorg/backup/handlers/memory.py (4)
backup(43-87)component(39-41)restore(89-133)validate_source(135-145)src/synthorg/backup/handlers/persistence.py (4)
backup(41-79)component(37-39)restore(81-122)validate_source(124-148)src/synthorg/backup/models.py (1)
BackupComponent(30-35)
src/synthorg/backup/handlers/config_handler.py (5)
src/synthorg/backup/handlers/memory.py (3)
backup(43-87)component(39-41)restore(89-133)src/synthorg/backup/handlers/persistence.py (3)
backup(41-79)component(37-39)restore(81-122)src/synthorg/backup/handlers/protocol.py (3)
backup(28-37)component(24-26)restore(39-45)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(30-35)
tests/unit/backup/test_handlers/test_config_handler.py (6)
src/synthorg/backup/handlers/config_handler.py (5)
backup(41-88)ConfigComponentHandler(24-169)component(37-39)restore(90-141)validate_source(143-153)src/synthorg/backup/handlers/memory.py (4)
backup(43-87)component(39-41)restore(89-133)validate_source(135-145)src/synthorg/backup/handlers/persistence.py (4)
backup(41-79)component(37-39)restore(81-122)validate_source(124-148)src/synthorg/backup/handlers/protocol.py (4)
backup(28-37)component(24-26)restore(39-45)validate_source(47-56)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(30-35)
src/synthorg/api/state.py (1)
src/synthorg/backup/service.py (1)
BackupService(71-838)
tests/unit/backup/conftest.py (2)
src/synthorg/backup/config.py (2)
BackupConfig(34-107)RetentionConfig(20-31)src/synthorg/backup/models.py (3)
BackupComponent(30-35)BackupManifest(38-86)BackupTrigger(20-27)
src/synthorg/backup/__init__.py (2)
src/synthorg/backup/config.py (2)
BackupConfig(34-107)RetentionConfig(20-31)src/synthorg/backup/service.py (1)
BackupService(71-838)
tests/unit/backup/test_scheduler.py (3)
src/synthorg/backup/models.py (1)
BackupTrigger(20-27)src/synthorg/backup/service.py (4)
scheduler(103-105)create_backup(126-153)start(117-120)stop(122-124)src/synthorg/backup/scheduler.py (6)
BackupScheduler(23-115)is_running(38-40)start(42-58)stop(60-68)reschedule(70-90)_run_loop(92-115)
tests/unit/config/conftest.py (1)
src/synthorg/backup/config.py (1)
BackupConfig(34-107)
tests/unit/backup/test_handlers/test_memory_handler.py (3)
src/synthorg/backup/handlers/memory.py (5)
backup(43-87)MemoryComponentHandler(24-177)component(39-41)restore(89-133)validate_source(135-145)src/synthorg/backup/errors.py (1)
ComponentBackupError(24-25)src/synthorg/backup/models.py (1)
BackupComponent(30-35)
🪛 LanguageTool
docs/design/operations.md
[style] ~1091-~1091: Using four (or more) nouns in a row may decrease readability.
Context: ...util.copytreewithsymlinks=Truefor agent memory data directory -ConfigComponentHandler: shutil.c...
(FOUR_NN)
CLAUDE.md
[style] ~202-~202: Since ownership is already implied, this phrasing may be redundant.
Context: ... from events.backup). Each domain has its own module -- see `src/synthorg/observabili...
(PRP_OWN)
| - `MemoryComponentHandler`: `shutil.copytree` with `symlinks=True` for agent memory data directory | ||
| - `ConfigComponentHandler`: `shutil.copy2` for company YAML configuration |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rephrase the noun-heavy phrase for readability.
The phrase “agent memory data directory” is dense; a small rewrite improves scanability in docs.
✍️ Suggested wording
- - `MemoryComponentHandler`: `shutil.copytree` with `symlinks=True` for agent memory data directory
+ - `MemoryComponentHandler`: `shutil.copytree` with `symlinks=True` for the agent-memory directory📝 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.
| - `MemoryComponentHandler`: `shutil.copytree` with `symlinks=True` for agent memory data directory | |
| - `ConfigComponentHandler`: `shutil.copy2` for company YAML configuration | |
| - `MemoryComponentHandler`: `shutil.copytree` with `symlinks=True` for the agent-memory directory | |
| - `ConfigComponentHandler`: `shutil.copy2` for company YAML configuration |
🧰 Tools
🪛 LanguageTool
[style] ~1091-~1091: Using four (or more) nouns in a row may decrease readability.
Context: ...util.copytreewithsymlinks=Truefor agent memory data directory -ConfigComponentHandler: shutil.c...
(FOUR_NN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/operations.md` around lines 1091 - 1092, Reword the dense phrase
"agent memory data directory" in the docs entry for MemoryComponentHandler to
make it more readable and scannable; for example, replace it with "the directory
that stores an agent's memory data" (or "agent's memory directory") in the list
item referencing MemoryComponentHandler (alongside the existing
ConfigComponentHandler entry) so the line reads clearly while preserving the
mention of shutil.copytree and symlinks=True.
| @model_validator(mode="after") | ||
| def _reject_duplicate_components(self) -> Self: | ||
| """Reject duplicate entries in the include tuple.""" | ||
| if len(self.include) != len(set(self.include)): | ||
| msg = "Duplicate components in include" | ||
| raise ValueError(msg) | ||
| return self |
There was a problem hiding this comment.
Log before raising on duplicate components validation failure.
The _reject_duplicate_components validator raises ValueError without logging context first. Per coding guidelines, all error paths must log at WARNING or ERROR with context before raising.
🛡️ Proposed fix
`@model_validator`(mode="after")
def _reject_duplicate_components(self) -> Self:
"""Reject duplicate entries in the include tuple."""
if len(self.include) != len(set(self.include)):
msg = "Duplicate components in include"
+ logger.warning(
+ CONFIG_VALIDATION_FAILED,
+ field="include",
+ value=str(self.include),
+ reason=msg,
+ )
raise ValueError(msg)
return selfBased on learnings: "All error paths must log at WARNING or ERROR with context before raising."
📝 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.
| @model_validator(mode="after") | |
| def _reject_duplicate_components(self) -> Self: | |
| """Reject duplicate entries in the include tuple.""" | |
| if len(self.include) != len(set(self.include)): | |
| msg = "Duplicate components in include" | |
| raise ValueError(msg) | |
| return self | |
| `@model_validator`(mode="after") | |
| def _reject_duplicate_components(self) -> Self: | |
| """Reject duplicate entries in the include tuple.""" | |
| if len(self.include) != len(set(self.include)): | |
| msg = "Duplicate components in include" | |
| logger.warning( | |
| CONFIG_VALIDATION_FAILED, | |
| field="include", | |
| value=str(self.include), | |
| reason=msg, | |
| ) | |
| raise ValueError(msg) | |
| return self |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/config.py` around lines 101 - 107, The
_reject_duplicate_components model_validator currently raises ValueError when
duplicate entries are found but doesn't log; update the function
_reject_duplicate_components to log the validation failure (including the
offending self.include contents and a clear message) at WARNING or ERROR level
before raising; use the module-level logger (e.g., logger) or the existing
logger instance to emit the log entry with context and then raise
ValueError(msg) as before.
| if not source_file.exists(): | ||
| backup_files = list(config_dir.iterdir()) | ||
| if not backup_files: | ||
| logger.warning( | ||
| BACKUP_COMPONENT_FAILED, | ||
| component=self.component.value, | ||
| error=f"No config files found in backup: {config_dir}", | ||
| ) | ||
| msg = f"No config files found in backup: {config_dir}" | ||
| raise ComponentBackupError(msg) | ||
| source_file = backup_files[0] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Synchronous I/O calls in async method.
source_file.exists() at line 115 and list(config_dir.iterdir()) at line 116 perform blocking filesystem I/O directly in the async context. While these are typically fast operations, wrapping them in asyncio.to_thread would be consistent with the pattern used elsewhere in this file (e.g., line 103).
♻️ Proposed fix for consistency
# Look for the original filename first, fall back to first file
source_file = config_dir / self._config_path.name
- if not source_file.exists():
- backup_files = list(config_dir.iterdir())
+ exists = await asyncio.to_thread(source_file.exists)
+ if not exists:
+ backup_files = await asyncio.to_thread(
+ lambda: list(config_dir.iterdir())
+ )📝 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.
| if not source_file.exists(): | |
| backup_files = list(config_dir.iterdir()) | |
| if not backup_files: | |
| logger.warning( | |
| BACKUP_COMPONENT_FAILED, | |
| component=self.component.value, | |
| error=f"No config files found in backup: {config_dir}", | |
| ) | |
| msg = f"No config files found in backup: {config_dir}" | |
| raise ComponentBackupError(msg) | |
| source_file = backup_files[0] | |
| exists = await asyncio.to_thread(source_file.exists) | |
| if not exists: | |
| backup_files = await asyncio.to_thread( | |
| lambda: list(config_dir.iterdir()) | |
| ) | |
| if not backup_files: | |
| logger.warning( | |
| BACKUP_COMPONENT_FAILED, | |
| component=self.component.value, | |
| error=f"No config files found in backup: {config_dir}", | |
| ) | |
| msg = f"No config files found in backup: {config_dir}" | |
| raise ComponentBackupError(msg) | |
| source_file = backup_files[0] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/handlers/config_handler.py` around lines 115 - 125, The
code does blocking filesystem I/O by calling source_file.exists() and
list(config_dir.iterdir()) directly in an async context; change these to run on
a thread (e.g., wrap both calls with asyncio.to_thread or the event loop's
run_in_executor) so they do not block the event loop — locate the block that
references source_file, config_dir, BACKUP_COMPONENT_FAILED, logger and
ComponentBackupError and replace the synchronous exists() and iterdir() usage
with asyncio.to_thread calls that return the boolean/extracted list before
proceeding with the existing warning and exception logic.
| backup_id: NotBlankStr | ||
|
|
There was a problem hiding this comment.
Validate backup-id format at all model boundaries.
BackupManifest.backup_id and RestoreResponse.safety_backup_id are currently only NotBlankStr; malformed IDs can leak deeper into restore/delete flows.
🛡️ Proposed fix
class BackupManifest(BaseModel):
@@
checksum: NotBlankStr
backup_id: NotBlankStr
+
+ `@field_validator`("backup_id")
+ `@classmethod`
+ def _validate_backup_id(cls, v: str) -> str:
+ """Validate backup ID format (12-char lowercase hex)."""
+ if not _BACKUP_ID_RE.match(v):
+ msg = f"backup_id must be a 12-character hex string, got: {v!r}"
+ raise ValueError(msg)
+ return v
@@
class RestoreResponse(BaseModel):
@@
restored_components: tuple[BackupComponent, ...]
safety_backup_id: NotBlankStr
restart_required: bool = True
+
+ `@field_validator`("safety_backup_id")
+ `@classmethod`
+ def _validate_safety_backup_id(cls, v: str) -> str:
+ """Validate safety backup ID format (12-char lowercase hex)."""
+ if not _BACKUP_ID_RE.match(v):
+ msg = f"safety_backup_id must be a 12-character hex string, got: {v!r}"
+ raise ValueError(msg)
+ return vAlso applies to: 174-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/models.py` around lines 66 - 67, BackupManifest.backup_id
and RestoreResponse.safety_backup_id are only typed as NotBlankStr and need
format validation at model boundaries to prevent malformed IDs from propagating;
add a stricter type or validator (e.g., create/replace NotBlankStr with
BackupIDStr or attach a pydantic/validator function) that enforces the backup-id
regex/format and use it for both BackupManifest.backup_id and
RestoreResponse.safety_backup_id (also update the other occurrence mentioned
around lines 174-175) so any bad ID is rejected early at model construction.
| @field_validator("timestamp") | ||
| @classmethod | ||
| def _validate_timestamp(cls, v: str) -> str: | ||
| """Reject timestamps that are not valid ISO 8601.""" | ||
| try: | ||
| datetime.fromisoformat(v) | ||
| except ValueError as exc: | ||
| msg = f"Invalid ISO 8601 timestamp: {v}" | ||
| raise ValueError(msg) from exc | ||
| return v |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f . src/synthorg/backup --max-one-resultRepository: Aureliolo/synthorg
Length of output: 232
🏁 Script executed:
git ls-files src/synthorg/backup/Repository: Aureliolo/synthorg
Length of output: 492
🏁 Script executed:
cat -n src/synthorg/backup/models.py | head -100Repository: Aureliolo/synthorg
Length of output: 3826
🏁 Script executed:
cat -n src/synthorg/backup/retention.pyRepository: Aureliolo/synthorg
Length of output: 9842
Enforce timezone-aware manifest timestamps to prevent TypeError in retention age checks.
The timestamp validator on line 70 accepts naive ISO 8601 strings. However, RetentionManager._should_prune (retention.py:103–104) parses the timestamp and subtracts it from datetime.now(UTC), which is timezone-aware. Mixing naive and aware datetimes raises TypeError, which goes uncaught by the specific ValueError handler and is misattributed as a manifest load failure.
Fix
`@field_validator`("timestamp")
`@classmethod`
def _validate_timestamp(cls, v: str) -> str:
"""Reject timestamps that are not valid ISO 8601."""
try:
- datetime.fromisoformat(v)
+ parsed = datetime.fromisoformat(v)
except ValueError as exc:
msg = f"Invalid ISO 8601 timestamp: {v}"
raise ValueError(msg) from exc
+ if parsed.tzinfo is None or parsed.tzinfo.utcoffset(parsed) is None:
+ msg = f"Timestamp must include timezone offset: {v}"
+ raise ValueError(msg)
return v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/models.py` around lines 68 - 77, The timestamp validator
_validate_timestamp currently accepts naive ISO 8601 strings causing a TypeError
later in RetentionManager._should_prune when subtracting from datetime.now(UTC);
update _validate_timestamp (the field_validator for "timestamp") to parse the
string with datetime.fromisoformat (or equivalent) and verify that the resulting
datetime.tzinfo is not None (i.e., timezone-aware), and if it is naive raise a
ValueError with a clear message like "Timestamp must include timezone/offset";
keep the existing ValueError handling for invalid formats so callers see a
single manifest-validation error for both format and timezone issues.
| # Sort by timestamp descending (newest first) | ||
| manifests.sort(key=lambda m: m.timestamp, reverse=True) | ||
| candidates = self._identify_prunable(manifests) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/synthorg/backup/retention.pyRepository: Aureliolo/synthorg
Length of output: 9842
🏁 Script executed:
find . -name "*.py" -type f -exec grep -l "class BackupManifest" {} \;Repository: Aureliolo/synthorg
Length of output: 93
🏁 Script executed:
cat -n src/synthorg/backup/models.pyRepository: Aureliolo/synthorg
Length of output: 6767
🏁 Script executed:
python3 << 'EOF'
# Demonstrate the issue with string-based ISO 8601 sorting with different timezone offsets
timestamps = [
"2025-03-01T10:00:00+02:00", # 2025-03-01 08:00:00 UTC
"2025-03-01T11:00:00+03:00", # 2025-03-01 08:00:00 UTC (same moment in time)
"2025-03-01T09:00:00+01:00", # 2025-03-01 08:00:00 UTC (same moment in time)
"2025-03-01T12:00:00+00:00", # 2025-03-01 12:00:00 UTC (later)
]
print("String-based sort (incorrect):")
sorted_strings = sorted(timestamps, reverse=True)
for ts in sorted_strings:
print(f" {ts}")
print("\nDatetime-based sort (correct):")
from datetime import datetime, UTC
sorted_datetimes = sorted(
timestamps,
key=lambda ts: datetime.fromisoformat(ts).astimezone(UTC),
reverse=True
)
for ts in sorted_datetimes:
print(f" {ts}")
print(f" -> UTC: {datetime.fromisoformat(ts).astimezone(UTC)}")
EOFRepository: Aureliolo/synthorg
Length of output: 500
🏁 Script executed:
# Search for where timestamps are created in the backup system
rg "timestamp" src/synthorg/backup --type py -B 2 -A 2 | grep -E "(datetime|now|timestamp\s*=)" | head -30Repository: Aureliolo/synthorg
Length of output: 759
🏁 Script executed:
cat -n src/synthorg/backup/service.py | grep -A 5 -B 5 "timestamp = datetime"Repository: Aureliolo/synthorg
Length of output: 585
Sort manifests by parsed datetime to handle variable timezone offsets.
Line 69 uses lexical string sort of ISO 8601 timestamps. While current code generates all timestamps in UTC (so string sorting happens to work), this is fragile: if timestamps with different timezone offsets ever exist, lexical comparison fails. For example, 2025-03-01T11:00:00+03:00 (08:00 UTC) would sort after 2025-03-01T10:00:00+02:00 (08:00 UTC) lexically, risking incorrect pruning of the "newest" backup.
Proposed fix
- # Sort by timestamp descending (newest first)
- manifests.sort(key=lambda m: m.timestamp, reverse=True)
+ # Sort by actual instant descending (newest first)
+ manifests.sort(
+ key=lambda m: datetime.fromisoformat(m.timestamp).astimezone(UTC),
+ reverse=True,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/retention.py` around lines 68 - 70, Manifests are being
sorted by their ISO string timestamps which fails when timezone offsets vary;
change the sort to parse each manifest.timestamp into a timezone-aware datetime
and sort by that datetime (newest first) before calling
self._identify_prunable(manifests). Locate the sort around the manifests
variable in retention.py and replace the lexical key with a parsed datetime key
(e.g., using datetime.fromisoformat or dateutil.parser.parse) so comparisons use
actual UTC-aware datetimes; ensure the parsed values are timezone-aware and
comparable so the ordering is correct across offsets.
| if interval_hours < 1: | ||
| msg = "interval_hours must be >= 1" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Log before raising ValueError.
The ValueError is raised without prior logging. Per coding guidelines, all error paths must log at WARNING or ERROR with context before raising.
🛡️ Proposed fix
if interval_hours < 1:
msg = "interval_hours must be >= 1"
+ logger.warning(
+ BACKUP_SCHEDULER_RESCHEDULED,
+ interval_hours=interval_hours,
+ error=msg,
+ )
raise ValueError(msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/scheduler.py` around lines 82 - 84, Before raising the
ValueError for interval_hours in the backup scheduler, add a log call that
includes context (the invalid interval_hours value and the function/operation)
at WARNING or ERROR level; for example, call the module/logger used in this file
(e.g., logger.warning or logger.error) with a message like "invalid backup
interval_hours=%s: must be >=1" and then raise ValueError(msg) as currently
done, ensuring the log appears immediately before the raise in the function that
checks interval_hours.
| if not self._backup_path.exists(): | ||
| logger.debug(BACKUP_LISTED, count=0) | ||
| return () | ||
|
|
||
| infos: list[BackupInfo] = [] | ||
|
|
||
| for entry in self._backup_path.iterdir(): | ||
| if entry.is_dir(): | ||
| info = self._try_load_dir_info(entry) | ||
| if info is not None: | ||
| infos.append(info) | ||
| elif entry.name.endswith(".tar.gz"): | ||
| info = await self._try_load_archive_info(entry) | ||
| if info is not None: | ||
| infos.append(info) | ||
|
|
||
| infos.sort(key=lambda i: i.timestamp, reverse=True) | ||
| logger.debug(BACKUP_LISTED, count=len(infos)) | ||
| return tuple(infos) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Synchronous directory iteration in async method.
self._backup_path.iterdir() at line 510 performs blocking I/O directly in the async list_backups method. For directories with many backups, this could block the event loop noticeably. Consider wrapping the iteration in asyncio.to_thread.
♻️ Proposed fix
async def list_backups(self) -> tuple[BackupInfo, ...]:
...
- if not self._backup_path.exists():
+ exists = await asyncio.to_thread(self._backup_path.exists)
+ if not exists:
logger.debug(BACKUP_LISTED, count=0)
return ()
infos: list[BackupInfo] = []
- for entry in self._backup_path.iterdir():
+ entries = await asyncio.to_thread(list, self._backup_path.iterdir())
+ for entry in entries:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/service.py` around lines 504 - 522, The async method
list_backups performs blocking filesystem iteration via
self._backup_path.iterdir() and entry.is_dir(); wrap these blocking ops in
asyncio.to_thread to avoid blocking the event loop: replace direct iteration
with something like entries = await asyncio.to_thread(list,
self._backup_path.iterdir()) and use await asyncio.to_thread(entry.is_dir) (or
batch directory checks in the same to_thread call) when deciding to call the
synchronous _try_load_dir_info, while keeping the existing await call to the
async _try_load_archive_info for .tar.gz files; update list_backups to collect
entries from the thread and then proceed with the same filtering, sorting,
logging, and return logic.
| if entry.is_dir(): # noqa: ASYNC240 | ||
| manifest_path = entry / "manifest.json" | ||
| if manifest_path.exists(): | ||
| try: | ||
| data = json.loads(manifest_path.read_text(encoding="utf-8")) | ||
| m = BackupManifest.model_validate(data) | ||
| if m.backup_id == backup_id: | ||
| return m | ||
| except Exception as exc: | ||
| logger.warning( | ||
| BACKUP_MANIFEST_INVALID, | ||
| path=str(manifest_path), | ||
| error=str(exc), | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Synchronous I/O in async-accessible method with noqa suppression.
Lines 650-654 perform synchronous filesystem operations (entry.is_dir(), manifest_path.exists(), read_text()) with a # noqa: ASYNC240 suppression. While the suppression indicates awareness, this path is called from _load_manifest which iterates over potentially many entries, amplifying blocking time.
Consider wrapping the entire _try_load_entry_manifest call in asyncio.to_thread from the caller, similar to how _try_delete_backup is handled at line 589.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/service.py` around lines 650 - 663, The helper
_try_load_entry_manifest performs blocking filesystem calls (entry.is_dir,
manifest_path.exists, manifest_path.read_text) but is left async with a noqa;
instead make it a synchronous helper (or keep as is) and ensure callers (notably
_load_manifest) invoke it via asyncio.to_thread like _try_delete_backup does so
the blocking I/O runs off the event loop; update references to
_try_load_entry_manifest in _load_manifest to call it via asyncio.to_thread(...,
args=(entry, backup_id)) and remove the ASYNC240 suppression, keeping
BackupManifest and backup_id checks and the existing warning handling intact.
|
|
||
| scheduler.reschedule(hours) | ||
| logger.info( | ||
| SETTINGS_SUBSCRIBER_NOTIFIED, | ||
| subscriber=self.subscriber_name, | ||
| namespace="backup", | ||
| key="schedule_hours", | ||
| note="rescheduled", | ||
| ) |
There was a problem hiding this comment.
Handle potential reschedule validation errors.
Per the scheduler.reschedule snippet, the method validates interval_hours >= 1. If a user sets schedule_hours to 0 or a negative value in settings, int() conversion succeeds but reschedule() will raise ValueError. This exception would propagate without logging context.
🛡️ Proposed fix
- scheduler.reschedule(hours)
+ try:
+ scheduler.reschedule(hours)
+ except ValueError:
+ logger.error(
+ SETTINGS_SUBSCRIBER_NOTIFIED,
+ subscriber=self.subscriber_name,
+ namespace="backup",
+ key="schedule_hours",
+ value=hours,
+ note="invalid interval rejected by scheduler",
+ exc_info=True,
+ )
+ return
+
logger.info(
SETTINGS_SUBSCRIBER_NOTIFIED,
subscriber=self.subscriber_name,Based on learnings: "All error paths must log at WARNING or ERROR with context before raising."
📝 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.
| scheduler.reschedule(hours) | |
| logger.info( | |
| SETTINGS_SUBSCRIBER_NOTIFIED, | |
| subscriber=self.subscriber_name, | |
| namespace="backup", | |
| key="schedule_hours", | |
| note="rescheduled", | |
| ) | |
| try: | |
| scheduler.reschedule(hours) | |
| except ValueError: | |
| logger.error( | |
| SETTINGS_SUBSCRIBER_NOTIFIED, | |
| subscriber=self.subscriber_name, | |
| namespace="backup", | |
| key="schedule_hours", | |
| value=hours, | |
| note="invalid interval rejected by scheduler", | |
| exc_info=True, | |
| ) | |
| raise | |
| logger.info( | |
| SETTINGS_SUBSCRIBER_NOTIFIED, | |
| subscriber=self.subscriber_name, | |
| namespace="backup", | |
| key="schedule_hours", | |
| note="rescheduled", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/settings/subscribers/backup_subscriber.py` around lines 159 -
167, The call to scheduler.reschedule(hours) can raise ValueError for invalid
interval_hours (e.g., 0 or negative) and currently would propagate without
context; wrap the call to scheduler.reschedule(hours) in a try/except that
catches ValueError, log a warning or error (using logger.warn/logger.error)
including SETTINGS_SUBSCRIBER_NOTIFIED, self.subscriber_name,
namespace="backup", key="schedule_hours", the offending hours value and a clear
note, then re-raise the exception so callers still see it; keep the successful
logger.info(...) unchanged when no exception occurs.
- Remove unused `frequency` ENUM setting definition (orphaned -- backup system uses `schedule_hours` exclusively) - Pass resolved SYNTHORG_DB_PATH to _build_backup_service so the persistence handler targets the actual runtime DB file, not the config default
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/synthorg/api/app.py (1)
654-658:⚠️ Potential issue | 🔴 CriticalBackup handlers still drift from the runtime DB/config files.
resolved_db_pathis only populated in theSYNTHORG_DB_PATHbranch, and Line 721 still never passesresolved_config_pathat all. Ifcreate_app()gets a prebuiltpersistencebackend or aRootConfigloaded from a non-default file,_build_backup_service()falls back toconfig.persistence.sqlite.path/SYNTHORG_CONFIG_PATH/company.yaml, so backup and restore can still operate on the wrong files.Also applies to: 721-724, 885-899
🤖 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 654 - 658, The backup service is using default paths because resolved_db_path and resolved_config_path are only set from env vars; update create_app() so that when a prebuilt persistence backend or a RootConfig is provided you compute and populate resolved_db_path and resolved_config_path from those inputs (e.g., read persistence.sqlite.path from the provided persistence or config.batch/source file path from the provided RootConfig) and then pass these resolved_* values into _build_backup_service() instead of relying on config.persistence.sqlite.path / SYNTHORG_CONFIG_PATH / company.yaml; ensure references include resolved_db_path, resolved_config_path, create_app(), _build_backup_service(), persistence, and RootConfig so backup/restore operate on the same DB/config the app is running with.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/app.py`:
- Around line 481-490: The lifecycle backup try/except currently swallows fatal
runtime exceptions (e.g., MemoryError, RecursionError) around the startup backup
call to backup_service.create_backup(BackupTrigger.STARTUP) and the analogous
shutdown block (lines ~542-551); change the except Exception: to except
Exception as e: and immediately re-raise if the exception is a fatal runtime
error (e.g., if isinstance(e, (MemoryError, RecursionError)): raise), otherwise
log via logger.warning(API_APP_STARTUP, error="Startup backup failed
(non-fatal)", exc_info=True) (and apply the same pattern to the shutdown backup
block and its log constant).
- Around line 857-912: The _build_backup_service function is over the size limit
and should be split: extract the component-to-handler resolution loop into a new
helper (e.g., a function build_backup_handlers or BackupWiring.build_handlers)
that accepts BackupConfig, resolved_db_path, resolved_config_path and returns
dict[BackupComponent, ComponentHandler]; move the for-loop logic that constructs
PersistenceComponentHandler, MemoryComponentHandler, and ConfigComponentHandler
(referencing BackupComponent, PersistenceComponentHandler,
MemoryComponentHandler, ConfigComponentHandler) into that helper and call it
from _build_backup_service, keeping _build_backup_service under 50 lines and
leaving only orchestration/try/return logic there; place the new helper in a
small backup wiring module and import it into app.py so file length is reduced.
In `@src/synthorg/settings/definitions/backup.py`:
- Around line 47-58: The SettingDefinition for namespace SettingNamespace.BACKUP
key "path" currently accepts any string; add a validator_pattern argument to the
SettingDefinition call to enforce a valid filesystem path format (e.g.,
absolute/relative path rules you require) so invalid values fail at the settings
boundary; update the SettingDefinition invocation (the instance created by
SettingDefinition in this diff) to include validator_pattern="<your-regex>" and
an appropriate validator_message to make failures clear.
---
Duplicate comments:
In `@src/synthorg/api/app.py`:
- Around line 654-658: The backup service is using default paths because
resolved_db_path and resolved_config_path are only set from env vars; update
create_app() so that when a prebuilt persistence backend or a RootConfig is
provided you compute and populate resolved_db_path and resolved_config_path from
those inputs (e.g., read persistence.sqlite.path from the provided persistence
or config.batch/source file path from the provided RootConfig) and then pass
these resolved_* values into _build_backup_service() instead of relying on
config.persistence.sqlite.path / SYNTHORG_CONFIG_PATH / company.yaml; ensure
references include resolved_db_path, resolved_config_path, create_app(),
_build_backup_service(), persistence, and RootConfig so backup/restore operate
on the same DB/config the app is running with.
🪄 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: 437fb01d-f890-4b9b-8128-7771e81c5897
📒 Files selected for processing (2)
src/synthorg/api/app.pysrc/synthorg/settings/definitions/backup.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). (2)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Use PEP 758 except syntax: useexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
All public functions must have type hints; mypy strict mode is enforced
Docstrings must be in Google style and are required on all public classes and functions (enforced by ruff D rules)
Create new objects instead of mutating existing ones. For non-Pydantic internal collections (registries, BaseTool), usecopy.deepcopy()at construction and wrap withMappingProxyTypefor read-only enforcement. For dict/list fields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.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 conventions:BaseModel,model_validator,computed_field,ConfigDict. Use@computed_fieldfor derived values instead of storing redundant fields. UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional and tuple variants — instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations (e.g. multiple tool invocations, parallel agent calls). Use structured concurrency over barecreate_task.
Line length must be 88 characters (enforced by ruff)
Functions must be < 50 lines, files < 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 have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport logging/logging.getLogger()/print()in ap...
Files:
src/synthorg/api/app.pysrc/synthorg/settings/definitions/backup.py
**/*.{js,ts,py,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Commits must follow format:
<type>: <description>with types: feat, fix, refactor, docs, test, chore, perf, ci. Enforced by commitizen (commit-msg hook). Signed commits are required on main via branch protection — all commits must be GPG/SSH signed.
Files:
src/synthorg/api/app.pysrc/synthorg/settings/definitions/backup.py
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Pre-commit hooks enforce: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint (Dockerfile linting)
Files:
src/synthorg/api/app.pysrc/synthorg/settings/definitions/backup.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Scripts in
scripts/directory have relaxed ruff rules:print()and deferred imports are allowed
Files:
src/synthorg/api/app.pysrc/synthorg/settings/definitions/backup.py
🧬 Code graph analysis (1)
src/synthorg/settings/definitions/backup.py (3)
src/synthorg/settings/enums.py (3)
SettingType(24-36)SettingLevel(39-47)SettingNamespace(6-21)src/synthorg/settings/registry.py (1)
register(30-50)src/synthorg/settings/models.py (1)
SettingDefinition(34-140)
🔇 Additional comments (3)
src/synthorg/settings/definitions/backup.py (3)
23-30: Retention setting update looks correct.
retention_daysuses integer type with clear bounds and an advanced visibility level, which aligns with safe runtime configuration.
37-45: Schedule interval setting is well constrained.
schedule_hoursis typed correctly and bounded to a practical range for scheduled execution.
60-69: New trigger/compression settings are consistent and clear.
compression,on_shutdown, andon_startupare defined with appropriate boolean typing and operator-friendly descriptions/groups.Also applies to: 71-80, 82-90
| try: | ||
| await backup_service.create_backup( | ||
| BackupTrigger.STARTUP, | ||
| ) | ||
| except Exception: | ||
| logger.warning( | ||
| API_APP_STARTUP, | ||
| error="Startup backup failed (non-fatal)", | ||
| exc_info=True, | ||
| ) |
There was a problem hiding this comment.
Re-raise fatal runtime exceptions around lifecycle backups.
These except Exception blocks also catch MemoryError and RecursionError, unlike the rest of this module. A startup or shutdown backup that hits one of those should still abort immediately instead of being downgraded to a warning.
Suggested fix
try:
await backup_service.create_backup(
BackupTrigger.STARTUP,
)
+ except MemoryError, RecursionError:
+ raise
except Exception:
logger.warning(
API_APP_STARTUP,
error="Startup backup failed (non-fatal)",
exc_info=True,
@@
try:
await backup_service.create_backup(
BackupTrigger.SHUTDOWN,
)
+ except MemoryError, RecursionError:
+ raise
except Exception:
logger.warning(
API_APP_SHUTDOWN,
error="Shutdown backup failed (non-fatal)",
exc_info=True,Also applies to: 542-551
🤖 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 481 - 490, The lifecycle backup
try/except currently swallows fatal runtime exceptions (e.g., MemoryError,
RecursionError) around the startup backup call to
backup_service.create_backup(BackupTrigger.STARTUP) and the analogous shutdown
block (lines ~542-551); change the except Exception: to except Exception as e:
and immediately re-raise if the exception is a fatal runtime error (e.g., if
isinstance(e, (MemoryError, RecursionError)): raise), otherwise log via
logger.warning(API_APP_STARTUP, error="Startup backup failed (non-fatal)",
exc_info=True) (and apply the same pattern to the shutdown backup block and its
log constant).
| _r.register( | ||
| SettingDefinition( | ||
| namespace=SettingNamespace.BACKUP, | ||
| key="path", | ||
| type=SettingType.STRING, | ||
| default="/data/backups", | ||
| description="Directory path for storing backups", | ||
| group="General", | ||
| level=SettingLevel.ADVANCED, | ||
| restart_required=True, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add path-format validation at the settings boundary.
path currently accepts any string. Adding a validator_pattern here will fail fast on invalid values before backup logic consumes them.
♻️ Proposed update
_r.register(
SettingDefinition(
namespace=SettingNamespace.BACKUP,
key="path",
type=SettingType.STRING,
default="/data/backups",
description="Directory path for storing backups",
group="General",
level=SettingLevel.ADVANCED,
+ validator_pattern=r"^/.+",
restart_required=True,
)
)As per coding guidelines, "Validate at system boundaries (user input, external APIs, config files)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/settings/definitions/backup.py` around lines 47 - 58, The
SettingDefinition for namespace SettingNamespace.BACKUP key "path" currently
accepts any string; add a validator_pattern argument to the SettingDefinition
call to enforce a valid filesystem path format (e.g., absolute/relative path
rules you require) so invalid values fail at the settings boundary; update the
SettingDefinition invocation (the instance created by SettingDefinition in this
diff) to include validator_pattern="<your-regex>" and an appropriate
validator_message to make failures clear.
- Re-raise MemoryError/RecursionError in startup and shutdown backup try/except blocks (were being silently swallowed) - Extract _build_backup_service + handler-building loop into src/synthorg/backup/factory.py (app.py 912 -> 852 lines, both functions under 50 lines) - Remove stale SCHEMA_VERSION and handler imports from app.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/synthorg/api/app.py (1)
719-722:⚠️ Potential issue | 🟠 MajorPropagate the resolved runtime config path into backup service wiring.
This call threads only
resolved_db_path. If runtime config was loaded from a non-default/non-env path,ConfigComponentHandlercan fall back to"company.yaml"and back up the wrong file.Suggested fix
backup_service = build_backup_service( effective_config, resolved_db_path=resolved_db_path, + resolved_config_path=resolved_config_path, )(And thread
resolved_config_pathintocreate_app()from the same startup source that resolved the config file.)🤖 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 719 - 722, The backup service is only receiving resolved_db_path so ConfigComponentHandler may back up the wrong config file when the runtime config was loaded from a non-default path; modify the wiring to thread the resolved_config_path through create_app and into build_backup_service (alongside resolved_db_path and effective_config) and ensure ConfigComponentHandler uses resolved_config_path instead of defaulting to "company.yaml". Update call sites for create_app and build_backup_service to accept and pass resolved_config_path and adjust ConfigComponentHandler initialization to consume that resolved path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/app.py`:
- Around line 461-471: The rollback can miss stopping a partially-started
backup_service because started_backup_service is only set after await
backup_service.start(); move the marker or ensure cleanup knows the service was
assigned: set started_backup_service = True immediately after
app_state.set_backup_service(backup_service) (before awaiting
backup_service.start()) or, alternatively, update _cleanup_on_failure() to call
backup_service.stop() whenever app_state.get_backup_service() is non-null;
reference backup_service, app_state.set_backup_service, backup_service.start,
started_backup_service, and _cleanup_on_failure to locate and apply the change.
In `@src/synthorg/backup/factory.py`:
- Around line 102-108: The current broad except Exception block in the backup
service builder (which logs via logger.warning with API_APP_STARTUP and returns
None) swallows fatal runtime errors; change it to explicitly re-raise critical
exceptions (at minimum MemoryError and RecursionError) while still catching and
logging only recoverable exceptions—i.e., detect if the caught exception is an
instance of (MemoryError, RecursionError) and re-raise it, otherwise log the
failure with the existing logger.warning(API_APP_STARTUP, error="Failed to build
backup service", exc_info=True) and return None; ensure you modify the exception
handling around the backup construction code in factory.py where logger.warning
and the return None are currently used.
- Around line 46-65: The loop over backup_config.include silently ignores
unknown BackupComponent values; update the factory to fail fast by adding an
explicit else branch after the existing if/elif chain that raises a clear
exception (e.g., ValueError) when the component is not one of
BackupComponent.PERSISTENCE, .MEMORY, or .CONFIG. Modify the code surrounding
BackupComponent handling (the for component_name in backup_config.include loop
and creation of BackupComponent(component_name)) to validate or map the
component and raise with a message referencing the component variable so
unknown/new enum members are surfaced immediately instead of being skipped.
---
Duplicate comments:
In `@src/synthorg/api/app.py`:
- Around line 719-722: The backup service is only receiving resolved_db_path so
ConfigComponentHandler may back up the wrong config file when the runtime config
was loaded from a non-default path; modify the wiring to thread the
resolved_config_path through create_app and into build_backup_service (alongside
resolved_db_path and effective_config) and ensure ConfigComponentHandler uses
resolved_config_path instead of defaulting to "company.yaml". Update call sites
for create_app and build_backup_service to accept and pass resolved_config_path
and adjust ConfigComponentHandler initialization to consume that resolved path.
🪄 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: 873f7bd4-3f2d-415a-aab6-15685c85cebf
📒 Files selected for processing (2)
src/synthorg/api/app.pysrc/synthorg/backup/factory.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). (4)
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Use PEP 758 except syntax: useexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
All public functions must have type hints; mypy strict mode is enforced
Docstrings must be in Google style and are required on all public classes and functions (enforced by ruff D rules)
Create new objects instead of mutating existing ones. For non-Pydantic internal collections (registries, BaseTool), usecopy.deepcopy()at construction and wrap withMappingProxyTypefor read-only enforcement. For dict/list fields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.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 conventions:BaseModel,model_validator,computed_field,ConfigDict. Use@computed_fieldfor derived values instead of storing redundant fields. UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional and tuple variants — instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations (e.g. multiple tool invocations, parallel agent calls). Use structured concurrency over barecreate_task.
Line length must be 88 characters (enforced by ruff)
Functions must be < 50 lines, files < 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 have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Never useimport logging/logging.getLogger()/print()in ap...
Files:
src/synthorg/backup/factory.pysrc/synthorg/api/app.py
**/*.{js,ts,py,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Commits must follow format:
<type>: <description>with types: feat, fix, refactor, docs, test, chore, perf, ci. Enforced by commitizen (commit-msg hook). Signed commits are required on main via branch protection — all commits must be GPG/SSH signed.
Files:
src/synthorg/backup/factory.pysrc/synthorg/api/app.py
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Pre-commit hooks enforce: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint (Dockerfile linting)
Files:
src/synthorg/backup/factory.pysrc/synthorg/api/app.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Scripts in
scripts/directory have relaxed ruff rules:print()and deferred imports are allowed
Files:
src/synthorg/backup/factory.pysrc/synthorg/api/app.py
🧠 Learnings (2)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/app.py
📚 Learning: 2026-03-18T09:27:32.266Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T09:27:32.266Z
Learning: Applies to src/**/*.py : Handle errors explicitly, never silently swallow exceptions
Applied to files:
src/synthorg/api/app.py
🧬 Code graph analysis (1)
src/synthorg/backup/factory.py (5)
src/synthorg/backup/models.py (1)
BackupComponent(30-35)src/synthorg/backup/service.py (1)
BackupService(71-838)src/synthorg/backup/config.py (1)
BackupConfig(34-107)src/synthorg/backup/handlers/protocol.py (1)
ComponentHandler(16-56)src/synthorg/config/schema.py (1)
RootConfig(461-730)
🔇 Additional comments (1)
src/synthorg/api/app.py (1)
479-486: Good fatal-exception handling in lifecycle backups.Re-raising
MemoryError/RecursionErrorwhile keeping other backup failures non-fatal and logged is the right behavior.Also applies to: 542-549
| except Exception: | ||
| logger.warning( | ||
| API_APP_STARTUP, | ||
| error="Failed to build backup service", | ||
| exc_info=True, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Do not swallow fatal runtime exceptions during backup service construction.
The broad catch downgrades MemoryError/RecursionError into a warning + None, which can mask process-fatal conditions.
Suggested fix
- except Exception:
+ except MemoryError, RecursionError:
+ raise
+ except Exception:
logger.warning(
API_APP_STARTUP,
error="Failed to build backup service",
exc_info=True,
)
return NoneBased on learnings, "Handle errors explicitly, never silently swallow exceptions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/backup/factory.py` around lines 102 - 108, The current broad
except Exception block in the backup service builder (which logs via
logger.warning with API_APP_STARTUP and returns None) swallows fatal runtime
errors; change it to explicitly re-raise critical exceptions (at minimum
MemoryError and RecursionError) while still catching and logging only
recoverable exceptions—i.e., detect if the caught exception is an instance of
(MemoryError, RecursionError) and re-raise it, otherwise log the failure with
the existing logger.warning(API_APP_STARTUP, error="Failed to build backup
service", exc_info=True) and return None; ensure you modify the exception
handling around the backup construction code in factory.py where logger.warning
and the return None are currently used.
- Move started_backup_service marker before start() so cleanup can stop a partially-started service - Re-raise MemoryError/RecursionError in factory except block - Add assert_never exhaustiveness guard for unknown BackupComponent - Resolve SYNTHORG_CONFIG_PATH at create_app and pass through to build_backup_service so ConfigComponentHandler targets the actual runtime config file
🤖 I have created a release *beep* *boop* --- ## [0.3.3](v0.3.2...v0.3.3) (2026-03-18) ### Features * **backup:** implement automated backup and restore system ([#541](#541)) ([867b7c1](867b7c1)) * **providers:** runtime provider management with CRUD, presets, and multi-auth ([#540](#540)) ([936c345](936c345)), closes [#451](#451) * **tools:** wire per-category sandbox backend selection ([#534](#534)) ([311a1ab](311a1ab)) ### Bug Fixes * **cli:** switch cosign verification from .sig tags to OCI referrers ([#533](#533)) ([8ee5471](8ee5471)), closes [#532](#532) ### CI/CD * bump wrangler from 4.74.0 to 4.75.0 in /.github in the minor-and-patch group ([#535](#535)) ([de15867](de15867)) ### Maintenance * bump github.com/google/go-containerregistry from 0.21.2 to 0.21.3 in /cli in the minor-and-patch group ([#536](#536)) ([4a09aed](4a09aed)) * bump litellm from 1.82.3 to 1.82.4 in the minor-and-patch group ([#538](#538)) ([9f7f83d](9f7f83d)) * bump vue-tsc from 3.2.5 to 3.2.6 in /web in the minor-and-patch group across 1 directory ([#537](#537)) ([eb3dc4e](eb3dc4e)) --- 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>
🤖 I have created a release *beep* *boop* --- ## [0.3.3](v0.3.2...v0.3.3) (2026-03-18) ### Features * **backup:** implement automated backup and restore system ([#541](#541)) ([867b7c1](867b7c1)) * **providers:** runtime provider management with CRUD, presets, and multi-auth ([#540](#540)) ([936c345](936c345)), closes [#451](#451) * **tools:** wire per-category sandbox backend selection ([#534](#534)) ([311a1ab](311a1ab)) ### Bug Fixes * **ci:** add COSIGN_EXPERIMENTAL=1 for OCI referrer mode in cosign sign ([#543](#543)) ([226ed2f](226ed2f)) * **cli:** switch cosign verification from .sig tags to OCI referrers ([#533](#533)) ([8ee5471](8ee5471)), closes [#532](#532) ### CI/CD * bump wrangler from 4.74.0 to 4.75.0 in /.github in the minor-and-patch group ([#535](#535)) ([de15867](de15867)) ### Maintenance * bump github.com/google/go-containerregistry from 0.21.2 to 0.21.3 in /cli in the minor-and-patch group ([#536](#536)) ([4a09aed](4a09aed)) * bump litellm from 1.82.3 to 1.82.4 in the minor-and-patch group ([#538](#538)) ([9f7f83d](9f7f83d)) * bump vue-tsc from 3.2.5 to 3.2.6 in /web in the minor-and-patch group across 1 directory ([#537](#537)) ([eb3dc4e](eb3dc4e)) * **main:** release 0.3.3 ([#539](#539)) ([c3de2a2](c3de2a2)) * revert v0.3.3 release artifacts (Docker signing failed) ([#544](#544)) ([7f48f52](7f48f52)) --- 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>
Summary
/admin/backup(create, list, get, delete, restore)Closes #449
Test plan
Review coverage
Pre-reviewed by 8 specialized agents:
22 findings addressed across all agents including:
Not in scope (separate issues)