refactor: backend-runtime remainder — idempotency, lifecycle-locks, controller-to-service, shutdown drains - #2418
Merged
Merged
Conversation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes the backend-runtime refactoring, focusing on robustness, idempotency, and lifecycle management. It introduces standardized patterns for service startup/shutdown, ensures retry safety for mutating operations, and decouples controllers from direct repository access to improve maintainability and import-cycle safety. 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 the 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 counterproductive. 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
|
Aureliolo
temporarily deployed
to
cloudflare-preview
June 19, 2026 20:37 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive idempotency guards and lifecycle improvements across the codebase, notably requiring Idempotency-Key headers for approvals and backup restores, and implementing a robust, unrestartable state pattern for background services that time out during shutdown. It also refactors several controllers to route repository access through dedicated service facades (such as ConversationalResumeService and AnalyticsReadService) and enables seen-claims dedup for standalone workers. The code review identified two critical issues: a missing import of log_task_exceptions in webhook_bridge.py that would cause a runtime NameError, and an exception handling gap in backup.py where Pydantic's ValidationError is not caught during cached response validation, potentially leading to unhandled 500 errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+17
to
20
| from synthorg.core.clock import Clock, SystemClock | ||
| from synthorg.core.critical_errors import reraise_critical | ||
| from synthorg.core.lifecycle_constants import DEFAULT_DRAIN_TIMEOUT_SECONDS | ||
| from synthorg.core.types import NotBlankStr |
Contributor
There was a problem hiding this comment.
The log_task_exceptions helper is used in the stop method to handle orphaned drain tasks, but it is not imported in this file. This will result in a NameError at runtime when a timeout occurs during shutdown. Please import log_task_exceptions from synthorg.observability.background_tasks after verifying it is not already imported elsewhere in the file.
Suggested change
| from synthorg.core.clock import Clock, SystemClock | |
| from synthorg.core.critical_errors import reraise_critical | |
| from synthorg.core.lifecycle_constants import DEFAULT_DRAIN_TIMEOUT_SECONDS | |
| from synthorg.core.types import NotBlankStr | |
| from synthorg.core.clock import Clock, SystemClock | |
| from synthorg.core.critical_errors import reraise_critical | |
| from synthorg.core.lifecycle_constants import DEFAULT_DRAIN_TIMEOUT_SECONDS | |
| from synthorg.core.types import NotBlankStr | |
| from synthorg.observability.background_tasks import log_task_exceptions |
References
- Before suggesting or adding an import statement, verify if the function, class, or module is already imported in the file to prevent duplicate imports (which can trigger linter errors like ruff F811).
Comment on lines
+479
to
+481
| try: | ||
| response = RestoreResponse.model_validate(outcome.result) | ||
| except (ValueError, TypeError) as exc: |
Contributor
There was a problem hiding this comment.
In Pydantic v2, ValidationError does not inherit from ValueError or TypeError. Since RestoreResponse.model_validate() raises ValidationError on schema mismatch, any validation failures on the cached restore response will bypass this except block and propagate as unhandled 500 errors. We should explicitly catch ValidationError to ensure it is correctly wrapped in a RestoreError. Please ensure ValidationError is imported from pydantic if it is not already imported in the file to avoid duplicate imports.
Suggested change
| try: | |
| response = RestoreResponse.model_validate(outcome.result) | |
| except (ValueError, TypeError) as exc: | |
| try: | |
| response = RestoreResponse.model_validate(outcome.result) | |
| except (ValueError, TypeError, ValidationError) as exc: |
References
- Before suggesting or adding an import statement, verify if the function, class, or module is already imported in the file to prevent duplicate imports (which can trigger linter errors like ruff F811).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (17)
🧰 Additional context used📓 Path-based instructions (3)src/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{src/**/*.py,tests/**/*.py}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/meta/mcp/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (16)📓 Common learnings📚 Learning: 2026-06-11T17:01:48.351ZApplied to files:
📚 Learning: 2026-05-05T09:04:46.195ZApplied to files:
📚 Learning: 2026-05-21T22:55:20.496ZApplied to files:
📚 Learning: 2026-05-29T08:50:58.380ZApplied to files:
📚 Learning: 2026-06-09T09:22:47.752ZApplied to files:
📚 Learning: 2026-05-21T22:55:09.289ZApplied to files:
📚 Learning: 2026-05-31T18:00:32.445ZApplied to files:
📚 Learning: 2026-06-10T12:09:37.293ZApplied to files:
📚 Learning: 2026-06-10T12:09:46.221ZApplied to files:
📚 Learning: 2026-06-03T11:43:13.104ZApplied to files:
📚 Learning: 2026-06-09T10:06:53.040ZApplied to files:
📚 Learning: 2026-06-09T17:05:23.619ZApplied to files:
📚 Learning: 2026-06-09T17:05:38.738ZApplied to files:
📚 Learning: 2026-06-09T17:07:02.613ZApplied to files:
📚 Learning: 2026-06-13T08:51:11.124ZApplied to files:
🔇 Additional comments (3)
WalkthroughThe pull request adds idempotency-key handling for approval decisions, backup restore, and MCP restore, and routes those flows through 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/synthorg/api/controllers/_webhooks_wiring.py (1)
139-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the final hash fallback tolerant of surrogate input.
The nonce hashes use
errors="replace", but the final over-length fallback encodes the composed key with strict UTF-8. A long connection/event segment containing an unpaired surrogate can raiseUnicodeEncodeErrorinstead of returning a bounded dedup key.🐛 Proposed fix
- raw_key = hashlib.sha256(raw_key.encode("utf-8")).hexdigest() + raw_key = hashlib.sha256( + raw_key.encode("utf-8", errors="replace") + ).hexdigest()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/api/controllers/_webhooks_wiring.py` at line 139, The raw_key.encode("utf-8") call on the line with hashlib.sha256 uses strict UTF-8 encoding by default, which will raise UnicodeEncodeError if raw_key contains unpaired surrogates. Make this encoding tolerant by adding the errors="replace" parameter to the encode call, similar to how nonce hashes are already handled elsewhere in the code, so that surrogate characters are safely replaced rather than causing the function to fail.src/synthorg/budget/quota_poller.py (1)
101-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAttach the task-exception callback to the poll loop.
_poll_loop()re-raises critical errors, but the task is kept inself._task; without a done callback, those failures can stay buffered until a later lifecycle call clears the handle. Use the already-importedlog_task_exceptionswhen creating the poll task.Proposed fix
- self._task = asyncio.get_running_loop().create_task( + task = asyncio.get_running_loop().create_task( self._poll_loop(), name="quota-poller", ) + task.add_done_callback( + log_task_exceptions( + logger, + QUOTA_POLL_FAILED, + note="quota_poller_loop", + ) + ) + self._task = task🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/budget/quota_poller.py` around lines 101 - 104, The task created for _poll_loop() in the create_task call is missing an exception handler callback, which allows critical errors to remain buffered rather than being logged. Add a done callback to the self._task by calling add_done_callback with the log_task_exceptions function (which is already imported) immediately after creating the task to ensure exceptions from _poll_loop() are properly logged when the task completes.src/synthorg/notifications/dispatcher.py (1)
279-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound the sink-close phase as well.
The new hard deadline only covers
_dispatch_idle.wait(). After that succeeds, a stucksink.close()can still hold_lifecycle_lockindefinitely; if an outer shutdown timeout cancelsaclose()here,_stop_failedremainsFalsewhile_started/_stoppingstay set. Wrap the close fan-out in the same shieldedwait_for(...)pattern and mark the dispatcher unrestartable on timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/notifications/dispatcher.py` around lines 279 - 282, The sink close phase starting at the TaskGroup creation (lines 279-282) lacks a hard deadline, allowing stuck sink.close() calls to hold _lifecycle_lock indefinitely and leave the dispatcher in an inconsistent state. Wrap the entire TaskGroup fan-out that iterates through sinks and calls _safe_close() in a shielded wait_for() context using the same timeout pattern already applied to _dispatch_idle.wait(). On timeout, catch the asyncio.TimeoutError, set _stop_failed to True to mark the dispatcher unrestartable, and re-raise or handle appropriately to ensure cleanup proceeds.src/synthorg/integrations/oauth/token_manager.py (1)
201-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle completed refresh tasks before returning from
start().Line 201 returns for any non-
Nonetask, including a completed or failed refresh loop. That makes a laterstart()report success while no loop is running, and the task exception is not surfaced promptly. Clear done handles and attach the standard background-task exception callback.Proposed fix
- if self._task is not None: + if self._task is not None and self._task.done(): + self._task = None + if self._task is not None: return await self._resolve_flow_timeout() await self._resolve_loop_tuning() - self._task = asyncio.create_task(self._refresh_loop()) + task = asyncio.create_task( + self._refresh_loop(), + name="oauth-token-manager", + ) + task.add_done_callback( + log_task_exceptions( + logger, + OAUTH_TOKEN_REFRESH_FAILED, + note="oauth_token_manager_loop", + ) + ) + self._task = task🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/integrations/oauth/token_manager.py` around lines 201 - 205, The start() method currently returns early whenever self._task is not None, but this doesn't account for completed or failed tasks. Modify the logic to check whether the existing task is actually still running using the task's done() method. If the task exists but is already done, clear the self._task reference and attach the standard background-task exception callback to surface any task exceptions before proceeding to create a new refresh loop. Only return early if the task exists and is still running (not done).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/api/controllers/approvals/decisions.py`:
- Around line 53-66: The max_length constraint of 255 characters on the
_IdempotencyKeyHeader annotation is too large because the idempotency key is
combined with the approval_id in a composite key format
(approval_id:idempotency_key) before being stored in the database. Since the
approval_id is a UUID taking 36 characters plus one colon character, the
remaining space for the idempotency_key must be reduced to avoid exceeding the
255 character database column constraint. Change the max_length parameter in the
HeaderParameter definition from 255 to 218 to ensure the composite key never
exceeds the database limit.
In `@src/synthorg/api/controllers/backup.py`:
- Around line 413-419: The idempotency key generation in the run_idempotent call
for the backup restore operation only includes the backup_id and the caller's
idempotency_key, but does not account for the specific components being
restored. This causes requests with the same key but different restore
components to incorrectly reuse the cached result from the first request. Modify
the key parameter to include a canonical representation of the restore intent
(the components from the data object) by incorporating it into the key
generation using hashlib and json to create a deterministic hash, or use an
existing shared idempotency-key helper function if available in the codebase.
In `@src/synthorg/api/lifecycle_runner_shutdown.py`:
- Around line 72-77: The constant `_ENTRY_TASK_DRAIN_OUTER_SECONDS` uses a
hardcoded value of `3.0` instead of referencing the actual
`_ENTRY_TASK_DRAIN_GRACE_SECONDS` constant defined in `api/state.py`. To fix
this, import `_ENTRY_TASK_DRAIN_GRACE_SECONDS` from the state module and replace
the hardcoded `3.0` in the calculation of `_ENTRY_TASK_DRAIN_OUTER_SECONDS` with
the imported constant, ensuring the two values remain synchronized and adhering
to the guideline of avoiding hardcoded numeric values.
In `@src/synthorg/api/lifecycle_runner_startup.py`:
- Around line 437-441: The `_wire_webhook_request_services(persistence,
app_state)` call is currently gated inside the `if persistence is not None:`
block, which prevents the webhook replay protector singleton from being
published when persistence is not available. Move the
`_wire_webhook_request_services(persistence, app_state)` call outside and after
the entire persistence guard block so that it executes unconditionally on every
startup, ensuring the webhook replay nonce cache is always initialized
regardless of persistence configuration.
- Around line 208-233: The exception handler at line 208 currently catches only
Exception, but asyncio.CancelledError inherits from BaseException (since Python
3.8) and will bypass this guard, allowing resources to leak if the lifespan is
cancelled after startup has begun. Modify the except clause to catch both
Exception and asyncio.CancelledError so that the _cleanup_on_failure function is
properly invoked for cancellation scenarios as well, ensuring the documented
contract that all started services are stopped during startup failure is
maintained across all failure modes.
In `@src/synthorg/api/lifecycle_runner_support.py`:
- Around line 229-239: The `_wire_webhook_request_services()` function is
currently called only inside the `if persistence is not None` block, but the
docstring explicitly states the `ReplayProtector` must be wired unconditionally
regardless of persistence. Move the `_wire_webhook_request_services()` function
call outside the persistence guard so it always executes during startup, while
keeping any persistence-dependent initialization within the conditional block to
preserve the intended behavior.
In `@src/synthorg/api/lifecycle_shared.py`:
- Around line 154-237: The `_cleanup_on_failure` function exceeds the 50-line
convention limit due to repeated stop logic for each service. Refactor by
creating a declarative data structure (such as a list or tuple of configuration
objects/dicts) that contains the stop parameters for each service (the started
flag, the service object, the error message, and the service name). Then iterate
through this structure in a single loop calling `_try_stop` instead of repeating
the same if-block pattern seven times. This eliminates the duplication, reduces
the function size significantly, and makes it easier to add or modify services
in the future.
In `@src/synthorg/api/state.py`:
- Around line 181-213: The current drain logic only processes tasks that are
still running after the timeout grace period, but ignores tasks that completed
during the grace window. Modify the code to capture both return values from the
asyncio.wait() call (the done tasks and still_running tasks). Then gather and
log any exceptions from the completed tasks in the done set, similar to how
exceptions from still_running tasks are currently logged. This ensures that
tasks which failed before the timeout are properly awaited and their errors are
surfaced in the log rather than being silently abandoned.
In `@src/synthorg/engine/workflow/execution_observer.py`:
- Around line 45-49: The config_resolver parameter docstring incorrectly
describes the configuration precedence as DB > env > YAML, but the project
contract specifies DB > env > code default, with YAML being only an ingestion
format and not a precedence tier. Locate the config_resolver parameter docstring
in the file and update the precedence description to correctly state DB > env >
code default, and add clarification that YAML is only used as a company-template
ingestion format, not as a precedence tier in the configuration override
hierarchy.
In `@src/synthorg/engine/workflow/webhook_bridge.py`:
- Around line 227-233: The issue is that the task is being cleared
unconditionally when done, but it should only be cleared if the unsubscribe was
successful. Add a _subscribed boolean attribute to track subscription state,
initialize it to False, and set it to True after a successful subscribe() call.
In both stop paths, only set _subscribed to False after unsubscribe() succeeds.
Then modify the condition in the done task check to only clear _task when
_subscribed is False, ensuring we do not clear the task reference when the
subscription is still active due to a failed unsubscribe operation.
- Around line 297-319: The stop() method's exception handling does not account
for cancellation of the stop() call itself during hard process shutdown. Add an
exception handler for BaseException (after the existing TimeoutError handler)
that catches when the asyncio.wait_for call is interrupted by an outer
cancellation. In this handler, explicitly cancel the drain_task using its
cancel() method, await the drain_task to allow it to complete cleanup, and add a
done callback using log_task_exceptions() with note="shutdown_interrupted" to
log the interruption. This pattern is already correctly implemented in
backup/scheduler.py and meta/toolsmith/cycle_scheduler.py and should be applied
here to prevent the shielded drain_task from becoming orphaned during shutdown.
In `@src/synthorg/integrations/rate_limiting/shared_state.py`:
- Around line 216-244: The current code handles TimeoutError but does not handle
asyncio.CancelledError that can occur when stop() itself is cancelled by its
caller. Add an additional except clause after the existing TimeoutError handler
to catch asyncio.CancelledError. In this handler, call the cancel() method on
drain_task and await it to ensure cleanup completes before re-raising the
CancelledError to the caller.
In `@src/synthorg/meta/mcp/handlers/infrastructure/backup.py`:
- Around line 251-257: The IdempotencyService is constructed on line 256 without
passing a clock parameter, causing it to default to SystemClock and ignore the
test/app clock seam. This breaks MCP in-flight polling in tests by forcing
real-time waits instead of using injected FakeClock. Pass the clock parameter
when constructing IdempotencyService, extracting it from the app_state or clock
seam mechanism (following the coding guideline pattern of clock: Clock | None =
None) so that tests can properly control time through the fake clock injection.
In `@src/synthorg/workers/__main__.py`:
- Around line 269-285: The _safe_cleanup function only catches Exception, but
asyncio.CancelledError (a BaseException subclass) can escape and interrupt
subsequent cleanup steps. Modify the exception handling in _safe_cleanup to also
capture BaseException or specifically asyncio.CancelledError, ensure the
function continues through all cleanup attempts even when CancelledError is
raised, collect any cancellation errors, and re-raise them after all three
cleanup steps (http_client.aclose(), task_queue.stop(), and persistence cleanup)
have been attempted. This ensures every finally block completes before the
cancellation propagates.
In `@tests/unit/engine/workflow/test_execution_lifecycle.py`:
- Line 249: The _depth_resolver function has a hardcoded numeric value 16 as the
default parameter for the depth parameter, which violates the no magic numbers
policy enforced by scripts/check_no_magic_numbers.py for function parameter
defaults. Define a module-level named constant with a descriptive name (e.g.,
DEFAULT_DEPTH or DEPTH_LIMIT) at the top of the file with the value 16, then
replace the hardcoded 16 in the _depth_resolver function signature with this
constant reference.
In `@tests/unit/meta/mcp/test_handlers_infrastructure.py`:
- Around line 424-436: The test test_restore_requires_idempotency_key only
verifies that an error response is returned when the idempotency_key argument is
missing, but it does not verify that the actual destructive restore operation
was never executed. Mock the underlying restore method (the destructive
operation called by the synthorg_backup_restore handler) and add an assertion
after the handler call to confirm the mock was never called, ensuring that
validation failures prevent the restore from running and catching any future
regressions.
---
Outside diff comments:
In `@src/synthorg/api/controllers/_webhooks_wiring.py`:
- Line 139: The raw_key.encode("utf-8") call on the line with hashlib.sha256
uses strict UTF-8 encoding by default, which will raise UnicodeEncodeError if
raw_key contains unpaired surrogates. Make this encoding tolerant by adding the
errors="replace" parameter to the encode call, similar to how nonce hashes are
already handled elsewhere in the code, so that surrogate characters are safely
replaced rather than causing the function to fail.
In `@src/synthorg/budget/quota_poller.py`:
- Around line 101-104: The task created for _poll_loop() in the create_task call
is missing an exception handler callback, which allows critical errors to remain
buffered rather than being logged. Add a done callback to the self._task by
calling add_done_callback with the log_task_exceptions function (which is
already imported) immediately after creating the task to ensure exceptions from
_poll_loop() are properly logged when the task completes.
In `@src/synthorg/integrations/oauth/token_manager.py`:
- Around line 201-205: The start() method currently returns early whenever
self._task is not None, but this doesn't account for completed or failed tasks.
Modify the logic to check whether the existing task is actually still running
using the task's done() method. If the task exists but is already done, clear
the self._task reference and attach the standard background-task exception
callback to surface any task exceptions before proceeding to create a new
refresh loop. Only return early if the task exists and is still running (not
done).
In `@src/synthorg/notifications/dispatcher.py`:
- Around line 279-282: The sink close phase starting at the TaskGroup creation
(lines 279-282) lacks a hard deadline, allowing stuck sink.close() calls to hold
_lifecycle_lock indefinitely and leave the dispatcher in an inconsistent state.
Wrap the entire TaskGroup fan-out that iterates through sinks and calls
_safe_close() in a shielded wait_for() context using the same timeout pattern
already applied to _dispatch_idle.wait(). On timeout, catch the
asyncio.TimeoutError, set _stop_failed to True to mark the dispatcher
unrestartable, and re-raise or handle appropriately to ensure cleanup proceeds.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09683b04-2dc2-4e19-b0cc-48ac9926b452
📒 Files selected for processing (93)
CLAUDE.mddata/architecture_report.jsondata/feature_index.jsondata/runtime_stats.yamldocs/reference/lifecycle-sync.mdsrc/synthorg/api/api_core_state.pysrc/synthorg/api/controllers/_conversational_resume.pysrc/synthorg/api/controllers/_webhooks_wiring.pysrc/synthorg/api/controllers/analytics/overview.pysrc/synthorg/api/controllers/analytics/trends.pysrc/synthorg/api/controllers/approvals/decisions.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/controllers/training.pysrc/synthorg/api/controllers/webhooks/_shared.pysrc/synthorg/api/controllers/webhooks/activity.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/lifecycle_helpers/conversational_wiring.pysrc/synthorg/api/lifecycle_runner_shutdown.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/lifecycle_runner_support.pysrc/synthorg/api/lifecycle_shared.pysrc/synthorg/api/services/analytics_read_service.pysrc/synthorg/api/state.pysrc/synthorg/backup/scheduler.pysrc/synthorg/backup/service.pysrc/synthorg/budget/errors.pysrc/synthorg/budget/quota_poller.pysrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/communication/bus/errors.pysrc/synthorg/engine/workflow/errors.pysrc/synthorg/engine/workflow/execution_node_dispatch.pysrc/synthorg/engine/workflow/execution_observer.pysrc/synthorg/engine/workflow/execution_service.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/hr/training/plan_service.pysrc/synthorg/idempotency/__init__.pysrc/synthorg/idempotency/service.pysrc/synthorg/integrations/errors.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/state.pysrc/synthorg/meta/chief_of_staff/resume_service.pysrc/synthorg/meta/mcp/domains/_remaining_args/_infrastructure.pysrc/synthorg/meta/mcp/domains/infrastructure.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/meta/state.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/errors.pysrc/synthorg/observability/events/workers.pysrc/synthorg/workers/__main__.pysrc/synthorg/workers/execution_service/_protocol.pysrc/synthorg/workers/heartbeat_subscriber.pytests/unit/a2a/test_gateway.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_backup.pytests/unit/api/controllers/test_backup_required_idempotency.pytests/unit/api/controllers/test_conversational_intake_resume.pytests/unit/api/controllers/test_webhooks_idempotency.pytests/unit/api/controllers/test_webhooks_retry.pytests/unit/api/controllers/test_webhooks_service_layer.pytests/unit/api/services/test_analytics_read_service.pytests/unit/api/test_cleanup_on_failure.pytests/unit/api/test_lifecycle_builder_shutdown.pytests/unit/api/test_lifecycle_runner_support.pytests/unit/api/test_oauth_state_cleanup_wiring.pytests/unit/api/test_runtime_background_services_cleanup.pytests/unit/api/test_startup_wiring.pytests/unit/api/test_state.pytests/unit/backup/test_scheduler_lifecycle.pytests/unit/backup/test_service.pytests/unit/budget/test_quota_poller_lifecycle.pytests/unit/communication/bus/test_nats_stop_drain.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_execution_observer.pytests/unit/engine/workflow/test_execution_service.pytests/unit/engine/workflow/test_execution_service_subworkflows.pytests/unit/engine/workflow/test_subworkflow_service.pytests/unit/engine/workflow/test_webhook_bridge_lifecycle.pytests/unit/hr/training/test_plan_service.pytests/unit/idempotency/__init__.pytests/unit/idempotency/test_service.pytests/unit/integrations/oauth/test_token_manager_lifecycle.pytests/unit/integrations/rate_limiting/__init__.pytests/unit/integrations/rate_limiting/test_shared_state_lifecycle.pytests/unit/meta/chief_of_staff/test_invite.pytests/unit/meta/chief_of_staff/test_resume_service.pytests/unit/meta/mcp/test_handlers_infrastructure.pytests/unit/notifications/test_dispatcher_lifecycle.pytests/unit/workers/test_main.pyweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/backup.tsweb/src/api/idempotency.tsweb/src/api/types/openapi.gen.ts
💤 Files with no reviewable changes (1)
- src/synthorg/workers/execution_service/_protocol.py
| return FakeTaskEngine() | ||
|
|
||
|
|
||
| def _depth_resolver(depth: int = 16) -> ConfigResolver: |
Contributor
There was a problem hiding this comment.
Replace the default literal depth with an allowlisted named constant.
depth: int = 16 is a function-default magic number and can trip the repository’s numeric gate for Python files.
Suggested fix
+from typing import Final
+
+_DEFAULT_TEST_SUBWORKFLOW_DEPTH: Final[int] = 16
+
-def _depth_resolver(depth: int = 16) -> ConfigResolver:
+def _depth_resolver(
+ depth: int = _DEFAULT_TEST_SUBWORKFLOW_DEPTH,
+) -> ConfigResolver:As per coding guidelines, “No hardcoded numeric values… function/method parameter defaults” are enforced, and based on learnings scripts/check_no_magic_numbers.py intentionally checks function/method parameter defaults.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/engine/workflow/test_execution_lifecycle.py` at line 249, The
_depth_resolver function has a hardcoded numeric value 16 as the default
parameter for the depth parameter, which violates the no magic numbers policy
enforced by scripts/check_no_magic_numbers.py for function parameter defaults.
Define a module-level named constant with a descriptive name (e.g.,
DEFAULT_DEPTH or DEPTH_LIMIT) at the top of the file with the value 16, then
replace the hardcoded 16 in the _depth_resolver function signature with this
constant reference.
Sources: Coding guidelines, Learnings
Aureliolo
force-pushed
the
refactor/backend-runtime-2409
branch
from
June 19, 2026 21:12
9183c84 to
b18fdaf
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
June 19, 2026 21:14 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2418 +/- ##
========================================
Coverage 89.18% 89.19%
========================================
Files 2953 2958 +5
Lines 149755 150183 +428
========================================
+ Hits 133561 133951 +390
- Misses 16194 16232 +38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthorg/api/controllers/_webhooks_wiring.py (1)
123-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDomain-separate hashed nonces from raw nonces.
A nonce longer than
MAX_NONCE_CHARSis replaced by its bare SHA-256 hex. A later delivery can use that hex digest as a short raw nonce and produce the same idempotency key, suppressing a distinct webhook event. Prefix raw and hashed nonce material before length-prefixing.Proposed fix
- nonce_for_key = ( - nonce - if len(nonce) <= MAX_NONCE_CHARS - else hashlib.sha256( - nonce.encode("utf-8", errors="replace"), - ).hexdigest() - ) + if len(nonce) <= MAX_NONCE_CHARS: + nonce_for_key = f"raw:{nonce}" + else: + nonce_for_key = ( + "sha256:" + + hashlib.sha256( + nonce.encode("utf-8", errors="replace"), + ).hexdigest() + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/api/controllers/_webhooks_wiring.py` around lines 123 - 132, The idempotency key generation in the nonce handling logic is vulnerable to collisions where a hashed nonce digest could be reused as a raw nonce input, producing the same key for different webhook events. To fix this, add domain-separating prefixes that distinguish between raw and hashed nonce material before the _len_prefixed calls. Modify the logic so that when the nonce is hashed (when its length exceeds MAX_NONCE_CHARS), it is prefixed with a marker indicating it is hashed, and when using the raw nonce directly, it is prefixed with a marker indicating it is raw. Apply these prefixes before passing the nonce_for_key value to _len_prefixed in the raw_key construction.src/synthorg/api/controllers/_conversational_resume.py (1)
534-549:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEnforce the participant cap in the write path.
Line 535 reads the roster and Line 549 writes later through a separate service call. Two concurrent approvals for different invited agents can both pass Line 540 and then both insert, so the configured cap can still be exceeded. Move the duplicate/cap check and insert into one service/repository operation with a backend transaction/lock/CAS outcome.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/api/controllers/_conversational_resume.py` around lines 534 - 549, Move the participant duplicate check and cap enforcement logic from the current location into the service.add_participant() call (or create a new atomic service method that handles both operations together). Currently, the check using active_participants() happens separately from the add_participant() write, creating a race condition where concurrent requests can both pass the cap check before either completes the insert. Refactor the service or repository layer to perform the duplicate/cap validation and insert operation as a single atomic transaction or with a backend lock/CAS mechanism, ensuring that the roster cap cannot be exceeded even when multiple concurrent requests arrive simultaneously.
♻️ Duplicate comments (2)
src/synthorg/api/lifecycle_runner_startup.py (2)
437-441:⚠️ Potential issue | 🟠 MajorWire webhook request services outside the persistence gate.
Line 441 is inside
if persistence is not None, but_wire_webhook_request_services(...)intentionally wires the replay protector even when persistence is absent. In no-persistence boots, replay protection is skipped.Proposed fix
- if persistence is not None: + _wire_webhook_request_services(persistence, app_state) + if persistence is not None: try: await _wire_workflow_observer(task_engine, persistence, app_state) _wire_workflow_execution_service(persistence, app_state) - _wire_webhook_request_services(persistence, app_state)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/api/lifecycle_runner_startup.py` around lines 437 - 441, The _wire_webhook_request_services function call is currently inside the if persistence is not None: block, but it should be executed unconditionally outside this gate. Move the _wire_webhook_request_services(persistence, app_state) call after the if block so it runs regardless of whether persistence is available, allowing the replay protector to be properly wired even in no-persistence scenarios (where the function will skip replay protection internally).
208-233:⚠️ Potential issue | 🟠 MajorCatch cancellation in the startup-failure cleanup guard.
Line 208 only catches
Exception, so cancellation can bypass_cleanup_on_failure(...)and leak already-started services.Proposed fix
- except Exception: + except Exception, asyncio.CancelledError: # A propagating exception (a re-raised critical, or an unexpectedAs per coding guidelines,
src/**/*.pyuses Python 3.14+ PEP 758 exception form (except A, B:).In Python 3.14, does asyncio.CancelledError inherit from BaseException (and therefore bypass except Exception)?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/api/lifecycle_runner_startup.py` around lines 208 - 233, The exception handler at line 208 only catches Exception, which means asyncio.CancelledError (which inherits from BaseException) will bypass the _cleanup_on_failure call and leak already-started services. Update the except clause to catch both Exception and asyncio.CancelledError using Python 3.14+ PEP 758 exception form syntax to ensure cleanup always executes before re-raising.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/api/controllers/backup.py`:
- Around line 413-419: The idempotency key being constructed by combining
data.backup_id with the idempotency_key header parameter can exceed the
255-character column constraint in the idempotency store. Instead of passing the
raw composed key string f"{data.backup_id}:{idempotency_key}" to the
run_idempotent method, hash this composed key material using a cryptographic
hash function (such as hashlib.sha256) and pass the resulting hash digest as the
key parameter to ensure it stays within the database column limit while
maintaining uniqueness and preventing token reuse across different backups.
In `@src/synthorg/api/services/analytics_read_service.py`:
- Around line 27-39: The list_tasks() method calls
self._task_repo.query(TaskFilterSpec()) without explicit limit and offset
parameters, which causes it to use default pagination (limit=100, offset=0) and
return only the first 100 tasks. Since the method is intended to return the full
task set for analytics aggregation as stated in the docstring, modify the query
call to pass explicit unbounded pagination parameters (such as limit=sys.maxsize
with offset=0) to ensure all tasks are returned regardless of the total count.
Alternatively, implement a loop to fetch all results by iterating through pages
until exhausted, or add a dedicated list_all() method if that pattern is
preferred in the codebase.
In `@src/synthorg/integrations/oauth/token_manager.py`:
- Around line 241-266: The current code only handles TimeoutError when awaiting
the shielded drain_task, but if the caller cancels stop() before the timeout
fires, the cancellation is not properly handled. Add an except clause for
asyncio.CancelledError in addition to the existing TimeoutError handler that
performs the same operations: set _stop_failed to True, log an error message
indicating the cancellation, attach the same orphan-drain logging callback to
drain_task using add_done_callback with a note about the cancellation, and
re-raise the exception.
In `@src/synthorg/meta/mcp/domains/infrastructure.py`:
- Around line 203-209: Add a first-line module-kind header to the
infrastructure.py file to classify it as a declarative module. Since this file
serves as a declarative MCP tool registry and has grown beyond 500 lines of
code, add # module-kind: declarative as the very first line of the file. This
will exempt it from the default code size cap and allow it to continue growing
as a registry definition without violating module-size budgets.
In `@src/synthorg/workers/__main__.py`:
- Around line 321-331: Move the initialization of `backend` inside the try block
to ensure cleanup happens if `_build_seen_claims_backend()` raises an exception.
Currently, the call to `_build_seen_claims_backend(no_dedup=args.no_dedup)`
occurs before the try block, but if it fails after `_resolve_executor()` has
created the owned HTTP client, the finally cleanup block never executes and the
client doesn't get closed. Initialize `backend` to None before the try block,
then move the actual construction call `backend =
_build_seen_claims_backend(no_dedup=args.no_dedup)` inside the try block after
the try statement at line 331 to ensure the cleanup path is protected.
In `@tests/unit/api/controllers/test_conversational_intake_resume.py`:
- Around line 84-85: The count() method delegates to query() without overriding
the default limit parameter, which caps results at 100 and causes undercounting
when matching rows exceed that threshold. Fix this by passing a parameter to
query() that removes or bypasses the default limit constraint, ensuring the full
result set is counted and returned by the count() method. Refer to the query()
method signature to determine the appropriate parameter (likely limit or
similar) that should be passed to retrieve all matching rows without capping.
---
Outside diff comments:
In `@src/synthorg/api/controllers/_conversational_resume.py`:
- Around line 534-549: Move the participant duplicate check and cap enforcement
logic from the current location into the service.add_participant() call (or
create a new atomic service method that handles both operations together).
Currently, the check using active_participants() happens separately from the
add_participant() write, creating a race condition where concurrent requests can
both pass the cap check before either completes the insert. Refactor the service
or repository layer to perform the duplicate/cap validation and insert operation
as a single atomic transaction or with a backend lock/CAS mechanism, ensuring
that the roster cap cannot be exceeded even when multiple concurrent requests
arrive simultaneously.
In `@src/synthorg/api/controllers/_webhooks_wiring.py`:
- Around line 123-132: The idempotency key generation in the nonce handling
logic is vulnerable to collisions where a hashed nonce digest could be reused as
a raw nonce input, producing the same key for different webhook events. To fix
this, add domain-separating prefixes that distinguish between raw and hashed
nonce material before the _len_prefixed calls. Modify the logic so that when the
nonce is hashed (when its length exceeds MAX_NONCE_CHARS), it is prefixed with a
marker indicating it is hashed, and when using the raw nonce directly, it is
prefixed with a marker indicating it is raw. Apply these prefixes before passing
the nonce_for_key value to _len_prefixed in the raw_key construction.
---
Duplicate comments:
In `@src/synthorg/api/lifecycle_runner_startup.py`:
- Around line 437-441: The _wire_webhook_request_services function call is
currently inside the if persistence is not None: block, but it should be
executed unconditionally outside this gate. Move the
_wire_webhook_request_services(persistence, app_state) call after the if block
so it runs regardless of whether persistence is available, allowing the replay
protector to be properly wired even in no-persistence scenarios (where the
function will skip replay protection internally).
- Around line 208-233: The exception handler at line 208 only catches Exception,
which means asyncio.CancelledError (which inherits from BaseException) will
bypass the _cleanup_on_failure call and leak already-started services. Update
the except clause to catch both Exception and asyncio.CancelledError using
Python 3.14+ PEP 758 exception form syntax to ensure cleanup always executes
before re-raising.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aebf3a4c-fb5b-4818-a8f7-5352bbea102a
📒 Files selected for processing (96)
CLAUDE.mddata/architecture_report.jsondata/feature_index.jsondata/runtime_stats.yamldocs/reference/lifecycle-sync.mdsrc/synthorg/api/api_core_state.pysrc/synthorg/api/controllers/_conversational_resume.pysrc/synthorg/api/controllers/_webhooks_wiring.pysrc/synthorg/api/controllers/analytics/overview.pysrc/synthorg/api/controllers/analytics/trends.pysrc/synthorg/api/controllers/approvals/decisions.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/controllers/training.pysrc/synthorg/api/controllers/webhooks/_shared.pysrc/synthorg/api/controllers/webhooks/activity.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/lifecycle_helpers/conversational_wiring.pysrc/synthorg/api/lifecycle_runner_shutdown.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/lifecycle_runner_support.pysrc/synthorg/api/lifecycle_shared.pysrc/synthorg/api/services/analytics_read_service.pysrc/synthorg/api/state.pysrc/synthorg/backup/scheduler.pysrc/synthorg/backup/service.pysrc/synthorg/budget/errors.pysrc/synthorg/budget/quota_poller.pysrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/communication/bus/errors.pysrc/synthorg/engine/workflow/errors.pysrc/synthorg/engine/workflow/execution_node_dispatch.pysrc/synthorg/engine/workflow/execution_observer.pysrc/synthorg/engine/workflow/execution_service.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/hr/training/plan_service.pysrc/synthorg/idempotency/__init__.pysrc/synthorg/idempotency/service.pysrc/synthorg/integrations/errors.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/state.pysrc/synthorg/meta/chief_of_staff/resume_service.pysrc/synthorg/meta/mcp/domains/_remaining_args/_infrastructure.pysrc/synthorg/meta/mcp/domains/infrastructure.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/meta/state.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/errors.pysrc/synthorg/observability/events/workers.pysrc/synthorg/workers/__main__.pysrc/synthorg/workers/execution_service/_protocol.pysrc/synthorg/workers/heartbeat_subscriber.pytests/_shared/app_state.pytests/e2e/test_agent_invite_e2e.pytests/e2e/test_conversational_propose_e2e.pytests/unit/a2a/test_gateway.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_backup.pytests/unit/api/controllers/test_backup_required_idempotency.pytests/unit/api/controllers/test_conversational_intake_resume.pytests/unit/api/controllers/test_webhooks_idempotency.pytests/unit/api/controllers/test_webhooks_retry.pytests/unit/api/controllers/test_webhooks_service_layer.pytests/unit/api/services/test_analytics_read_service.pytests/unit/api/test_cleanup_on_failure.pytests/unit/api/test_lifecycle_builder_shutdown.pytests/unit/api/test_lifecycle_runner_support.pytests/unit/api/test_oauth_state_cleanup_wiring.pytests/unit/api/test_runtime_background_services_cleanup.pytests/unit/api/test_startup_wiring.pytests/unit/api/test_state.pytests/unit/backup/test_scheduler_lifecycle.pytests/unit/backup/test_service.pytests/unit/budget/test_quota_poller_lifecycle.pytests/unit/communication/bus/test_nats_stop_drain.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_execution_observer.pytests/unit/engine/workflow/test_execution_service.pytests/unit/engine/workflow/test_execution_service_subworkflows.pytests/unit/engine/workflow/test_subworkflow_service.pytests/unit/engine/workflow/test_webhook_bridge_lifecycle.pytests/unit/hr/training/test_plan_service.pytests/unit/idempotency/__init__.pytests/unit/idempotency/test_service.pytests/unit/integrations/oauth/test_token_manager_lifecycle.pytests/unit/integrations/rate_limiting/__init__.pytests/unit/integrations/rate_limiting/test_shared_state_lifecycle.pytests/unit/meta/chief_of_staff/test_invite.pytests/unit/meta/chief_of_staff/test_resume_service.pytests/unit/meta/mcp/test_handlers_infrastructure.pytests/unit/notifications/test_dispatcher_lifecycle.pytests/unit/workers/test_main.pyweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/backup.tsweb/src/api/idempotency.tsweb/src/api/types/openapi.gen.ts
💤 Files with no reviewable changes (1)
- src/synthorg/workers/execution_service/_protocol.py
Apply the ProviderHealthProber pattern (_stop_failed unrestartable flag + shielded drain-timeout) to the rate-limit coordinator, OAuth token manager, webhook bridge, quota poller, and notification dispatcher; fold the shield into the nats bus drain and surface the backup scheduler's flag via BackupService.is_unrestartable. Adds 4 reuse-RESOURCE_CONFLICT *UnrestartableError classes.
Wrap approvals approve/reject, backup restore, and the MCP backup-restore handler in IdempotencyService.run_idempotent behind a required Idempotency-Key; wire optional seen_claims dedup into the standalone worker (gated on DB config). Relocate IdempotencyService to a neutral synthorg.idempotency package so meta can use it without reaching into api.services. Regenerate OpenAPI TS mirrors for the new header params.
backup.py: the cached create/restore validation caught only (ValueError, TypeError), but pydantic v2 ValidationError inherits from neither, so a corrupt/stale cached payload leaked a raw 500 instead of the intended stable 5xx. Add ValidationError to both except tuples. e2e: the controller refactor routes conversational approval resume through ConversationalResumeService, but the propose + agent-invite dispatch states still wired the raw repos, so the resume flow 503'd (ServiceUnavailableError). Wire the service into both tests; make_app_state gains a conversational_resume_service kwarg.
…potency Lifecycle cancellation safety: webhook_bridge / oauth token_manager / rate_limiting shared_state reap their shielded drains when stop() itself is cancelled; workers _safe_cleanup swallows-then-re-raises CancelledError so all teardown runs; startup cleanup guard catches CancelledError. quota_poller + token_manager attach log_task_exceptions and clear done handles. Idempotency keys: approvals header bounded to 218 (UUID prefix + 255 column); backup restore key hashed over backup_id + components + raw key; webhooks nonce domain-separated (raw:/sha256:) and surrogate-tolerant. Dispatcher bounds the sink-close fan-out; api/state surfaces grace-window task failures; shutdown references the source grace constant; _cleanup_on_failure uses a declarative step table; analytics_read_service paginates the full set; execution_observer precedence docstring fixed; infrastructure MCP registry marked declarative; MCP backup handler threads the clock seam. Participant-cap TOCTOU: new atomic admit_active_within_cap (SQLite write-serialised, Postgres advisory-locked) via ConversationalResumeService + invite accept path, with dual-backend conformance coverage.
The fetch-all loop used while True + break, which the long-running-loop kill-switch gate flags as a daemon needing a kill-switch. Rewrite as a bounded while page: loop that terminates on the first short page.
Aureliolo
force-pushed
the
refactor/backend-runtime-2409
branch
from
June 20, 2026 07:20
b18fdaf to
ceff0ba
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
June 20, 2026 07:22 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/engine/workflow/webhook_bridge.py (1)
269-377: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftKeep
stop()under the 50-line function limit.Line 284 through Line 377 makes the method body far exceed the repository limit. Move the common cancel/drain/
wait_for(shield(...))/orphan-logging sequence into a reusable lifecycle helper and keep the webhook-specific unsubscribe handling here.As per coding guidelines, "
src/**/*.py: functions <50 lines" and "Do not suggest extracting single-use helper functions called exactly once."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/engine/workflow/webhook_bridge.py` around lines 269 - 377, The stop() method exceeds the 50-line repository limit for function bodies. Extract the common task cancellation, drain, and timeout handling logic (including the nested _drain() function and the asyncio.wait_for with shield call spanning lines 284-365) into a reusable lifecycle helper method. This helper should accept the task and timeout duration as parameters and handle the cancellation, draining with timeout protection, orphan logging on timeout, and interruption handling. Keep the webhook-specific unsubscribe logic and related error handling (the try block calling self._bus.unsubscribe) in the stop() method itself, calling the extracted helper to manage the poll-task lifecycle before attempting unsubscribe.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/api/controllers/backup.py`:
- Around line 134-137: The docstring for the restore function (which mirrors
_do_backup_as_dict) uses American English spelling "serialized" but the
repository follows British English conventions throughout comments and
docstrings. Change "JSON-serialized" to "JSON-serialised" in the docstring to
align with the repository's British English spelling guidelines.
In `@src/synthorg/api/lifecycle_runner_startup.py`:
- Around line 444-452: The call to _wire_webhook_request_services is currently
placed outside the guarded try/except block that invokes _safe_shutdown for
error handling, while _wire_workflow_observer and
_wire_workflow_execution_service are protected within that same block. If
_wire_webhook_request_services raises an exception, startup will exit without
triggering the cleanup path, leaving partially-started resources behind. Move
the _wire_webhook_request_services call inside the try block with the other
service wiring calls so that any exceptions raised during webhook
request-service initialization will be caught and handled by the same
startup-failure cleanup mechanism.
In `@src/synthorg/api/state.py`:
- Around line 193-195: The issue is that task.exception() is called
unconditionally on every task in the done set without first checking if the task
was cancelled. When a task is cancelled, calling exception() raises
CancelledError which aborts the drain before remaining stragglers are cancelled
and awaited. Guard the task.exception() call by first checking if the task is
cancelled using task.cancelled(), and only call exception() if the task is not
cancelled.
In `@src/synthorg/integrations/oauth/token_manager.py`:
- Around line 227-307: The stop() method exceeds the 50-line function limit due
to the lifecycle drain logic. Extract the internal _drain() function definition
and the subsequent asyncio.wait_for/asyncio.shield logic (including all
exception handling for TimeoutError and BaseException) into a shared reusable
helper function that can be called with token-manager-specific parameters. Call
this shared helper from stop() passing only the token-manager-specific values
such as the task to drain, timeout value (self._stop_drain_timeout_seconds),
logger, logging constants (OAUTH_TOKEN_REFRESH_FAILED), and the
log_task_exceptions callback. Ensure the drain_task, self._stop_failed flag, and
self._task cleanup remain in the stop() method after the helper call.
In `@src/synthorg/meta/mcp/handlers/infrastructure/backup.py`:
- Around line 267-269: The conflict path in the backup handler (the if
outcome.timed_out block) returns silently without logging, creating an
observability gap compared to other error branches in the same handler and the
REST controller. Import IDEMPOTENCY_CLAIM_IN_FLIGHT from the observability
events module, then add a log statement before the return statement in the if
outcome.timed_out condition to log this event, ensuring observability parity
with other error branches (lines 274, 277, 281) and the REST controller.
In `@src/synthorg/persistence/postgres/conversation_participant_repo.py`:
- Around line 352-382: The admit_active_within_cap method should enforce that
participants are persisted with ACTIVE status, but currently writes
participant.status.value to the database in the params tuple. Replace
participant.status.value with ConversationParticipantStatus.ACTIVE.value in the
params construction to ensure the method atomically admits participants as
active regardless of their input status, maintaining proper admission semantics
and cap accounting. The same fix should also be applied to any similar status
writes mentioned in the related code sections.
In `@tests/unit/api/services/test_analytics_read_service.py`:
- Around line 29-31: The assertion for the filter specification in the test only
validates the type with isinstance(spec, TaskFilterSpec), but does not verify
that the filter actually contains the expected default/empty values. To fix
this, add an additional assertion after the type check to compare the spec
object against TaskFilterSpec() with default values, or individually assert that
specific filter fields are set to their expected defaults. This ensures the test
validates the complete contract that the query is truly unfiltered, not just
that it's the correct type.
---
Outside diff comments:
In `@src/synthorg/engine/workflow/webhook_bridge.py`:
- Around line 269-377: The stop() method exceeds the 50-line repository limit
for function bodies. Extract the common task cancellation, drain, and timeout
handling logic (including the nested _drain() function and the asyncio.wait_for
with shield call spanning lines 284-365) into a reusable lifecycle helper
method. This helper should accept the task and timeout duration as parameters
and handle the cancellation, draining with timeout protection, orphan logging on
timeout, and interruption handling. Keep the webhook-specific unsubscribe logic
and related error handling (the try block calling self._bus.unsubscribe) in the
stop() method itself, calling the extracted helper to manage the poll-task
lifecycle before attempting unsubscribe.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 28cedb63-962e-430d-8a9e-9ef5719eee30
📒 Files selected for processing (102)
CLAUDE.mddata/architecture_report.jsondata/feature_index.jsondata/runtime_stats.yamldocs/reference/lifecycle-sync.mdsrc/synthorg/api/api_core_state.pysrc/synthorg/api/controllers/_conversational_resume.pysrc/synthorg/api/controllers/_webhooks_wiring.pysrc/synthorg/api/controllers/analytics/overview.pysrc/synthorg/api/controllers/analytics/trends.pysrc/synthorg/api/controllers/approvals/decisions.pysrc/synthorg/api/controllers/backup.pysrc/synthorg/api/controllers/training.pysrc/synthorg/api/controllers/webhooks/_shared.pysrc/synthorg/api/controllers/webhooks/activity.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/lifecycle_helpers/conversational_wiring.pysrc/synthorg/api/lifecycle_runner_shutdown.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/lifecycle_runner_support.pysrc/synthorg/api/lifecycle_shared.pysrc/synthorg/api/services/analytics_read_service.pysrc/synthorg/api/state.pysrc/synthorg/backup/scheduler.pysrc/synthorg/backup/service.pysrc/synthorg/budget/errors.pysrc/synthorg/budget/quota_poller.pysrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/communication/bus/errors.pysrc/synthorg/engine/workflow/errors.pysrc/synthorg/engine/workflow/execution_node_dispatch.pysrc/synthorg/engine/workflow/execution_observer.pysrc/synthorg/engine/workflow/execution_service.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/hr/training/plan_service.pysrc/synthorg/idempotency/__init__.pysrc/synthorg/idempotency/service.pysrc/synthorg/integrations/errors.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/state.pysrc/synthorg/meta/chief_of_staff/enums.pysrc/synthorg/meta/chief_of_staff/resume_service.pysrc/synthorg/meta/mcp/domains/_remaining_args/_infrastructure.pysrc/synthorg/meta/mcp/domains/infrastructure.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/meta/state.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/errors.pysrc/synthorg/observability/events/workers.pysrc/synthorg/persistence/conversation_participant_protocol.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/workers/__main__.pysrc/synthorg/workers/execution_service/_protocol.pysrc/synthorg/workers/heartbeat_subscriber.pytests/_shared/app_state.pytests/conformance/persistence/test_conversation_participant_repository.pytests/e2e/test_agent_invite_e2e.pytests/e2e/test_conversational_propose_e2e.pytests/unit/a2a/test_gateway.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_backup.pytests/unit/api/controllers/test_backup_required_idempotency.pytests/unit/api/controllers/test_conversational_intake_resume.pytests/unit/api/controllers/test_webhooks_idempotency.pytests/unit/api/controllers/test_webhooks_retry.pytests/unit/api/controllers/test_webhooks_service_layer.pytests/unit/api/services/test_analytics_read_service.pytests/unit/api/test_cleanup_on_failure.pytests/unit/api/test_lifecycle_builder_shutdown.pytests/unit/api/test_lifecycle_runner_support.pytests/unit/api/test_oauth_state_cleanup_wiring.pytests/unit/api/test_runtime_background_services_cleanup.pytests/unit/api/test_startup_wiring.pytests/unit/api/test_state.pytests/unit/backup/test_scheduler_lifecycle.pytests/unit/backup/test_service.pytests/unit/budget/test_quota_poller_lifecycle.pytests/unit/communication/bus/test_nats_stop_drain.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_execution_observer.pytests/unit/engine/workflow/test_execution_service.pytests/unit/engine/workflow/test_execution_service_subworkflows.pytests/unit/engine/workflow/test_subworkflow_service.pytests/unit/engine/workflow/test_webhook_bridge_lifecycle.pytests/unit/hr/training/test_plan_service.pytests/unit/idempotency/__init__.pytests/unit/idempotency/test_service.pytests/unit/integrations/oauth/test_token_manager_lifecycle.pytests/unit/integrations/rate_limiting/__init__.pytests/unit/integrations/rate_limiting/test_shared_state_lifecycle.pytests/unit/meta/chief_of_staff/group_chat_fakes.pytests/unit/meta/chief_of_staff/test_invite.pytests/unit/meta/chief_of_staff/test_resume_service.pytests/unit/meta/mcp/test_handlers_infrastructure.pytests/unit/notifications/test_dispatcher_lifecycle.pytests/unit/workers/test_main.pyweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/backup.tsweb/src/api/idempotency.tsweb/src/api/types/openapi.gen.ts
💤 Files with no reviewable changes (1)
- src/synthorg/workers/execution_service/_protocol.py
Address CodeRabbit findings: shared drain_lifecycle_task helper (DRY + 50-line limit), British spelling, participant ACTIVE-force in atomic admit, MCP idempotency-conflict warning log, startup webhook-wiring guard, cancelled-task guard in drain, analytics test assertion.
Aureliolo
temporarily deployed
to
cloudflare-preview
June 20, 2026 08:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/synthorg/engine/workflow/webhook_bridge.py (1)
77-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new constructor arguments.
config_resolverandclockwere added to__init__, but the classArgs:list still documents onlybusandceremony_scheduler.Proposed docstring update
Args: bus: The message bus instance. ceremony_scheduler: The ceremony scheduler holding the active sprint and strategy. + config_resolver: Optional resolver for operator-tuned bridge settings. + clock: Optional clock seam for poll and backoff sleeps.As per coding guidelines, "Type hints on public functions; mypy strict; Google-style docstrings; line length 88; functions <50 lines".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/engine/workflow/webhook_bridge.py` around lines 77 - 90, The docstring for the `__init__` method is missing documentation for the newly added keyword-only arguments `config_resolver` and `clock`. Update the `Args:` section of the docstring to include entries for both `config_resolver` (type ConfigResolver | None with description noting it's optional) and `clock` (type Clock | None with description noting it's optional), maintaining consistency with the existing documentation style for `bus` and `ceremony_scheduler`.Source: Coding guidelines
src/synthorg/api/controllers/backup.py (1)
376-390:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove the idempotency header limit into a named constant.
Line 389 adds a raw
255; this is outside the PydanticField(...)numeric exception and will trip the repository’s no-hardcoded-values convention gate. Reuse an existing idempotency key length constant if one exists, otherwise add a module-level annotated constant.Proposed fix
+_IDEMPOTENCY_KEY_MAX_LENGTH: Final[int] = 255 + ... - max_length=255, + max_length=_IDEMPOTENCY_KEY_MAX_LENGTH,As per coding guidelines, “No hardcoded numeric values; numerics live in
settings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/api/controllers/backup.py` around lines 376 - 390, The max_length parameter in the HeaderParameter definition for idempotency_key contains a hardcoded value of 255, which violates the repository's no-hardcoded-values convention. Replace this hardcoded 255 with a named constant by either locating an existing idempotency key length constant in the codebase (check settings/definitions/ or module constants), or by adding a new module-level annotated constant at the top of the backup.py file to define the maximum idempotency key length, then reference that constant in the HeaderParameter max_length parameter.Source: Coding guidelines
src/synthorg/meta/mcp/handlers/infrastructure/backup.py (1)
263-266:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHash the MCP restore idempotency key before claiming it.
Line 265 prefixes the caller key with
backup_id, so a max-length caller key can overflow the idempotency store’s 255-character key column. The REST restore path now hashes the same composite material beforerun_idempotent; apply the same fixed-width digest here.Proposed fix
+def _restore_idempotency_key(backup_id: str, idempotency_key: str) -> str: + """Return a fixed-width restore idempotency key.""" + material = f"{backup_id}:{idempotency_key}" + return hashlib.sha256(material.encode("utf-8")).hexdigest() + ... outcome = await service.run_idempotent( scope="mcp:backup_restore", - key=f"{backup_id}:{args.idempotency_key}", + key=_restore_idempotency_key(backup_id, args.idempotency_key), callback=_restore, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/meta/mcp/handlers/infrastructure/backup.py` around lines 263 - 266, The idempotency key constructed in the call to service.run_idempotent is formed by concatenating backup_id and args.idempotency_key, which can exceed the 255-character limit of the idempotency store's key column when the caller key is max-length. Hash the composite key material (the f-string combining backup_id and args.idempotency_key) using a fixed-width digest algorithm before passing it as the key parameter to run_idempotent, matching the approach already implemented in the REST restore path to ensure the final key always fits within the database column constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/observability/background_tasks.py`:
- Around line 292-320: The drain_lifecycle_task() function is missing an Args
section in its docstring to document the public parameters. Add a Google-style
Args section that documents all public parameters: task, timeout, logger_,
event, timeout_message, and log_fields. Include brief descriptions of what each
parameter represents and its purpose in the function's lifecycle task draining
operation.
---
Outside diff comments:
In `@src/synthorg/api/controllers/backup.py`:
- Around line 376-390: The max_length parameter in the HeaderParameter
definition for idempotency_key contains a hardcoded value of 255, which violates
the repository's no-hardcoded-values convention. Replace this hardcoded 255 with
a named constant by either locating an existing idempotency key length constant
in the codebase (check settings/definitions/ or module constants), or by adding
a new module-level annotated constant at the top of the backup.py file to define
the maximum idempotency key length, then reference that constant in the
HeaderParameter max_length parameter.
In `@src/synthorg/engine/workflow/webhook_bridge.py`:
- Around line 77-90: The docstring for the `__init__` method is missing
documentation for the newly added keyword-only arguments `config_resolver` and
`clock`. Update the `Args:` section of the docstring to include entries for both
`config_resolver` (type ConfigResolver | None with description noting it's
optional) and `clock` (type Clock | None with description noting it's optional),
maintaining consistency with the existing documentation style for `bus` and
`ceremony_scheduler`.
In `@src/synthorg/meta/mcp/handlers/infrastructure/backup.py`:
- Around line 263-266: The idempotency key constructed in the call to
service.run_idempotent is formed by concatenating backup_id and
args.idempotency_key, which can exceed the 255-character limit of the
idempotency store's key column when the caller key is max-length. Hash the
composite key material (the f-string combining backup_id and
args.idempotency_key) using a fixed-width digest algorithm before passing it as
the key parameter to run_idempotent, matching the approach already implemented
in the REST restore path to ensure the final key always fits within the database
column constraint.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f594e356-45c8-4190-b8f8-c084a8841a5c
📒 Files selected for processing (14)
data/architecture_report.jsondata/feature_index.jsondata/runtime_stats.yamlsrc/synthorg/api/controllers/backup.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pytests/unit/api/services/test_analytics_read_service.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Read design specification indocs/design/before implementing; deviations require approval per DESIGN_SPEC.md
Use metric units, no region/currency/locale privilege; use British English (Regional Defaults MANDATORY)
Every convention PR ships its enforcement gate per docs/reference/convention-gates.md (Convention Rollout MANDATORY)
No AGPL/GPL (non-LGPL) dependencies; LGPL deps (psycopg/psycopg_pool/psycopg_binary) must be attributed in NOTICE; golangci-lint stays external binary; pymupdf/fitz excluded (License Compatibility MANDATORY)
Configuration precedence: DB > env > code default via SettingsService/ConfigResolver (Cat-1) or env > code default (Cat-2, read_only_post_init); Cat-3 bootstrap secrets pure env; no os.environ.get outside startup (enforced by check_no_os_environ_outside_bootstrap.py; single-key reads allowed in bootstrap/entry-point/dynamic-secret-backend/Cat-3 allowlist or behind # lint-allow: env-read); pre-init Cat-2 reads use settings.bootstrap_resolver.resolve_init_value (Configuration Precedence MANDATORY)
No hardcoded numeric values; numerics live insettings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants (NAME: int|float|Final|Final[int]|Final[float] = literal) (No Hardcoded Values MANDATORY)
Each ErrorCode maps to exactly one DomainError subclass; exemptions: inheritance alias and per-category fallbacks in SHAREABLE_CODES; one-off twins opt out with # lint-allow: error-code-uniqueness -- (enforced by check_error_code_uniqueness.py) (Error-Code Uniqueness MANDATORY)
Module-size budget enforced by tiered LOC caps per # module-kind: header (controller 400, service/orchestrator 600, complex_service 1100, repository 500, adapter/integration 700, feature 100, code 500 default, tests 800, declarative exempt, generated glob-exempt) via check_module_size_budget.py + check_no_growth_in_god_modules.py (Module-Size Budget MANDATORY)
Import layering via declarative .impo...
Files:
tests/unit/api/services/test_analytics_read_service.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Timeout/slow failures indicate source-code regression; never edit tests/baselines/unit_timing.json or any scripts/baseline.{txt,json} / scripts/_baseline.py; PreToolUse-blocked (Test Regression MANDATORY)
Markers:@pytest.mark.{unit,integration,e2e,slow}; async auto; timeout 30s global; coverage 80% min
Test doubles: FakeClock for Clock seam, mock_ofT for typed-boundary substitutions, SimpleNamespace for attribute-bags; bare MagicMock at typed boundary blocked by scripts/check_mock_spec.py
Entity ids: as_uuid(label) for UUID PK fields, sid(label) for canonical-string FK / wire form, coerce_id(value) to normalise either to string, as_pk(value) to normalise to typed UUID (all from tests._shared); never bare uuid4() for cross-referenced id
API test client: HTTP tests use async_test_client fixture (LoopAsyncClient, portal-free); websocket tests use sync ws_test_client (litestar TestClient); Windows socket.socketpair retry wrapper permanent guard for CPython 122797
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add@example(...)); never skip/xfail, fix fundamentally
Files:
tests/unit/api/services/test_analytics_read_service.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/api/services/test_analytics_read_service.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Structured observability event names from observability.events. constants; sink pipeline (level + event filtering): synthorg.log excludes routine HTTP-request events; debug.log pins specific events; see .claude/skills/analyse-logs/SKILL.md for SINK_EVENT_EXCLUDES / SINK_EXACT_LEVELS
Telemetry: opt-in, off by default; every event property must be in _ALLOWED_PROPERTIES; see telemetry.md
Resilience: provider calls via BaseCompletionProvider (retry + rate limit, never implement retry in driver subclasses); retryable: RateLimitError, Provider{Timeout,Connection,Internal}Error; WebSocket: per-frame timeout closes silent peers (1008), revalidation saturation closes (4011); non-provider transient I/O (git push/fetch) uses core.resilience.GeneralRetryHandler with retryable predicate, never hand-rolled loop; see retry-patterns.md
Files:
src/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
src/**/*.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/persistence/**/*.py: Onlysrc/synthorg/persistence/may import sqlite/psycopg or emit raw SQL; inherit repository protocols frompersistence/_generics.py(SingletonRepository, IdKeyedRepository, FilteredQueryRepository, AppendOnlyRepository, StatefulRepository, MVCCRepository); bespoke methods permitted only under ADR-0001 D7 (Persistence Boundary MANDATORY)
Repository CRUD: save(entity), get(id), delete(id) -> bool, list_items(...), query(...) returning tuples
Datetime in persistence: parse_iso_utc / format_iso_utc from persistence._shared (reject naive); normalize_utc for already-typed
Files:
src/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/persistence/postgres/conversation_participant_repo.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
API startup lifecycle: construction phase wires synchronous services; on_startup wires persistence-dependent services; ordering invariants documented in CLAUDE.md; cost-dial services via _try_wire_cost_dial (best-effort, logs warning if absent); knowledge substrate via _wire_knowledge_engine (gated on has_persistence + has_memory_backend); EnvironmentService via _install_runtime_services (gated on has_persistence); steering wires in two phases; red-team completion gate stakes-gated via set_red_team_min_stakes; runtime services: build_runtime_services selects behind ONE provider-present switch
Files:
src/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/api/controllers/backup.py
🧠 Learnings (27)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T08:34:26.659Z
Learning: Present every plan for accept/deny before coding (MANDATORY planning step)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T08:34:26.659Z
Learning: Setup completion: post_setup_reinit() propagates failures and settings_svc.set("api", "setup_complete", "true") runs only if reinit clean; whole sequence serialised under COMPLETE_LOCK to prevent race on flag write
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T08:34:26.659Z
Learning: Git commits: <type>: <description> (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced; signed commits on protected refs (GPG/SSH or GitHub App); branches <type>/<slug> from main; squash merge with PR body as commit; trailers (Release-As, Closes `#N`) in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T08:34:26.659Z
Learning: Workflow: after every squash merge → /post-merge-cleanup; CLI is Docker-only (init/start/stop/status), features in dashboard + REST API
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
tests/unit/api/services/test_analytics_read_service.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-21T22:55:20.496Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/models.py:114-114
Timestamp: 2026-05-21T22:55:20.496Z
Learning: In this repo’s “magic number” review standard, the existing gate in `scripts/check_no_magic_numbers.py` intentionally does NOT flag numeric literals used as raw call-site arguments. So, do not flag numeric literals passed as keyword arguments to Pydantic `Field()` (e.g., `Field(ge=0, le=100)` / `Field(ge=1, le=50)`)—this is an established idiom. Only treat numeric literals as “magic numbers” when they occur in the locations the gate checks (module-level assignments and function/method parameter defaults).
Applied to files:
tests/unit/api/services/test_analytics_read_service.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-29T08:50:58.380Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2160
File: src/synthorg/persistence/sqlite/escalation_repo.py:370-370
Timestamp: 2026-05-29T08:50:58.380Z
Learning: In this repo, Ruff flake8-unused-arguments (ARG002) already suppresses unused-argument warnings on parameters of methods decorated with `override` (from `typing`). Therefore, if you see `# noqa: ARG002` (or equivalent) on parameters of an `override`-decorated method, treat it as stale/unused and remove it. Do not recommend re-adding `# noqa: ARG002` in these cases, because Ruff will flag the redundant directive (RUF100) and fail the Ruff CI gate.
Applied to files:
tests/unit/api/services/test_analytics_read_service.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-09T09:22:47.752Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2283
File: src/synthorg/engine/evolution/config.py:278-280
Timestamp: 2026-06-09T09:22:47.752Z
Learning: This repository uses ruff rule TC006 (`runtime-cast-value`), which requires the quoted string-literal form for the type argument in `cast()` calls (e.g., `cast("object", value)` rather than `cast(object, value)`). During code review, do not suggest removing the quotes from the `cast(<type>, ...)` first argument; the unquoted form will be auto-reverted by ruff on commit, so the quoted form should be treated as lint-compliant.
Applied to files:
tests/unit/api/services/test_analytics_read_service.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-23T12:24:00.128Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2080
File: tests/_shared/test_postgres_proxy.py:19-48
Timestamp: 2026-05-23T12:24:00.128Z
Learning: When creating test doubles for Python typing.Protocols in tests, prefer a hand-written Protocol fake (a concrete class that explicitly implements the Protocol) over `mock_of[T]` if the Protocol only defines annotation-only attributes (e.g., `username: str`, `password: str`, `dbname: str`) with no class-level values/assignments. This is because `mock_of[T]` relies on `create_autospec(..., spec_set=True)`, which enumerates members via `dir(spec)`; annotation-only attributes are not included, so `mock_of`’s kwarg-based attribute setting can raise `AttributeError: attribute not present on spec type`. In that annotation-only case, don’t recommend `mock_of[T]`—use an explicit fake class instead.
Applied to files:
tests/unit/api/services/test_analytics_read_service.py
📚 Learning: 2026-06-11T11:04:22.231Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2311
File: tests/conformance/persistence/test_workflow_service.py:77-77
Timestamp: 2026-06-11T11:04:22.231Z
Learning: When using Aureliolo/synthorg test-id helpers, treat `sid(label)`/`as_uuid(label)` as label-only functions that deterministically hash a human-readable label into a UUID (uuid5). Do NOT call them on an already-canonical UUID value (string or `UUID` object), since this re-hashes into a different UUID and breaks key matching/persistence. Instead:
- Use `coerce_id(value)` / `as_pk(value)` for pass-through-or-hash behavior (accept label or canonical UUID/`UUID` object and return the canonical form without re-hashing).
- If you already have a `UUID` object (e.g., `definition.id`), use `str(definition.id)` (or `coerce_id(definition.id)` / `as_pk(definition.id)`) for string-key conversion—never `sid(str(definition.id))`.
Applied to files:
tests/unit/api/services/test_analytics_read_service.py
📚 Learning: 2026-06-19T14:28:19.662Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2415
File: tests/unit/api/controllers/test_decomposition.py:118-139
Timestamp: 2026-06-19T14:28:19.662Z
Learning: For pytest test modules in this repo, if the module defines a module-level `pytestmark = pytest.mark.<marker>` (e.g., `pytest.mark.unit`, `pytest.mark.integration`, `pytest.mark.e2e`, `pytest.mark.slow`), then individual test functions in that module do not need their own per-function `pytest.mark.<marker>` decorators to satisfy the repo’s marker requirement. Only flag a missing per-function marker when the module does not set `pytestmark` and the test function also lacks the required `pytest.mark.<marker>` marker(s).
Applied to files:
tests/unit/api/services/test_analytics_read_service.py
📚 Learning: 2026-06-19T14:28:49.107Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2415
File: tests/unit/observability/audit_chain/test_signer.py:61-93
Timestamp: 2026-06-19T14:28:49.107Z
Learning: For pytest test modules under tests/, if the module defines a module-level marker like `pytestmark = pytest.mark.unit`, then the repository requirement “tests carry the unit marker” is satisfied for that module. In that case, do not flag individual test functions for missing `pytest.mark.unit` decorators solely because they lack per-function decorators—per-function markers would be redundant unless the module-level `pytestmark` is missing or not applicable.
Applied to files:
tests/unit/api/services/test_analytics_read_service.py
📚 Learning: 2026-06-03T11:43:13.104Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2200
File: tests/unit/engine/artifacts/test_service.py:42-45
Timestamp: 2026-06-03T11:43:13.104Z
Learning: For the D7 protocol method `save_returning_outcome(artifact: Artifact) -> bool` defined in `src/synthorg/persistence/artifact_protocol.py`, any implementation—including fake/stub/test doubles—must use the exact same parameter name `artifact` (i.e., `save_returning_outcome(self, artifact=...)` / `save_returning_outcome(self, artifact: Artifact)`), not `entity`. This name must match for typeguard positional-or-keyword name conformance. Do not suggest renaming the protocol method’s parameter to `entity`.
Applied to files:
tests/unit/api/services/test_analytics_read_service.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-09T10:06:53.040Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2283
File: src/synthorg/engine/middleware/coordination_constraints.py:152-152
Timestamp: 2026-06-09T10:06:53.040Z
Learning: Use International/British English spellings throughout the repository (code comments, docstrings, and documentation). British spellings such as "Analyse" (not "Analyze"), "Behaviour" (not "Behavior"), and "Colour" (not "Color") are mandatory and should not be flagged as spelling errors or inconsistencies during code review. This repo’s style is enforced via `vale` (see `CLAUDE.md` → "Regional Defaults (MANDATORY)"). If an American spelling appears, prefer the corresponding British spelling.
Applied to files:
tests/unit/api/services/test_analytics_read_service.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-13T08:51:11.124Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2348
File: docs/guides/custom-mcp-server-dev.md:155-161
Timestamp: 2026-06-13T08:51:11.124Z
Learning: When reviewing Aureliolo/synthorg usage of `mcp_descriptor()` (from `src/synthorg/meta/mcp/feature_descriptors.py`), ensure all call sites pass the keyword argument `handlers=...` (type `Callable[[], Mapping[str, object]]`). `mcp_descriptor()` maps this `handlers` value internally to the descriptor’s `handlers_factory` field; `handlers_factory=...` is not a valid keyword and will raise `TypeError: unexpected keyword argument`. Also ensure documentation and code examples do not suggest renaming `handlers` to `handlers_factory`; always show `handlers=`.
Applied to files:
tests/unit/api/services/test_analytics_read_service.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-11T17:01:48.351Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2321
File: .github/actions/start-postgres/action.yml:100-106
Timestamp: 2026-06-11T17:01:48.351Z
Learning: When using Docker CLI v29.5.3+ with `docker tag`, a digest-qualified reference is valid as the SOURCE but not as the TARGET. Specifically, `docker tag SOURCEsha256:<digest> TARGET:tag` should work, while the error like “refusing to create a tag with a digest reference” applies when the digest reference is used as the TARGET (e.g., `docker tag src imagesha256:<digest>`). A digest-qualified source resolves to a locally available image (typically one you’ve pulled) before tagging it with `TARGET:tag`.
Applied to files:
data/runtime_stats.yaml
📚 Learning: 2026-05-21T22:55:09.289Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/config.py:29-30
Timestamp: 2026-05-21T22:55:09.289Z
Learning: For this repo’s Pydantic configuration idiom, do not treat numeric literals passed directly as arguments to `pydantic.Field(...)` as “magic numbers” during review. This includes call-site usages like `Field(default=0.2, ge=0.0, le=1.0)` (e.g., in config models such as `ToolAuthoringConfig`, `ToolValidationConfig`, `ToolsmithConfig`). Do not request extracting those `Field(...)` numeric arguments into named constants, since the repo’s `scripts/check_no_magic_numbers.py` intentionally excludes call-site `Field(...)` numerics and relies on `Field(...)` as the canonical way to express these constraints/defaults.
Applied to files:
src/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-31T18:00:32.445Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2180
File: src/synthorg/engine/intervention/models.py:182-203
Timestamp: 2026-05-31T18:00:32.445Z
Learning: In this repository, `NotBlankStr` is a Pydantic `Annotated[str, ...]` type alias (defined in `synthorg/core/types.py`). At runtime, calling `NotBlankStr(value)` acts like an identity/cast to `str(value)` and does not execute the `StringConstraints` or `AfterValidator(...)`. Therefore, during code review, do not treat `NotBlankStr(x)` used inside non-Pydantic model methods as a place that would raise `ValidationError`; it won’t. Similarly, when `tuple[NotBlankStr, ...]` values are involved, `NotBlankStr` erases to `str` at runtime, so membership tests/comparisons can be done with raw `str` values.
Applied to files:
src/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-10T12:09:37.293Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2295
File: src/synthorg/project_brain/service.py:77-80
Timestamp: 2026-06-10T12:09:37.293Z
Learning: Do not flag or recommend moving imports out of `if TYPE_CHECKING:` in `src/synthorg` when the imported class is a collaborator type that’s intentionally “concrete-faked” in tests (duck-typed stubs injected into code under constructor/typing annotations). This pattern is used to avoid runtime import side effects where runtime type enforcement (e.g., `typeguard`/`isinstance` checks against the concrete type) would cause the test fakes to be rejected. If the code relies only on annotations for those collaborators and tests provide duck-typed stubs, keep the import under `TYPE_CHECKING` rather than promoting it to a module-level runtime import.
Applied to files:
src/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-10T12:09:46.221Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2295
File: src/synthorg/settings/dispatcher.py:43-46
Timestamp: 2026-06-10T12:09:46.221Z
Learning: In this repository’s Python modules, do not treat `if TYPE_CHECKING:` imports as violations of any “hoist to module level” rule when they are deliberately used to support duck-typed test fakes (“concrete-faked collaborators”).
Concretely: if the code includes a comment/docstring indicating that tests drive the module using a duck-typed stub (e.g., “Concrete-faked collaborator: tests drive … with a duck-typed … stub, so a runtime import would make typeguard reject the fake”), then keep that collaborator import/type reference under `if TYPE_CHECKING:` rather than importing it at runtime. The intent is to avoid runtime typeguard validation rejecting the test fake; these guards are intentional and should not be flagged.
Applied to files:
src/synthorg/observability/background_tasks.pysrc/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/persistence/postgres/conversation_participant_repo.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-03T11:43:33.228Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2200
File: src/synthorg/persistence/sqlite/flight_recorder_repo.py:250-252
Timestamp: 2026-06-03T11:43:33.228Z
Learning: In Aureliolo/synthorg persistence repositories under src/synthorg/persistence/**/*.py, follow the existing error-path logging conventions:
1) For operational persistence failures (e.g., sqlite3/aiosqlite errors), emit a logger.warning before raising QueryError, using the form logger.warning(EVENT, error_type=..., error=safe_error_description(exc)), then raise QueryError.
2) For input-validation/guard paths (e.g., precondition checks like naive datetime checks such as if threshold.tzinfo is None: raise QueryError(...)), raise QueryError directly without any preceding logger.warning call.
Do not suggest adding a warning log before raise on input-validation guard paths.
Applied to files:
src/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/persistence/postgres/conversation_participant_repo.py
📚 Learning: 2026-06-03T11:43:33.228Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2200
File: src/synthorg/persistence/sqlite/flight_recorder_repo.py:250-252
Timestamp: 2026-06-03T11:43:33.228Z
Learning: When reviewing naive-datetime guards in persistence repositories under src/synthorg/persistence/** (e.g., in SQLite/Postgres flight recorder repos), follow the project’s deliberate consistency rule: guard naive datetimes using only `if threshold.tzinfo is None:` and do not add an additional `threshold.utcoffset() is None` check. Avoid suggesting the `utcoffset()` branch to keep behavior consistent with sibling repositories.
Applied to files:
src/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/persistence/postgres/conversation_participant_repo.py
📚 Learning: 2026-06-03T14:28:35.042Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2200
File: src/synthorg/persistence/sqlite/idempotency_repo.py:90-90
Timestamp: 2026-06-03T14:28:35.042Z
Learning: In the synthorg persistence layer, treat `normalize_utc(value: datetime)` as “coerce-by-design”: for valid `datetime` inputs it should not raise (naive datetimes are assigned UTC via `value.replace(tzinfo=UTC)`, aware datetimes are converted via `value.astimezone(UTC)`). During code review, don’t recommend wrapping `normalize_utc(...)` calls (e.g., `normalize_utc(now)`) in try/except to convert exceptions into persistence `QueryError`s, because that would mask programming bugs (non-`datetime` inputs). Instead, enforce the actual input-validation boundary used in this repo’s `purge_before` methods: use the explicit guard `if threshold.tzinfo is None: raise QueryError(...)` rather than relying on exception handling around `normalize_utc`.
Applied to files:
src/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/persistence/postgres/conversation_participant_repo.py
📚 Learning: 2026-06-09T08:55:30.949Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2282
File: src/synthorg/persistence/postgres/flight_recorder_repo.py:13-13
Timestamp: 2026-06-09T08:55:30.949Z
Learning: In Aureliolo/synthorg persistence modules, keep “type-only” imports (e.g., `from psycopg_pool import AsyncConnectionPool`) at module scope when they are deliberately hoisted so runtime annotation checking (e.g., via `typeguard`) can resolve annotations. During code review, do not suggest moving these imports under `TYPE_CHECKING` unless you can point to a proven import cycle or a concrete runtime failure in a supported install/configuration.
Applied to files:
src/synthorg/persistence/sqlite/conversation_participant_repo.pysrc/synthorg/persistence/postgres/conversation_participant_repo.py
📚 Learning: 2026-05-30T18:10:10.435Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2171
File: src/synthorg/api/auth/controllers/sessions_mgmt.py:78-79
Timestamp: 2026-05-30T18:10:10.435Z
Learning: When reviewing Python code under src/synthorg/api, do not flag callsites that use `session_store_of(app_state)` as unsafe/bare field dereferences for missing-session-store scenarios. `session_store_of(app_state)` already routes through `require_service(slice.session_store, 'Session Store')`, which raises `ServiceUnavailableError` (HTTP 503) when the session store is not wired (e.g., JWT-only deployments). The existing 503 guard prevents the missing-store from becoming an unsafe access.
Applied to files:
src/synthorg/api/lifecycle_runner_startup.pysrc/synthorg/api/state.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-09T17:05:23.619Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/quality.py:44-45
Timestamp: 2026-06-09T17:05:23.619Z
Learning: In the meta MCP handler modules under `src/synthorg/meta/mcp/handlers/`, `AppState` imported from `synthorg.api.state` is intentionally done only inside `if TYPE_CHECKING:` (not at module import time). This avoids a meta→api runtime dependency/back-edge that would violate the project’s package layering. During code review, do NOT flag the `AppState` `TYPE_CHECKING` guard in these meta handlers as a layering or import-structure violation; mypy should validate the type usage statically, and runtime behavior intentionally omits the import.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T17:05:38.738Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/workflow_executions.py:55-57
Timestamp: 2026-06-09T17:05:38.738Z
Learning: In MCP meta handler modules under `src/synthorg/meta/mcp/handlers/`, do not flag `from synthorg.api.state import AppState` as a violation if it is intentionally placed at module scope inside an `if TYPE_CHECKING:` block. This is an explicit layering exception: importing `AppState` at runtime would create a meta→api back-edge, but keeping it under `TYPE_CHECKING` preserves static checking (e.g., mypy) without introducing the runtime dependency (e.g., typeguard behavior).
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T17:07:02.613Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/charter.py:37-38
Timestamp: 2026-06-09T17:07:02.613Z
Learning: In files under `src/synthorg/meta/mcp/handlers/`, `AppState` imported from `synthorg.api.state` should remain inside `if TYPE_CHECKING:` (i.e., not hoisted to module scope). This is intentional to avoid runtime imports/Typeguard enforcement (which would break handler unit tests that inject a lightweight `SimpleNamespace`) and to prevent a runtime meta→api back-edge that this package’s layering avoids. `AppState` annotations will still be validated statically by mypy; typeguard is expected to skip the TYPE_CHECKING-only import here. Do not flag `AppState` under `TYPE_CHECKING` as a violation of “type-only imports at module level” for this handler package.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-04T06:19:35.212Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2216
File: src/synthorg/persistence/postgres/benchmark_score_repo.py:10-13
Timestamp: 2026-06-04T06:19:35.212Z
Learning: In `src/synthorg/persistence/postgres/` repository modules, import `psycopg_pool.AsyncConnectionPool` only inside `if TYPE_CHECKING:` blocks (never at module scope). This is intentional because `psycopg_pool` exists only when the optional `postgres` extra is installed; module-scope imports would make these modules unimportable on SQLite-only installs. For this specific repo/dependency pattern, do not treat `AsyncConnectionPool` imports under `TYPE_CHECKING` as a runtime-introspection/security risk—CI/type correctness gates validate the usage.
Applied to files:
src/synthorg/persistence/postgres/conversation_participant_repo.py
📚 Learning: 2026-06-09T08:55:30.949Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2282
File: src/synthorg/persistence/postgres/flight_recorder_repo.py:13-13
Timestamp: 2026-06-09T08:55:30.949Z
Learning: In this repo’s Postgres persistence code (src/synthorg/persistence/postgres/**/*.py), do not treat module-scope imports of `psycopg_pool.AsyncConnectionPool` as optional-extra import regressions. `psycopg_pool` is shipped by the same `postgres` extra as `psycopg`, and there is no supported install scenario where the SQLite backend would be importable while `psycopg_pool` is absent.
Applied to files:
src/synthorg/persistence/postgres/conversation_participant_repo.py
🔇 Additional comments (13)
src/synthorg/engine/workflow/webhook_bridge.py (1)
32-35: LGTM!Also applies to: 94-113, 216-270, 272-326, 368-393, 464-485
src/synthorg/observability/background_tasks.py (1)
24-24: LGTM!Also applies to: 321-385
src/synthorg/persistence/postgres/conversation_participant_repo.py (1)
379-381: LGTM!Also applies to: 352-427
src/synthorg/persistence/sqlite/conversation_participant_repo.py (1)
411-413: LGTM!Also applies to: 384-459
tests/unit/api/services/test_analytics_read_service.py (1)
20-34: LGTM!data/runtime_stats.yaml (1)
2-6: LGTM!src/synthorg/api/controllers/backup.py (1)
129-145: LGTM!Also applies to: 400-452, 500-523
src/synthorg/api/lifecycle_runner_startup.py (2)
669-669: LGTM!
449-454: This exception guard is not vulnerable to cancellation bypass.
_wire_workflow_observer()is defined asasync defbut contains noawaitstatements, so theawaitat line 452 completes synchronously without creating a suspension point whereasyncio.CancelledErrorcan be raised. Both_wire_webhook_request_services()and_wire_workflow_execution_service()are synchronous functions. Sinceasyncio.CancelledErrorcan only be raised at actualawaitpoints, this block has no cancellation exposure despite usingexcept Exception.> Likely an incorrect or invalid review comment.src/synthorg/api/state.py (1)
193-202: LGTM!src/synthorg/integrations/oauth/token_manager.py (1)
33-36: LGTM!Also applies to: 92-96, 185-255
src/synthorg/integrations/rate_limiting/shared_state.py (1)
32-32: LGTM!Also applies to: 95-99, 117-128, 199-212
src/synthorg/meta/mcp/handlers/infrastructure/backup.py (1)
48-48: LGTM!Also applies to: 259-262, 268-280, 294-294
Hash MCP backup-restore idempotency key (255-char column overflow, Major); extract idempotency-key max-length to a named constant in backup controller (both header bounds); document drain_lifecycle_task Args; document webhook bridge config_resolver/clock constructor args.
Aureliolo
temporarily deployed
to
cloudflare-preview
June 20, 2026 09:02 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/meta/mcp/handlers/infrastructure/backup.py (1)
295-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate cached restore payload before returning success.
outcome.resultis returned directly from persisted idempotency state. Unlike the REST restore path, this flow does not re-validate cached payload shape before emittingok(...), so stale/corrupt cache rows can produce malformed success responses. Please validateoutcome.resultagainst the restore response schema before returning and map validation failure to an error that forces a rerun.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/meta/mcp/handlers/infrastructure/backup.py` around lines 295 - 309, The code returns the cached `outcome.result` directly without validating its schema, which can cause stale or corrupted cache entries to produce malformed success responses. After the line `payload = outcome.result`, add validation of the payload against the restore response schema before the final return statement. If validation fails, map the validation error to an error response using the err() function instead of returning ok(payload), ensuring that schema validation failures force a rerun rather than returning invalid data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/synthorg/meta/mcp/handlers/infrastructure/backup.py`:
- Around line 295-309: The code returns the cached `outcome.result` directly
without validating its schema, which can cause stale or corrupted cache entries
to produce malformed success responses. After the line `payload =
outcome.result`, add validation of the payload against the restore response
schema before the final return statement. If validation fails, map the
validation error to an error response using the err() function instead of
returning ok(payload), ensuring that schema validation failures force a rerun
rather than returning invalid data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33c0dbc6-57b3-40a0-8f8c-029215d3f6ce
📒 Files selected for processing (7)
data/architecture_report.jsondata/feature_index.jsondata/runtime_stats.yamlsrc/synthorg/api/controllers/backup.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: Build Backend
- GitHub Check: Dashboard Test
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: Test Integration (shard 4)
- GitHub Check: Test Integration (shard 2)
- GitHub Check: Test Integration (shard 1)
- GitHub Check: Test Unit (shard 4)
- GitHub Check: Test Integration (shard 3)
- GitHub Check: Test Unit (shard 2)
- GitHub Check: Test Unit (shard 1)
- GitHub Check: Test Unit (shard 3)
- GitHub Check: Test Conformance (SQLite)
- GitHub Check: Gates (pre-commit all-files + parity)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: DB > env > code default viaSettingsService/ConfigResolver(Cat-1) or env > code default (Cat-2,read_only_post_init); Cat-3 bootstrap secrets are pure env at the boot site. YAML is a company-template ingestion format, not a precedence tier. Noos.environ.getoutside startup (enforced bycheck_no_os_environ_outside_bootstrap.py: single-key reads allowed only in the bootstrap/entry-point/dynamic-secret-backend/Cat-3 allowlist or behind# lint-allow: env-read); pre-init Cat-2 reads usesettings.bootstrap_resolver.resolve_init_value.
Numerics live insettings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of the formNAME: int|float|Final|Final[int]|Final[float] = literal. Enforced byscripts/check_no_magic_numbers.py.
EachErrorCodemaps to exactly oneDomainErrorsubclass so clients can branch onerror_code. Two exemptions: an inheritance alias (a subclass keeping an ancestor's code) and the generic per-category fallbacks in the gate'sSHAREABLE_CODES; one-off intentional twins opt out with# lint-allow: error-code-uniqueness -- <reason>. Enforced byscripts/check_error_code_uniqueness.py.
Module-size budget tiered per# module-kind:header:controller400,service/orchestrator600,complex_service1100,repository500,adapter/integration700,feature100,code500 (default),tests800,declarativeexempt,generatedglob-exempt. Enforced bycheck_module_size_budget.py+check_no_growth_in_god_modules.py.
Declarative.importlintercontracts (forbidden-only, direct-imports, blessed back-edges; NO total-order layers) enforced bylint-imports(pre-push + CI), alongside 3 retained custom AST gates (raw-SQL boundary, DTO-leak, dependency-inversion). Graph-level smells (fan-in >=30, LCOM4, budget-pressure within 20% of tier cap) gated bycheck_architecture_drift.pyvs committeddata/architecture_report.json.
Comments WHY only;...
Files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Fromsynthorg.observability import get_logger; variable alwayslogger. Neverimport logging/print()in app code. Event names fromobservability.events.<domain>constants; structured kwargs (logger.info(EVENT, key=value)). Error paths log WARNING/ERROR with context before raising; state transitions log INFO via*_STATUS_TRANSITIONEDAFTER persistence write.
Secret-log redaction (SEC-1): nevererror=str(exc)or interpolate{exc}; useerror_type=type(exc).__name__+error=safe_error_description(exc). Neverexc_info=True. Neverlogger.exception(...)(attaches traceback whose frame-locals serialise in-scope secrets); replace withexcept ... as exc: logger.error(EVENT, ..., error_type=type(exc).__name__, error=safe_error_description(exc)). OTel:span.record_exception(exc)forbidden; usespan.set_attribute("exception.message", safe_error_description(exc))+record_exception=False, set_status_on_exception=False. Enforced bycheck_logger_exception_str_exc.py.
Telemetry: opt-in, off by default. Every event property must be in_ALLOWED_PROPERTIES. See telemetry.md.
Provider calls go throughBaseCompletionProvider(retry + rate limit); never implement retry in driver subclasses. Retryable:RateLimitError,Provider{Timeout,Connection,Internal}Error. WebSocket: per-frame timeout closes silent peers (1008); revalidation saturation closes (4011). Non-provider transient I/O usescore.resilience.GeneralRetryHandlerwith aretryablepredicate, never hand-rolled loops; see retry-patterns.md.
Files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
src/synthorg/meta/mcp/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
MCP: 240+ tools across 21 domain modules under
meta/mcp/domains/. DefineToolHandler+args_model; callrequire_admin_guardrails()on admin tools; route through service layers. See mcp-handler-contract.md.
Files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
API startup lifecycle: construction phase (create_app body) wires synchronous services; on_startup (_build_lifecycle.on_startup) wires services needing a connected persistence backend. Construction-phase ordering:
agent_registrybeforeauto_wire_meetings;tunnel_providerwired unconditionally.
Files:
src/synthorg/api/controllers/backup.py
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: Read `docs/design/` page before implementing; deviations need approval. See DESIGN_SPEC.md.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: Present every plan for accept/deny before coding.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: No region/locale privileged; use metric units; British English. See docs/reference/regional-defaults.md.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: Every convention PR ships its enforcement gate. See docs/reference/convention-gates.md.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: No AGPL/GPL (non-LGPL) dependency may ship; LGPL deps (`psycopg`/`psycopg_pool`/`psycopg_binary`) MUST be attributed in `NOTICE`; `golangci-lint` stays an external binary (never a `cli/go.mod` dependency); `pymupdf`/`fitz` (AGPL) stay excluded from all dependency tables. Enforced by `scripts/check_license_compat.py`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: Timeout/slow failures = source-code regression; never edit `tests/baselines/unit_timing.json` or any `scripts/*_baseline.{txt,json}` / `scripts/_*_baseline.py`. Both families are PreToolUse-blocked.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: After issue: branch + commit + push (no auto-PR); use `/pre-pr-review` (`gh pr create` is blocked by `scripts/check_no_pr_create.sh`). After PR: `/aurelio-review-pr` for external feedback. Fix EVERYTHING valid; no deferring.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced. Branches: `<type>/<slug>` from main. Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: Pre-commit/pre-push hooks: `.pre-commit-config.yaml`. Tool-call gates: `.claude/settings.json` PreToolUse (`scripts/check_*.sh`/`.py`). A failed pre-push leaves a `<hook>-FAILED` marker under `synthorg-hooks/` that blocks the next push; clear it with `bash scripts/clear_prepush_marker.sh` (never a raw `rm` of the marker).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: Squash merge. PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body. GitHub queries: `gh issue list` via Bash, NOT MCP `list_issues`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:01:42.366Z
Learning: After every squash merge → `/post-merge-cleanup`. CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API.
📚 Learning: 2026-06-11T17:01:48.351Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2321
File: .github/actions/start-postgres/action.yml:100-106
Timestamp: 2026-06-11T17:01:48.351Z
Learning: When using Docker CLI v29.5.3+ with `docker tag`, a digest-qualified reference is valid as the SOURCE but not as the TARGET. Specifically, `docker tag SOURCEsha256:<digest> TARGET:tag` should work, while the error like “refusing to create a tag with a digest reference” applies when the digest reference is used as the TARGET (e.g., `docker tag src imagesha256:<digest>`). A digest-qualified source resolves to a locally available image (typically one you’ve pulled) before tagging it with `TARGET:tag`.
Applied to files:
data/runtime_stats.yaml
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-21T22:55:20.496Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/models.py:114-114
Timestamp: 2026-05-21T22:55:20.496Z
Learning: In this repo’s “magic number” review standard, the existing gate in `scripts/check_no_magic_numbers.py` intentionally does NOT flag numeric literals used as raw call-site arguments. So, do not flag numeric literals passed as keyword arguments to Pydantic `Field()` (e.g., `Field(ge=0, le=100)` / `Field(ge=1, le=50)`)—this is an established idiom. Only treat numeric literals as “magic numbers” when they occur in the locations the gate checks (module-level assignments and function/method parameter defaults).
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-29T08:50:58.380Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2160
File: src/synthorg/persistence/sqlite/escalation_repo.py:370-370
Timestamp: 2026-05-29T08:50:58.380Z
Learning: In this repo, Ruff flake8-unused-arguments (ARG002) already suppresses unused-argument warnings on parameters of methods decorated with `override` (from `typing`). Therefore, if you see `# noqa: ARG002` (or equivalent) on parameters of an `override`-decorated method, treat it as stale/unused and remove it. Do not recommend re-adding `# noqa: ARG002` in these cases, because Ruff will flag the redundant directive (RUF100) and fail the Ruff CI gate.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-09T09:22:47.752Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2283
File: src/synthorg/engine/evolution/config.py:278-280
Timestamp: 2026-06-09T09:22:47.752Z
Learning: This repository uses ruff rule TC006 (`runtime-cast-value`), which requires the quoted string-literal form for the type argument in `cast()` calls (e.g., `cast("object", value)` rather than `cast(object, value)`). During code review, do not suggest removing the quotes from the `cast(<type>, ...)` first argument; the unquoted form will be auto-reverted by ruff on commit, so the quoted form should be treated as lint-compliant.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-21T22:55:09.289Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/config.py:29-30
Timestamp: 2026-05-21T22:55:09.289Z
Learning: For this repo’s Pydantic configuration idiom, do not treat numeric literals passed directly as arguments to `pydantic.Field(...)` as “magic numbers” during review. This includes call-site usages like `Field(default=0.2, ge=0.0, le=1.0)` (e.g., in config models such as `ToolAuthoringConfig`, `ToolValidationConfig`, `ToolsmithConfig`). Do not request extracting those `Field(...)` numeric arguments into named constants, since the repo’s `scripts/check_no_magic_numbers.py` intentionally excludes call-site `Field(...)` numerics and relies on `Field(...)` as the canonical way to express these constraints/defaults.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-31T18:00:32.445Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2180
File: src/synthorg/engine/intervention/models.py:182-203
Timestamp: 2026-05-31T18:00:32.445Z
Learning: In this repository, `NotBlankStr` is a Pydantic `Annotated[str, ...]` type alias (defined in `synthorg/core/types.py`). At runtime, calling `NotBlankStr(value)` acts like an identity/cast to `str(value)` and does not execute the `StringConstraints` or `AfterValidator(...)`. Therefore, during code review, do not treat `NotBlankStr(x)` used inside non-Pydantic model methods as a place that would raise `ValidationError`; it won’t. Similarly, when `tuple[NotBlankStr, ...]` values are involved, `NotBlankStr` erases to `str` at runtime, so membership tests/comparisons can be done with raw `str` values.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-10T12:09:37.293Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2295
File: src/synthorg/project_brain/service.py:77-80
Timestamp: 2026-06-10T12:09:37.293Z
Learning: Do not flag or recommend moving imports out of `if TYPE_CHECKING:` in `src/synthorg` when the imported class is a collaborator type that’s intentionally “concrete-faked” in tests (duck-typed stubs injected into code under constructor/typing annotations). This pattern is used to avoid runtime import side effects where runtime type enforcement (e.g., `typeguard`/`isinstance` checks against the concrete type) would cause the test fakes to be rejected. If the code relies only on annotations for those collaborators and tests provide duck-typed stubs, keep the import under `TYPE_CHECKING` rather than promoting it to a module-level runtime import.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-10T12:09:46.221Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2295
File: src/synthorg/settings/dispatcher.py:43-46
Timestamp: 2026-06-10T12:09:46.221Z
Learning: In this repository’s Python modules, do not treat `if TYPE_CHECKING:` imports as violations of any “hoist to module level” rule when they are deliberately used to support duck-typed test fakes (“concrete-faked collaborators”).
Concretely: if the code includes a comment/docstring indicating that tests drive the module using a duck-typed stub (e.g., “Concrete-faked collaborator: tests drive … with a duck-typed … stub, so a runtime import would make typeguard reject the fake”), then keep that collaborator import/type reference under `if TYPE_CHECKING:` rather than importing it at runtime. The intent is to avoid runtime typeguard validation rejecting the test fake; these guards are intentional and should not be flagged.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-03T11:43:13.104Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2200
File: tests/unit/engine/artifacts/test_service.py:42-45
Timestamp: 2026-06-03T11:43:13.104Z
Learning: For the D7 protocol method `save_returning_outcome(artifact: Artifact) -> bool` defined in `src/synthorg/persistence/artifact_protocol.py`, any implementation—including fake/stub/test doubles—must use the exact same parameter name `artifact` (i.e., `save_returning_outcome(self, artifact=...)` / `save_returning_outcome(self, artifact: Artifact)`), not `entity`. This name must match for typeguard positional-or-keyword name conformance. Do not suggest renaming the protocol method’s parameter to `entity`.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-09T10:06:53.040Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2283
File: src/synthorg/engine/middleware/coordination_constraints.py:152-152
Timestamp: 2026-06-09T10:06:53.040Z
Learning: Use International/British English spellings throughout the repository (code comments, docstrings, and documentation). British spellings such as "Analyse" (not "Analyze"), "Behaviour" (not "Behavior"), and "Colour" (not "Color") are mandatory and should not be flagged as spelling errors or inconsistencies during code review. This repo’s style is enforced via `vale` (see `CLAUDE.md` → "Regional Defaults (MANDATORY)"). If an American spelling appears, prefer the corresponding British spelling.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-06-09T17:05:23.619Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/quality.py:44-45
Timestamp: 2026-06-09T17:05:23.619Z
Learning: In the meta MCP handler modules under `src/synthorg/meta/mcp/handlers/`, `AppState` imported from `synthorg.api.state` is intentionally done only inside `if TYPE_CHECKING:` (not at module import time). This avoids a meta→api runtime dependency/back-edge that would violate the project’s package layering. During code review, do NOT flag the `AppState` `TYPE_CHECKING` guard in these meta handlers as a layering or import-structure violation; mypy should validate the type usage statically, and runtime behavior intentionally omits the import.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T17:05:38.738Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/workflow_executions.py:55-57
Timestamp: 2026-06-09T17:05:38.738Z
Learning: In MCP meta handler modules under `src/synthorg/meta/mcp/handlers/`, do not flag `from synthorg.api.state import AppState` as a violation if it is intentionally placed at module scope inside an `if TYPE_CHECKING:` block. This is an explicit layering exception: importing `AppState` at runtime would create a meta→api back-edge, but keeping it under `TYPE_CHECKING` preserves static checking (e.g., mypy) without introducing the runtime dependency (e.g., typeguard behavior).
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T17:07:02.613Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/charter.py:37-38
Timestamp: 2026-06-09T17:07:02.613Z
Learning: In files under `src/synthorg/meta/mcp/handlers/`, `AppState` imported from `synthorg.api.state` should remain inside `if TYPE_CHECKING:` (i.e., not hoisted to module scope). This is intentional to avoid runtime imports/Typeguard enforcement (which would break handler unit tests that inject a lightweight `SimpleNamespace`) and to prevent a runtime meta→api back-edge that this package’s layering avoids. `AppState` annotations will still be validated statically by mypy; typeguard is expected to skip the TYPE_CHECKING-only import here. Do not flag `AppState` under `TYPE_CHECKING` as a violation of “type-only imports at module level” for this handler package.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-13T08:51:11.124Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2348
File: docs/guides/custom-mcp-server-dev.md:155-161
Timestamp: 2026-06-13T08:51:11.124Z
Learning: When reviewing Aureliolo/synthorg usage of `mcp_descriptor()` (from `src/synthorg/meta/mcp/feature_descriptors.py`), ensure all call sites pass the keyword argument `handlers=...` (type `Callable[[], Mapping[str, object]]`). `mcp_descriptor()` maps this `handlers` value internally to the descriptor’s `handlers_factory` field; `handlers_factory=...` is not a valid keyword and will raise `TypeError: unexpected keyword argument`. Also ensure documentation and code examples do not suggest renaming `handlers` to `handlers_factory`; always show `handlers=`.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.pysrc/synthorg/observability/background_tasks.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/api/controllers/backup.py
📚 Learning: 2026-05-30T18:10:10.435Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2171
File: src/synthorg/api/auth/controllers/sessions_mgmt.py:78-79
Timestamp: 2026-05-30T18:10:10.435Z
Learning: When reviewing Python code under src/synthorg/api, do not flag callsites that use `session_store_of(app_state)` as unsafe/bare field dereferences for missing-session-store scenarios. `session_store_of(app_state)` already routes through `require_service(slice.session_store, 'Session Store')`, which raises `ServiceUnavailableError` (HTTP 503) when the session store is not wired (e.g., JWT-only deployments). The existing 503 guard prevents the missing-store from becoming an unsafe access.
Applied to files:
src/synthorg/api/controllers/backup.py
🔇 Additional comments (6)
data/runtime_stats.yaml (1)
2-3: LGTM!src/synthorg/api/controllers/backup.py (1)
66-68: LGTM!Also applies to: 187-187, 379-394, 443-455, 503-526
src/synthorg/engine/workflow/webhook_bridge.py (1)
81-82: LGTM!Also applies to: 219-252, 283-326, 395-395, 472-476, 487-487
src/synthorg/observability/background_tasks.py (1)
317-323: LGTM!data/architecture_report.json (1)
87-92: LGTM!Also applies to: 599-600
data/feature_index.json (1)
56-69: LGTM!Also applies to: 710-725, 1285-1285
MCP backup-restore returned cached idempotency-store result directly; a stale/corrupt cache row could emit a malformed success. Added RestoreConfirmation model and validate outcome.result before ok(), returning RestoreError to force a rerun on failure (mirrors the REST restore path).
Aureliolo
temporarily deployed
to
cloudflare-preview
June 20, 2026 09:23 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/meta/mcp/handlers/infrastructure/backup.py`:
- Around line 298-305: The validation of the payload using
RestoreConfirmation.model_validate(payload) discards the validated and
potentially coerced model instance, then returns the raw original payload dict
to ok(). Capture the result of the model_validate() call into a variable, then
use that validated model instance (serialized via .model_dump()) when calling
ok() instead of passing the uncorrected payload dict, ensuring type coercions
and field normalizations performed by Pydantic are preserved in the response.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d1211108-603d-4090-8e85-7635aa56288a
📒 Files selected for processing (4)
data/feature_index.jsondata/runtime_stats.yamlsrc/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Dashboard Test
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Test Unit (shard 1)
- GitHub Check: Test Unit (shard 4)
- GitHub Check: Test Integration (shard 3)
- GitHub Check: Test Integration (shard 2)
- GitHub Check: Test Conformance (SQLite)
- GitHub Check: Test Integration (shard 1)
- GitHub Check: Gates (pre-commit all-files + parity)
- GitHub Check: Lighthouse Site
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every convention PR must ship its enforcement gate via scripts; document gate in docs/reference/convention-gates.md
Files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: No AGPL/GPL (non-LGPL) dependencies may ship; LGPL deps (psycopg/psycopg_pool/psycopg_binary) MUST be attributed in NOTICE; golangci-lint stays external binary (never in cli/go.mod); pymupdf/fitz (AGPL) excluded from all dependency tables; enforced by scripts/check_license_compat.py
Vendor-agnostic: NEVER use real vendor names in project code/tests; use example-provider, test-provider, example-{large,medium,small}-001; allowed only in .claude/, third-party imports, providers/presets.py, web/public/provider-logos/
Files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: DB > env > code default via SettingsService/ConfigResolver (Cat-1) or env > code default (Cat-2, read_only_post_init); no os.environ.get outside startup (enforce via check_no_os_environ_outside_bootstrap.py); pre-init Cat-2 reads use settings.bootstrap_resolver.resolve_init_value
Numerics live in settings/definitions/; allowlist only 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants (NAME: int|float|Final|Final[int]|Final[float] = literal); enforced by scripts/check_no_magic_numbers.py
Each ErrorCode maps to exactly one DomainError subclass so clients can branch on error_code; exemptions: inheritance alias or per-category fallbacks in gate SHAREABLE_CODES; use # lint-allow: error-code-uniqueness -- for intentional twins; enforced by scripts/check_error_code_uniqueness.py
Module-size budget tiers per # module-kind: header on first non-blank line: controller 400, service/orchestrator 600, complex_service 1100, repository 500, adapter/integration 700, feature 100, code 500 (default), tests 800, declarative exempt, generated glob-exempt; every file enforced at tier cap from first write; enforced by check_module_size_budget.py + check_no_growth_in_god_modules.py
Declarative .importlinter contracts (forbidden-only, direct-imports, blessed back-edges; NO total-order layers) enforced by lint-imports; raw-SQL boundary, DTO-leak, dependency-inversion gated by custom AST; graph-level smells (fan-in >=30, LCOM4, budget-pressure within 20% of tier cap) vs committed data/architecture_report.json via check_architecture_drift.py
Cold-import cycles (eager re-export side effects in package inits) gated by tests/unit/test_cold_import.py; keep hub init files light; put shared types in core./execution. leaves, never reach UP into heavy hub; two layering contracts: no-business-logic-upward-into-api (tools/engine/meta/security/integrations must not import api.services/api.auth/api.dto) and controllers-no-persisten...
Files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: Present every plan for accept/deny before coding (MANDATORY planning gate)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: Apply no region/currency/locale privilege; use metric units; use British English throughout
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: After issue: branch + commit + push (no auto-PR); use /pre-pr-review; after PR: /aurelio-review-pr for external feedback; fix EVERYTHING valid, no deferring (POST-IMPLEMENTATION + PRE-PR REVIEW MANDATORY)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: API startup lifecycle: construction phase wires synchronous services; on_startup (_build_lifecycle.on_startup) wires services needing connected persistence; construction-phase ordering: agent_registry BEFORE auto_wire_meetings, tunnel_provider wired unconditionally
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: On-startup ordering invariants: SettingsService auto-wire precedes WorkflowExecutionObserver registration; OntologyService wires after persistence.connect() via _wire_ontology_service; cost-dial services wire via _try_wire_cost_dial AFTER persistence; knowledge substrate wires via _wire_knowledge_engine gated on has_persistence AND has_memory_backend; EnvironmentService behind has_persistence; mid-flight steering split: INBOX from persistence.project_brain injected at boot, SERVICE wires in _wire_steering_service AFTER _wire_project_brain (memory-gated) via partial app_state.wire; red-team-report repo published during _install_runtime_services; ConversationalResumeService (NOT api/services/, in meta/chief_of_staff/) published on MetaStateSlice, UNGATED; single WorkflowExecutionService caches config_resolver and resolves max_subworkflow_depth per call for hot-reload; webhook services wire unconditionally (replay_protector) or on persistence (activity_service); idempotency keys bind reso...
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: Commits: <type>: <description> (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via synthorg-repo-bot)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: Branches: <type>/<slug> from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: Squash merge; PR body becomes squash commit; trailers (Release-As, Closes `#N`) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: GitHub queries: gh issue list via Bash, NOT MCP list_issues
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: After every squash merge → /post-merge-cleanup
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-06-20T09:23:23.452Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-05-21T22:55:20.496Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/models.py:114-114
Timestamp: 2026-05-21T22:55:20.496Z
Learning: In this repo’s “magic number” review standard, the existing gate in `scripts/check_no_magic_numbers.py` intentionally does NOT flag numeric literals used as raw call-site arguments. So, do not flag numeric literals passed as keyword arguments to Pydantic `Field()` (e.g., `Field(ge=0, le=100)` / `Field(ge=1, le=50)`)—this is an established idiom. Only treat numeric literals as “magic numbers” when they occur in the locations the gate checks (module-level assignments and function/method parameter defaults).
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-05-29T08:50:58.380Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2160
File: src/synthorg/persistence/sqlite/escalation_repo.py:370-370
Timestamp: 2026-05-29T08:50:58.380Z
Learning: In this repo, Ruff flake8-unused-arguments (ARG002) already suppresses unused-argument warnings on parameters of methods decorated with `override` (from `typing`). Therefore, if you see `# noqa: ARG002` (or equivalent) on parameters of an `override`-decorated method, treat it as stale/unused and remove it. Do not recommend re-adding `# noqa: ARG002` in these cases, because Ruff will flag the redundant directive (RUF100) and fail the Ruff CI gate.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T09:22:47.752Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2283
File: src/synthorg/engine/evolution/config.py:278-280
Timestamp: 2026-06-09T09:22:47.752Z
Learning: This repository uses ruff rule TC006 (`runtime-cast-value`), which requires the quoted string-literal form for the type argument in `cast()` calls (e.g., `cast("object", value)` rather than `cast(object, value)`). During code review, do not suggest removing the quotes from the `cast(<type>, ...)` first argument; the unquoted form will be auto-reverted by ruff on commit, so the quoted form should be treated as lint-compliant.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-05-21T22:55:09.289Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/config.py:29-30
Timestamp: 2026-05-21T22:55:09.289Z
Learning: For this repo’s Pydantic configuration idiom, do not treat numeric literals passed directly as arguments to `pydantic.Field(...)` as “magic numbers” during review. This includes call-site usages like `Field(default=0.2, ge=0.0, le=1.0)` (e.g., in config models such as `ToolAuthoringConfig`, `ToolValidationConfig`, `ToolsmithConfig`). Do not request extracting those `Field(...)` numeric arguments into named constants, since the repo’s `scripts/check_no_magic_numbers.py` intentionally excludes call-site `Field(...)` numerics and relies on `Field(...)` as the canonical way to express these constraints/defaults.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-05-31T18:00:32.445Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2180
File: src/synthorg/engine/intervention/models.py:182-203
Timestamp: 2026-05-31T18:00:32.445Z
Learning: In this repository, `NotBlankStr` is a Pydantic `Annotated[str, ...]` type alias (defined in `synthorg/core/types.py`). At runtime, calling `NotBlankStr(value)` acts like an identity/cast to `str(value)` and does not execute the `StringConstraints` or `AfterValidator(...)`. Therefore, during code review, do not treat `NotBlankStr(x)` used inside non-Pydantic model methods as a place that would raise `ValidationError`; it won’t. Similarly, when `tuple[NotBlankStr, ...]` values are involved, `NotBlankStr` erases to `str` at runtime, so membership tests/comparisons can be done with raw `str` values.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-10T12:09:37.293Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2295
File: src/synthorg/project_brain/service.py:77-80
Timestamp: 2026-06-10T12:09:37.293Z
Learning: Do not flag or recommend moving imports out of `if TYPE_CHECKING:` in `src/synthorg` when the imported class is a collaborator type that’s intentionally “concrete-faked” in tests (duck-typed stubs injected into code under constructor/typing annotations). This pattern is used to avoid runtime import side effects where runtime type enforcement (e.g., `typeguard`/`isinstance` checks against the concrete type) would cause the test fakes to be rejected. If the code relies only on annotations for those collaborators and tests provide duck-typed stubs, keep the import under `TYPE_CHECKING` rather than promoting it to a module-level runtime import.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-10T12:09:46.221Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2295
File: src/synthorg/settings/dispatcher.py:43-46
Timestamp: 2026-06-10T12:09:46.221Z
Learning: In this repository’s Python modules, do not treat `if TYPE_CHECKING:` imports as violations of any “hoist to module level” rule when they are deliberately used to support duck-typed test fakes (“concrete-faked collaborators”).
Concretely: if the code includes a comment/docstring indicating that tests drive the module using a duck-typed stub (e.g., “Concrete-faked collaborator: tests drive … with a duck-typed … stub, so a runtime import would make typeguard reject the fake”), then keep that collaborator import/type reference under `if TYPE_CHECKING:` rather than importing it at runtime. The intent is to avoid runtime typeguard validation rejecting the test fake; these guards are intentional and should not be flagged.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-03T11:43:13.104Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2200
File: tests/unit/engine/artifacts/test_service.py:42-45
Timestamp: 2026-06-03T11:43:13.104Z
Learning: For the D7 protocol method `save_returning_outcome(artifact: Artifact) -> bool` defined in `src/synthorg/persistence/artifact_protocol.py`, any implementation—including fake/stub/test doubles—must use the exact same parameter name `artifact` (i.e., `save_returning_outcome(self, artifact=...)` / `save_returning_outcome(self, artifact: Artifact)`), not `entity`. This name must match for typeguard positional-or-keyword name conformance. Do not suggest renaming the protocol method’s parameter to `entity`.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T10:06:53.040Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2283
File: src/synthorg/engine/middleware/coordination_constraints.py:152-152
Timestamp: 2026-06-09T10:06:53.040Z
Learning: Use International/British English spellings throughout the repository (code comments, docstrings, and documentation). British spellings such as "Analyse" (not "Analyze"), "Behaviour" (not "Behavior"), and "Colour" (not "Color") are mandatory and should not be flagged as spelling errors or inconsistencies during code review. This repo’s style is enforced via `vale` (see `CLAUDE.md` → "Regional Defaults (MANDATORY)"). If an American spelling appears, prefer the corresponding British spelling.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-13T08:51:11.124Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2348
File: docs/guides/custom-mcp-server-dev.md:155-161
Timestamp: 2026-06-13T08:51:11.124Z
Learning: When reviewing Aureliolo/synthorg usage of `mcp_descriptor()` (from `src/synthorg/meta/mcp/feature_descriptors.py`), ensure all call sites pass the keyword argument `handlers=...` (type `Callable[[], Mapping[str, object]]`). `mcp_descriptor()` maps this `handlers` value internally to the descriptor’s `handlers_factory` field; `handlers_factory=...` is not a valid keyword and will raise `TypeError: unexpected keyword argument`. Also ensure documentation and code examples do not suggest renaming `handlers` to `handlers_factory`; always show `handlers=`.
Applied to files:
src/synthorg/backup/models.pysrc/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T17:05:23.619Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/quality.py:44-45
Timestamp: 2026-06-09T17:05:23.619Z
Learning: In the meta MCP handler modules under `src/synthorg/meta/mcp/handlers/`, `AppState` imported from `synthorg.api.state` is intentionally done only inside `if TYPE_CHECKING:` (not at module import time). This avoids a meta→api runtime dependency/back-edge that would violate the project’s package layering. During code review, do NOT flag the `AppState` `TYPE_CHECKING` guard in these meta handlers as a layering or import-structure violation; mypy should validate the type usage statically, and runtime behavior intentionally omits the import.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T17:05:38.738Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/workflow_executions.py:55-57
Timestamp: 2026-06-09T17:05:38.738Z
Learning: In MCP meta handler modules under `src/synthorg/meta/mcp/handlers/`, do not flag `from synthorg.api.state import AppState` as a violation if it is intentionally placed at module scope inside an `if TYPE_CHECKING:` block. This is an explicit layering exception: importing `AppState` at runtime would create a meta→api back-edge, but keeping it under `TYPE_CHECKING` preserves static checking (e.g., mypy) without introducing the runtime dependency (e.g., typeguard behavior).
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-09T17:07:02.613Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2287
File: src/synthorg/meta/mcp/handlers/charter.py:37-38
Timestamp: 2026-06-09T17:07:02.613Z
Learning: In files under `src/synthorg/meta/mcp/handlers/`, `AppState` imported from `synthorg.api.state` should remain inside `if TYPE_CHECKING:` (i.e., not hoisted to module scope). This is intentional to avoid runtime imports/Typeguard enforcement (which would break handler unit tests that inject a lightweight `SimpleNamespace`) and to prevent a runtime meta→api back-edge that this package’s layering avoids. `AppState` annotations will still be validated statically by mypy; typeguard is expected to skip the TYPE_CHECKING-only import here. Do not flag `AppState` under `TYPE_CHECKING` as a violation of “type-only imports at module level” for this handler package.
Applied to files:
src/synthorg/meta/mcp/handlers/infrastructure/backup.py
📚 Learning: 2026-06-11T17:01:48.351Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2321
File: .github/actions/start-postgres/action.yml:100-106
Timestamp: 2026-06-11T17:01:48.351Z
Learning: When using Docker CLI v29.5.3+ with `docker tag`, a digest-qualified reference is valid as the SOURCE but not as the TARGET. Specifically, `docker tag SOURCEsha256:<digest> TARGET:tag` should work, while the error like “refusing to create a tag with a digest reference” applies when the digest reference is used as the TARGET (e.g., `docker tag src imagesha256:<digest>`). A digest-qualified source resolves to a locally available image (typically one you’ve pulled) before tagging it with `TARGET:tag`.
Applied to files:
data/runtime_stats.yaml
🔇 Additional comments (4)
data/runtime_stats.yaml (1)
2-3: LGTM!src/synthorg/backup/models.py (1)
194-205: LGTM!src/synthorg/meta/mcp/handlers/infrastructure/backup.py (1)
8-11: LGTM!Also applies to: 252-297
data/feature_index.json (1)
56-69: LGTM!Also applies to: 710-725, 1285-1285
The round-15 cached-payload validation discarded the validated model and returned the raw dict; capture the RestoreConfirmation instance and return its model_dump() so Pydantic coercion/normalisation is preserved in the response.
Aureliolo
temporarily deployed
to
cloudflare-preview
June 20, 2026 09:41 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
June 20, 2026 09:55 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2409
Finishes the deferred remainder of #2402 across four audit areas.
Gate 15 / slot 133 — idempotency + retry safety
approvalsapprove/reject,backuprestore, MCPbackup.restore) now require anIdempotency-Keyheader and run under the existingIdempotencyService; a retried request returns the cached result rather than double-firing side effects. The dedup key binds the resource id (f"{resource_id}:{idempotency_key}") so a reused token cannot collide across resources.SeenClaimsRepository(when DB-backed) so NATS redeliveries dedup; NATS-only/placeholder runs stay DB-free.approvals.ts,backup.ts) sends the header via a sharedidempotencyKeyHeaderhelper.Gate 16 / slot 127 — lifecycle-lock discipline (7 services)
SharedRateLimitCoordinator,OAuthTokenManager,WebhookEventBridge,QuotaPoller,NotificationDispatcher, the NATS bus connection, and the backup scheduler now follow theProviderHealthProberpattern:_stop_failedflag, shielded drain wrapped inasyncio.wait_for, andstart()refusing once unrestartable. Four new*UnrestartableError(ConflictError)classes inherit the shareableRESOURCE_CONFLICTcode.Gate 6 / slot 68 — controllers through services
New
ConversationalResumeService(ungated, inmeta/to dodge the cold-import cycle),AnalyticsReadService, a startup-wired singletonWorkflowExecutionService(caches theconfig_resolver, resolvesmax_subworkflow_depthper call), and_wire_webhook_request_servicesremove the remaining direct repository touches and the webhook double-checked-lock factories.Gate 16 / slot 102 remainder — bounded shutdown/failure drains
api/state.pynow drains the objective/brownfield background-task sets on shutdown, and_cleanup_on_failurecovers the event-stream hub, escalation subscriber/sweeper, and provider health prober on the startup-failure path.Pre-PR review
Reviewed by 16 agents + a ghost-wiring pass; all valid findings addressed, including the idempotency state-machine split, the webhook poll-loop clock/race fix, worker logging cleanup, and added coverage (idempotency key-binding + missing-header + timed-out→409, depth-resolver fallback, ttl guard, resume-service delegation, cleanup-on-failure, and the startup wiring helpers). Generated data regenerated; full local gate suite, mypy strict, ruff, and the web lint/type-check/test suite all green.