refactor(wp1): persistence + layer discipline + restart safety#1945
Merged
Conversation
…ity_dtos Closes layer-violation flagged in #1916: persistence layer should not import from the api layer. ProviderAuditEvent / ProviderAuditActor / PresetOverride are re-exported by synthorg.api.dto_provider_capabilities from synthorg.providers.management.capability_dtos; import directly from the source-of-truth module instead. Files: provider_audit_protocol.py, preset_override_protocol.py, and the sqlite + postgres impls for each.
…dual-backend impls Per WP-1 (#1916): persist the ceremony scheduler's per-sprint state so a restart mid-sprint reconstructs counters, fired-once triggers, total completions, and velocity history. Single snapshot row per sprint_id keyed by sprint_id. JSON-blob columns hold the dict, set, and tuple fields; total_completions is plain INTEGER. Both backends use TEXT for the JSON columns (table is one row per sprint; JSONB indexing offers no benefit). New yoyo revisions 20260515000001 for both backends. New events PERSISTENCE_CEREMONY_STATE_{SAVED,SAVE_FAILED,LOADED,LOAD_FAILED,DELETED,DELETE_FAILED}. Repository wiring into PersistenceBackend protocol and scheduler integration come in the next commit.
…ckend impls Per WP-1 (#1916): persist the meeting scheduler's per-meeting-type last-triggered timestamp so the recurring-meeting cooldown survives process restart. One row per meeting type, keyed by meeting_type_name, with a single last_triggered_at column. The scheduler hydrates the dict at start via load_all() and upserts after every trigger. Migration combined with the WP-1 ceremony_scheduler_state table in revision 20260515000001 to honour the one-migration-per-PR gate.
Per WP-1 (#1916): persist the Docker sandbox lifecycle's _tracked_containers dict so a process restart can reconcile against the Docker daemon's label-filtered container list. One row per managed sandbox keyed by container_id with an optional sidecar_id column. Also closes a schema.sql / migration drift bug for the WP-1 tables: the ceremony_scheduler_state and meeting_cooldown CREATE TABLE statements landed in the migration but not in schema.sql in prior commits. This commit adds all 3 WP-1 tables to both schema.sql files matching the existing backend-native style, and appends 3 standard TEXT-vs-TIMESTAMPTZ entries to scripts/schema_drift_baseline.txt.
Per WP-1 (#1916): structurally prevent the schema.sql / migration drift bug-class introduced earlier in this PR. The check_schema_drift_revisions.py script already exists; it applies all yoyo revisions to a fresh SQLite DB and diffs the resulting schema against the declared schema.sql. Previously it ran manually only -- the drift my earlier commits introduced was caught by accident during PR development. This commit wires the sqlite arm into pre-push (~5-10s, no external dependencies). The postgres arm needs Docker and continues to run in CI only. Combined with the existing cross-backend schema-drift gate, this makes 'forgot to update schema.sql' impossible to push undetected.
…ftest.py Per WP-1 (#1916): the conftest carried _OP_LOG_DDL and _SNAPSHOT_DDL string constants describing the org_facts_operation_log and org_facts_snapshot tables. Grep shows zero references to either constant anywhere in the test suite -- they were dead code, presumably left over from an earlier fixture iteration. 73 unit tests under tests/unit/memory/org/ continue to pass without the constants; the tests work through the persistence layer's normal repository protocol, not raw DDL.
…ocol + both backends
Per WP-1 (#1916): the WP-1 reconciliation pass (synthorg.tools.sandbox.reconciliation) filters Docker containers by the synthorg.managed=true label so it can identify orphan containers (label present, no DB row) without misidentifying unrelated containers on a shared daemon. Every new sandbox container now carries the label alongside the existing synthorg.sandbox=true label, so the reconciliation pass at sandbox-subsystem start can fold them into its filtered listing.
…ackend Per Track #24 PersistenceBackend wiring (already landed): the protocol gained ceremony_scheduler_state, meeting_cooldown, and tracked_containers properties. The _FakeBackend test stub in test_protocol.py was a pre-existing structural-compliance fake; it needs matching stub properties or isinstance(_FakeBackend(), PersistenceBackend) fails. Three trivial @Property stubs added so the structural-compliance test continues to pass. No production code touched.
Address findings from 10 pre-PR review agents covering docs drift, concurrency hygiene, schema clarity, idempotency-callback safety, and the new layer-discipline gate. - Register check_no_api_dto_in_persistence_or_service.py in convention-gates.md, convention_gate_map.yaml, and runtime_stats.yaml. - Extend docs/design/persistence.md with the three new WP-1 repos and a row linking to ADR-0001 (six generic categories). - Refresh persistence-boundary.md Enforcement section. - Update root CLAUDE.md persistence-boundary one-liner to cite the ADR + generic categories. - _nats_state.py: stop_drain_timeout_seconds docstring no longer claims a non-existent NatsConfig override. - _generics.py docstrings document transition_if updates contract and the two query order invariants. - FakePersistenceBackend: PLR0915 noqa with rationale plus stubs for meeting_cooldown, ceremony_scheduler_state, tracked_container, and idempotency. - schema.sql comments document index strategy and the intentional SQLite TEXT vs Postgres TIMESTAMPTZ divergence. - InMemoryBackend.clear acquires _store_lock (signature flips async); tests updated for the await. - Webhook retry callback snapshots payload via dict(payload). - MeetingScheduler._hydrate_cooldowns_from_repo holds _cooldown_lock; stop() parks the drain task at self._background_drain_task for observability. - memory.py finally-block discard carries a single-threaded safety justification. - claim.py lifecycle-lock lint-allow comment kept short to satisfy E501; expanded rationale lives in the surrounding comment. - Caddyfile legacy-doc redirects gain rename rationale. - _lifecycle_lock_for_current_loop docstring enumerates the three branches so the cross-loop rebind is no longer implicit. - test_webhooks_retry adds _PassThroughIdempotencyService. - reconciliation.py loses a now-dead type: ignore.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Contributor
|
✅ Actions performedFull review triggered. |
…nce-layer-discipline
![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: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/synthorg/persistence/sqlite/checkpoint_repo.py (1)
61-82: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSerialise
created_atwithformat_iso_utc().
append()currently letsmodel_dump(mode="json")choose the wire format forcreated_at, butpurge_before()compares againstformat_iso_utc(threshold)in SQL. Using two serializers for the same TEXT column can make the cutoff comparison drift and breaks the persistence datetime contract.♻️ Proposed fix
- data = checkpoint.model_dump(mode="json") + data = checkpoint.model_dump(mode="json") + data["created_at"] = format_iso_utc(checkpoint.created_at)As per coding guidelines, "Datetime in persistence: use
parse_iso_utc/format_iso_utcfrompersistence._shared(reject naive); usenormalize_utcfor already-typed".🤖 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/persistence/sqlite/checkpoint_repo.py` around lines 61 - 82, The append method currently uses checkpoint.model_dump(mode="json") which may serialize created_at differently than purge_before's format_iso_utc threshold comparison; update append (in function append of Checkpoint repository) to serialize checkpoint.created_at explicitly using format_iso_utc from persistence._shared (or normalize_utc if the field is already a datetime) and inject that value into the data dict as the context_json/created_at wire value before executing the INSERT, ensuring created_at is always stored in the same ISO-UTC TEXT format that purge_before expects.src/synthorg/persistence/postgres/preset_override_repo.py (1)
38-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
get()row-decoding failures inQueryError.
get()returnsself._row_to_override(row)directly, so malformed rows now leakValidationError/ValueError/TypeError/KeyErrorinstead of the repository’sQueryErrorcontract.list_items()already normalises those failures;get()should do the same for parity.♻️ Suggested fix
if row is None: return None - return self._row_to_override(row) + try: + return self._row_to_override(row) + except (ValidationError, ValueError, TypeError, KeyError) as exc: + msg = "Failed to read preset override" + logger.warning( + PERSISTENCE_PRESET_OVERRIDE_QUERY_FAILED, + error_type=type(exc).__name__, + error=safe_error_description(exc), + preset_name=preset_name, + ) + raise QueryError(msg) from exc🤖 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/persistence/postgres/preset_override_repo.py` around lines 38 - 63, get() currently calls self._row_to_override(row) directly so any decoding errors (ValidationError/ValueError/TypeError/KeyError) propagate instead of being translated to QueryError; wrap the row-to-domain conversion in the same try/except handling used by list_items(): call self._row_to_override(row) inside a try block and catch ValidationError, ValueError, TypeError, KeyError (or a tuple of these) and re-raise a QueryError with an appropriate message (and keep the original exception as the cause) while preserving the existing PERSISTENCE_PRESET_OVERRIDE_QUERY_FAILED logging pattern used in the top-level except; update get() to return the converted PresetOverride on success or None if row is None.src/synthorg/persistence/postgres/preset_repo.py (1)
91-117:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWrap
config_jsoninJsonb()and validate JSON before binding to the UPSERT.The Postgres
custom_presets.config_jsoncolumn is declared as JSONB. Passing the protocol's JSON source string directly to the SQL parameters skips psycopg's necessary adaptation step. Throughout the persistence layer—parked_context_repo.py,checkpoint_repo.py,escalation_repo.py—JSON strings bound to JSONB columns are consistently parsed viajson.loads()and wrapped inJsonb(). Without this, the save operation fails at runtime.♻️ Suggested fix
+from psycopg.types.json import Jsonb @@ await cur.execute( @@ ( entity.name, + Jsonb(json.loads(entity.config_json)), - entity.config_json, entity.description, entity.created_at, entity.updated_at, ), ) await conn.commit() + except json.JSONDecodeError as exc: + msg = f"Invalid JSON in custom preset {entity.name!r}" + logger.warning( + PRESET_CUSTOM_SAVE_FAILED, + preset_name=entity.name, + error_type=type(exc).__name__, + error=safe_error_description(exc), + ) + raise QueryError(msg) from exc except psycopg.Error as exc:🤖 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/persistence/postgres/preset_repo.py` around lines 91 - 117, In the save method, the config_json value is being bound directly to the JSONB column—change Preset.save to parse/validate the JSON string with json.loads() and wrap the resulting Python object with psycopg Jsonb() before passing it as the parameter for config_json in the UPSERT; update the parameters tuple used in save (and mirror the same pattern used in parked_context_repo.py, checkpoint_repo.py, escalation_repo.py) so psycopg can adapt the value correctly and raise a QueryError if json.loads() fails.src/synthorg/persistence/postgres/session_repo.py (1)
90-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the new Postgres operations in the repository’s normal error boundary.
save(),get(),list_items(),query(),count(), anddelete()currently let driver/pool failures bubble out unlogged. That makes this repository inconsistent with the rest of the persistence layer and drops the structured failure event on the floor. Please catch the database exception in each of these methods, logAPI_AUTH_SESSION_PERSISTENCE_ERRORwith context, and re-raise the domain persistence error you use elsewhere in Postgres repositories.As per coding guidelines, "Error paths log WARNING/ERROR with context before raising".
Also applies to: 130-143, 145-219, 277-290
🤖 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/persistence/postgres/session_repo.py` around lines 90 - 129, Wrap all Postgres driver/pool calls in the session repository (methods save, get, list_items, query, count, delete) in a try/except that catches the DB-related exception(s), logs the structured event API_AUTH_SESSION_PERSISTENCE_ERROR with contextual fields (e.g., session_id, query params, exception info) via the repository logger, and then re-raise the repository’s standard domain persistence error used by other Postgres repos (the same exception class raised elsewhere in this package) so callers get the consistent error type; apply this pattern inside save(), get(), list_items(), query(), count(), and delete() (and the surrounding connection/cursor blocks) so all driver failures are logged and translated before propagating.src/synthorg/persistence/postgres/fine_tune_repo.py (1)
656-683:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLock the checkpoint row before deciding whether it is deletable.
This is still a read-then-delete race. Another transaction can delete or activate the same row between the
SELECT is_activeand theDELETE ... AND is_active = FALSE, socur.rowcount == 0does not reliably mean “became active during delete”. In that case this path can raiseQueryErroreven though the checkpoint simply disappeared concurrently. Acquire the rowFOR UPDATEhere, or collapse the check into a singleDELETE ... RETURNING, so the decision is atomic.🔒 Minimal locking fix
await cur.execute( - "SELECT is_active FROM fine_tune_checkpoints WHERE id = %s", + "SELECT is_active FROM fine_tune_checkpoints " + "WHERE id = %s FOR UPDATE", (checkpoint_id,), )🤖 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/persistence/postgres/fine_tune_repo.py` around lines 656 - 683, Replace the separate SELECT + DELETE race with an atomic operation: issue a single DELETE ... RETURNING to delete the checkpoint only if is_active = FALSE (e.g. "DELETE FROM fine_tune_checkpoints WHERE id = %s AND is_active = FALSE RETURNING id") via the same cur.execute/cur.fetchone flow that currently uses checkpoint_id; if RETURNING yields a row, deletion succeeded and return True, if it yields no row then query the checkpoint existence (SELECT is_active FROM fine_tune_checkpoints WHERE id = %s) to distinguish “became active” (raise QueryError using MEMORY_FINE_TUNE_PERSIST_FAILED and the same messages) versus “not found” (return False). This removes the read-then-delete race around cur.rowcount while keeping the same error handling and using the existing symbols checkpoint_id, MEMORY_FINE_TUNE_PERSIST_FAILED, QueryError, cur.execute, cur.fetchone.src/synthorg/persistence/sqlite/principle_override_repo.py (1)
130-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing pagination validation in
list_items.Unlike sibling repositories in this PR (e.g.,
SQLiteTaskRepository.list_items,SQLiteCostRecordRepository.query), this method passes rawlimit/offsetdirectly to SQLite without validation. In SQLite,LIMIT -1removes the cap entirely, allowing unbounded scans.🛡️ Proposed fix
+from synthorg.persistence._shared import validate_pagination_args + async def list_items( self, *, limit: int = DEFAULT_PAGE_SIZE, offset: int = 0, ) -> tuple[PrincipleOverride, ...]: """List all overrides ordered by ``scope`` ascending.""" + limit = validate_pagination_args( + limit, offset, event=PERSISTENCE_PRINCIPLE_OVERRIDE_LIST_FAILED + ) try: cursor = await self._db.execute(🤖 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/persistence/sqlite/principle_override_repo.py` around lines 130 - 145, The list_items method in PrincipleOverrideRepo passes limit/offset directly to SQLite (in the SQL executed in list_items) without validation, allowing values like LIMIT -1 to produce unbounded scans; add the same validation/clamping used in sibling repos (e.g., ensure limit is an int within 1..DEFAULT_PAGE_SIZE and offset is non-negative) before calling self._db.execute, coerce types with int(), clamp/raise for out-of-range values, and use the validated variables in the SELECT query so the SQL never receives a negative or overly large limit/offset.src/synthorg/persistence/sqlite/connection_repo.py (1)
233-267: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winInconsistent pagination validation pattern.
list_itemsandqueryuseif limit <= 0: return ()andmax(0, int(offset))to silently handle invalid inputs, whereas sibling repositories in this PR usevalidate_pagination_args(limit, offset, event=...)which raises on invalid values. This inconsistency can mask caller bugs and makes behaviour unpredictable across the codebase.♻️ Proposed fix for list_items
+from synthorg.persistence._shared.pagination import validate_pagination_args + async def list_items( self, *, limit: int = DEFAULT_PAGE_SIZE, offset: int = 0, ) -> tuple[Connection, ...]: """List all connections, sorted by name for determinism.""" - if limit is not None and limit <= 0: - return () + limit = validate_pagination_args( + limit, offset, event=PERSISTENCE_CONNECTION_LIST_FAILED + ) sql = f"SELECT {_SELECT_COLS} FROM connections ORDER BY name ASC" # noqa: S608 - params: tuple[object, ...] = () - if limit is not None: - sql += " LIMIT ? OFFSET ?" - params = (int(limit), max(0, int(offset))) + sql += " LIMIT ? OFFSET ?" + params = (limit, offset)Apply a similar change to
query.Also applies to: 269-308
🤖 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/persistence/sqlite/connection_repo.py` around lines 233 - 267, Replace the current permissive pagination handling in list_items (and similarly in query) with the shared validator: call validate_pagination_args(limit, offset, event=PERSISTENCE_CONNECTION_LIST_FAILED) at the start of each function to validate/normalize inputs (and capture returned normalized limit/offset if the helper returns them), remove the manual if limit <= 0/ max(0, int(offset)) logic, and keep the rest of the SQL/exception handling identical; ensure you import validate_pagination_args and use the same event constant for the query function (use the matching PERSISTENCE_CONNECTION_QUERY_FAILED event constant there) so invalid pagination now raises a validation error consistently across repositories.src/synthorg/persistence/sqlite/preset_repo.py (1)
137-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing pagination validation in
list_items.This method passes raw
limit/offsetdirectly to SQLite without validation, unlike sibling repositories that usevalidate_pagination_args. Negative values could cause unexpected behaviour (e.g.,LIMIT -1removes the cap).🛡️ Proposed fix
+from synthorg.persistence._shared.pagination import validate_pagination_args + async def list_items( self, *, limit: int = DEFAULT_PAGE_SIZE, offset: int = 0, ) -> tuple[Preset, ...]: """List custom presets ordered by name. ... """ + limit = validate_pagination_args( + limit, offset, event=PRESET_CUSTOM_LIST_FAILED + ) try: async with self._db.execute(🤖 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/persistence/sqlite/preset_repo.py` around lines 137 - 178, The list_items method accepts limit and offset and passes them directly into the SQL query, allowing negative values (e.g., LIMIT -1) to change behavior; call the existing validate_pagination_args(limit, offset) at the start of PresetRepo.list_items (or otherwise sanitize/normalize limit and offset) to ensure non-negative, bounded pagination parameters before using them in the execute call, and use the validated values in the SQL parameter tuple; keep the existing error handling (QueryError) and logging (PRESET_CUSTOM_LIST_FAILED, PRESET_CUSTOM_LISTED) intact.
🤖 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 `@scripts/check_no_api_dto_in_persistence_or_service.py`:
- Around line 83-113: The current _iter_import_violations only catches absolute
imports; update the ast.ImportFrom handling in _iter_import_violations to also
consider relative imports by checking node.level > 0 when deciding if the import
targets DTOs: treat a node as forbidden if (module or
"").startswith(_DTO_MODULE_PREFIX) or (node.level > 0 and (module or
"").startswith(_DTO_MODULE_PREFIX)), and likewise treat imports from
_API_PACKAGE or relative imports into api by checking node.level > 0 in the
branch that inspects alias.name.startswith(_DTO_NAME_PREFIX); keep the existing
ast.Import logic unchanged. Use the existing symbols _iter_import_violations,
ast.ImportFrom, node.level, _DTO_MODULE_PREFIX, _API_PACKAGE, and
_DTO_NAME_PREFIX to locate and modify the checks.
In `@scripts/check_no_git_no_verify.sh`:
- Around line 51-62: The SHORT_NO_VERIFY_RE currently treats "-n" after both
commit and push as a no-verify bypass, which incorrectly blocks "git push -n"
(dry-run); update the SHORT_NO_VERIFY_RE so it only matches the commit
subcommand (remove "push" from the alternation), and ensure the grep check that
uses "$SHORT_NO_VERIFY_RE" (the echo "$COMMAND" | grep -qiE
"$SHORT_NO_VERIFY_RE" branch alongside "$BYPASS_RE") continues to run unchanged;
update the surrounding comment to reflect that "-n" is matched only for commit.
In `@src/synthorg/api/lifecycle_helpers/bootstrap.py`:
- Line 35: The call to app_state.persistence.users.list_items() uses the default
paginated page size and may miss existing owners beyond the first page, causing
incorrect promotion of users[0]; update bootstrap logic in bootstrap.py to
reliably detect an existing owner by either iterating through all pages returned
by list_items() (using pagination parameters/next tokens) or by calling a
dedicated owner-existence method (e.g., add/use a users.has_owner(owner_role) or
users.find_one(filter={"role":"owner"})) so the has_owner check is exhaustive
and idempotent (ensure references to list_items, DEFAULT_PAGE_SIZE,
app_state.persistence.users, and the has_owner/owner-detection code are updated
accordingly).
In `@src/synthorg/communication/bus/_nats_connection.py`:
- Around line 212-265: The stop routine currently swallows
asyncio.CancelledError in both the unsubscribe loop (around await
sub.unsubscribe()) and the drain call (around await
asyncio.wait_for(state.client.drain()...)), causing cancelled shutdowns to
appear successful; change those except asyncio.CancelledError: pass handlers to
re-raise the cancellation (e.g., except asyncio.CancelledError: raise) or remove
the except block so the CancelledError propagates, ensuring the function does
not proceed to clear state.subscriptions, reset state.client/js/kv, or log
COMM_BUS_STOPPED when cancellation occurs.
In `@src/synthorg/communication/bus/_nats_state.py`:
- Around line 64-69: The dataclass field stop_drain_timeout_seconds currently
embeds the literal 30.0; extract that literal to a module-level annotated
constant (e.g. DEFAULT_STOP_DRAIN_TIMEOUT_SECONDS: float = 30.0) and update the
dataclass field default to reference that constant (stop_drain_timeout_seconds:
float = DEFAULT_STOP_DRAIN_TIMEOUT_SECONDS) so the numeric is named and
reusable; ensure the new constant is top-level in _nats_state.py and properly
annotated and documented per module conventions.
In `@src/synthorg/communication/meeting/scheduler.py`:
- Around line 258-264: The _persist_cooldown function currently catches
repository/storage exceptions, logs a warning and swallows the error; change it
to surface persistence failures instead: remove the try/except that suppresses
exceptions (or re-raise after logging) so repository errors propagate to
callers, ensuring durable cooldown writes are not lost; apply the same change to
the analogous persistence block around lines 278-290 (the other cooldown upsert
helper) and keep logging but do not suppress—let the exception bubble to
trigger_event() so the trigger flow can decide to abort or retry.
- Around line 247-256: Before writing persisted records back into the in-memory
cooldown map, clear the existing cooldown cache so stale keys don't persist
across restarts: inside the async with self._cooldown_lock_for_current_loop()
block, reset/clear self._last_triggered (e.g., assign an empty dict or call
clear()) before iterating over records to repopulate it, and then set
self._cooldown_hydrated = True as before; this ensures the hydration loop that
writes to self._last_triggered replaces any prior entries rather than overlaying
them.
In `@src/synthorg/engine/workflow/ceremony_scheduler.py`:
- Around line 273-279: Comments around the call to await
self._hydrate_state_from_repo(sprint.id) contain backlog/migration framing
(e.g., "WP-1 restart-safety") which violates the comment guideline; replace
those lines with a concise WHY-only comment that describes the operational
purpose (e.g., "Hydrate persisted sprint state so trigger counters resume after
a process restart") and remove any reviewer/issue prefixes, and apply the same
change to the other instance referenced (lines 453-455) where similar framing
appears; keep the reference to the behavior and affected attributes but do not
include migration IDs or reviewer citations.
- Around line 273-279: The hydration step in _hydrate_state_from_repo currently
assumes the persisted row is complete and replaces the in-memory seeded state
(self._completion_counters) with the persisted snapshot after json.loads(),
which can raise VelocityRecord.model_validate() errors or drop ceremony keys;
update _hydrate_state_from_repo to catch validation/deserialization exceptions
(e.g., from VelocityRecord.model_validate) and on any failure keep the existing
zeroed state, and when validation succeeds merge the persisted counters into the
existing seeded counters (overwrite counts only for keys present in the
persisted data) instead of assigning dict(counters); apply the same
tolerant-merge pattern to the analogous logic later in the file (the second
hydration block referenced around lines 333-354) so activate_sprint() never
aborts or produces KeyError due to partial/stale persisted rows.
In `@src/synthorg/persistence/__init__.py`:
- Around line 64-65: The package re-exports ProjectRepository but not the
ProjectFilterSpec required by ProjectRepository.query(); update
synthorg.persistence.__init__ to also import and export ProjectFilterSpec from
synthorg.persistence.project_protocol (alongside ProjectRepository) so callers
can "from synthorg.persistence import ProjectRepository, ProjectFilterSpec";
apply the same addition to the other export block referenced (the similar export
around the later lines exporting other filter specs) to keep the public surface
consistent.
In `@src/synthorg/persistence/_generics.py`:
- Around line 1-5: Edit the top-level docstring in module _generics (the module
docstring at the top of src/synthorg/persistence/_generics.py) to remove any
WP-1/refactor rollout history and reviewer/migration framing; replace it with a
concise description of the current generic repository protocol categories and
their invariants (what each protocol represents and how they compose), keeping
only technical contract details and no migration or issue references.
In `@src/synthorg/persistence/fine_tune_protocol.py`:
- Around line 26-33: Docstrings in fine_tune_protocol.py include ADR/D7 and
migration/review framing; remove those labels and rephrase the class and method
docstrings to state only the runtime contract and behavior. Edit the top-level
protocol docstring (and the docstrings for get_active_run, update_run,
mark_interrupted and any other noted blocks) to drop "ADR-0001", "D7",
"review/migration framing" language and replace with concise descriptions of the
interface: what each method does, inputs/outputs, and the domain invariant
(e.g., get_active_run filters in-progress runs; update_run performs bulk-field
updates without full serialization; mark_interrupted atomically marks active
runs FAILED). Keep content focused on runtime behavior only and remove reviewer
citations or migration history.
In `@src/synthorg/persistence/integration_stubs.py`:
- Around line 175-204: The in-memory save method currently appends blindly to
self._store causing duplicate entries; change save to be idempotent by detecting
an existing receipt with the same id (compare via str(existing.id) ==
str(receipt.id)) and replacing that entry (with copy.deepcopy(receipt)) instead
of appending; if no existing id is found, append the deep-copied receipt as
before. Ensure this behavior keeps get, delete, and list_items semantics intact
(they already compare ids using str(r.id)).
In `@src/synthorg/persistence/meeting_cooldown_protocol.py`:
- Around line 5-8: Update the module docstring in meeting_cooldown_protocol.py
to remove the migration-framing phrase "Pre-WP-1 this lived in..." and rewrite
it in present tense as a behavioral rationale: explain that the module stores
the last-triggered wall-clock timestamp per meeting type (previously stored in a
transient dict) and why persistent cooldown tracking is needed to prevent
recurring meetings from firing immediately after a restart; edit the top-of-file
docstring accordingly so it explains the current behavior and purpose without
historical/migration references.
In `@src/synthorg/persistence/postgres/agent_state_repo.py`:
- Around line 130-148: In list_items, normalize pagination args before the DB
call: clamp offset to >=0 (e.g., offset = max(0, offset)), clamp limit to the
allowed range (e.g., limit = min(max(0, limit), DEFAULT_PAGE_SIZE)), and
short-circuit when limit == 0 by returning an empty tuple; then use these
normalized limit and offset values in the SQL execute call so negative inputs
cannot reach the backend. Ensure you update the logic inside the list_items
method and keep the return type tuple[AgentRuntimeState, ...].
In `@src/synthorg/persistence/postgres/connection_repo.py`:
- Around line 214-227: Change list_items and query to reject invalid pagination
arguments rather than normalising them: remove the clamping behavior (the max(0,
int(offset)) and returning empty tuple for limit<=0) and call the shared
validate_pagination_args(limit, offset) used by other repos; on invalid args
ensure you raise the same QueryError used elsewhere so callers get consistent
backend behavior. Update references in the ConnectionRepo methods list_items and
query (and any helper that uses DEFAULT_PAGE_SIZE) to validate before building
the SQL params and then pass the validated limit and offset through to the SQL
query.
In `@src/synthorg/persistence/postgres/parked_context_repo.py`:
- Line 26: list_items currently skips the shared pagination validation and can
pass negative/zero page or page_size to Postgres; before building the query in
list_items, call the shared pagination validator from
synthorg.persistence._generics (validate the inputs from the caller, ensuring
page and page_size are positive and applying DEFAULT_PAGE_SIZE as a fallback)
and use the validated values when computing LIMIT/OFFSET; apply the same change
to the other paging code block around the second listing implementation (the
block noted in the comment) so both flows use the shared validator and avoid
DB-specific behavior.
In `@src/synthorg/persistence/postgres/provider_audit_repo.py`:
- Around line 182-226: In query(), validate the effective_offset before
executing SQL: after computing effective_offset (the variable set to offset or 0
when filter_spec.after_id is present), call the shared pagination validator used
elsewhere in the codebase (the same function you use to validate limit/offset in
other repos) to validate that effective_offset is non-negative (and that limit
is valid), and raise a QueryError if validation fails; update the validation to
run alongside the existing limit check in the query() method so callers get a
repository-level error instead of a DB error when a negative offset is provided.
In `@src/synthorg/persistence/postgres/settings_repo.py`:
- Around line 196-205: The list_items method currently hardcodes 1_000 as the
ceiling; replace that with the shared pagination validation/ceiling from
settings/definitions and the repo-wide pagination helper instead of an inline
literal. In practice, call the common pagination validator (the existing helper
used by other repo methods) to compute and validate effective_limit/offset
(using the existing DEFAULT_PAGE_SIZE symbol and the module-level page-ceiling
constant in settings/definitions) inside list_items so no magic number remains
and behavior matches other paginated repository methods.
In `@src/synthorg/persistence/postgres/webhook_receipt_repo.py`:
- Around line 265-307: In list_items, remove the redundant "if limit is not
None:" branch (limit is typed and defaulted, and there's already an early return
for limit <= 0); always append the " LIMIT %s OFFSET %s" clause to the base SQL
and set params = (int(limit), max(0, int(offset))) unconditionally so the
SQL/params construction is simplified; keep the existing error handling and row
deserialization logic (_row_to_receipt) unchanged.
In `@src/synthorg/persistence/postgres/workflow_definition_repo.py`:
- Around line 463-467: The ORDER BY in the SQL built in workflow_definition_repo
(the sql variable that uses _SELECT_COLUMNS and where_clause) must include a
unique secondary key to make pagination deterministic; change the ORDER BY
clause from "ORDER BY updated_at DESC" to "ORDER BY updated_at DESC, id DESC"
(or id ASC if you prefer stable ascending tie-break) so rows with identical
updated_at are consistently ordered before applying LIMIT %s OFFSET %s.
In `@src/synthorg/persistence/sqlite/agent_state_repo.py`:
- Around line 122-133: The query uses raw limit/offset values which can be
negative or too large; before calling self._db.execute in the method that
accepts limit and offset, validate and sanitize those parameters (coerce to int,
ensure offset >= 0 and limit > 0 and <= DEFAULT_PAGE_SIZE or a defined
MAX_PAGE_SIZE), and either clamp out-of-range values or raise a ValueError;
update the code paths that call this method to handle the error and add tests
for negative/oversized limit/offset to prevent unbounded reads against the
agent_states query.
In `@src/synthorg/persistence/sqlite/audit_repository.py`:
- Around line 124-167: The query method allows a negative offset because
_validate_query_args only checks limit and dates; update validation to reject
negative offsets by either extending _validate_query_args to accept an offset
parameter or adding an explicit check in query (before building the SQL) that
raises QueryError when offset < 0; reference the query method, AuditFilterSpec
usage, and _validate_query_args to locate where to add the check and ensure the
error path matches other QueryError raises for consistency.
In `@src/synthorg/persistence/sqlite/backend.py`:
- Around line 832-841: The persisted updated_at is using
datetime.now(UTC).isoformat(); replace this with the shared serializer by
importing and calling format_iso_utc from persistence._shared and pass
format_iso_utc(datetime.now(UTC)) into the SettingRow updated_at field (keep the
SettingRow(...) and NotBlankStr("_system") usage unchanged) so all persistence
timestamps use the common format_iso_utc formatter.
In `@src/synthorg/persistence/sqlite/circuit_breaker_repo.py`:
- Around line 138-151: The list_items method in circuit_breaker_repo.py accepts
limit/offset directly and must validate them before executing SQL; update the
async def list_items(...) to run the shared pagination validation (the same
helper used by other paginated repos) against limit and offset (e.g., ensure
limit is positive and capped, offset is non-negative) and raise or coerce
invalid values before calling self._db.execute, referencing DEFAULT_PAGE_SIZE
and the list_items function name so the change is localized and consistent with
other repo implementations.
In `@src/synthorg/persistence/sqlite/parked_context_repo.py`:
- Around line 107-120: The list_items method currently passes limit and offset
directly into the SQL query allowing negative values (which in SQLite can make
LIMIT behave as unbounded); update ParkedContextRepo.list_items to validate the
inputs before calling self._db.execute: ensure limit and offset are integers and
non-negative (e.g. raise ValueError for negative values or coerce to 0 for
offset and minimum 1 for limit), then use the validated values when executing
the SELECT from parked_contexts to preserve correct pagination semantics.
In `@src/synthorg/persistence/sqlite/settings_repo.py`:
- Around line 162-171: The pagination handling in list_items (function
list_items in settings_repo.py) clamps the incoming limit using min(limit,
1_000) which allows sentinel values like -1 through and embeds a magic number;
first call the shared pagination validator (the project's pagination validator
function) on limit/offset to enforce allowlisted values (0/1/-1 etc.), then
apply a cap using a named module-level constant (e.g., SETTINGS_LIST_MAX or
PAGINATION_MAX) defined in the canonical settings/definitions module instead of
1_000; replace the inline min(...) logic with validator-first logic and the
named constant reference, and update any imports to pull the constant from
settings/definitions.
In `@src/synthorg/persistence/sqlite/ssrf_violation_repo.py`:
- Around line 165-176: In list_items (async def list_items) validate the
incoming pagination parameters by calling validate_pagination_args(limit,
offset) before executing the SQLite query so negative or invalid values are
rejected; update the start of list_items to run validate_pagination_args(limit,
offset) (or assign validated values back if the helper returns normalized
values) and only then call self._db.execute with (limit, offset) to ensure a
buggy caller cannot pass a negative LIMIT/OFFSET to the SQL statement.
In `@src/synthorg/persistence/sqlite/training_plan_repo.py`:
- Around line 291-296: Both paginated methods currently call
validate_pagination_args(limit, 0, event=HR_TRAINING_PERSISTENCE_ERROR) which
only validates limit and uses a hardcoded offset of 0; change these calls in
list_items and query to validate both inputs by passing the actual offset
variable (e.g., validate_pagination_args(limit, offset,
event=HR_TRAINING_PERSISTENCE_ERROR)) before executing the SQL so a
caller-provided bad offset is checked; keep the same error event
(HR_TRAINING_PERSISTENCE_ERROR) and ensure the validated values are used in the
subsequent execute call.
---
Outside diff comments:
In `@src/synthorg/persistence/postgres/fine_tune_repo.py`:
- Around line 656-683: Replace the separate SELECT + DELETE race with an atomic
operation: issue a single DELETE ... RETURNING to delete the checkpoint only if
is_active = FALSE (e.g. "DELETE FROM fine_tune_checkpoints WHERE id = %s AND
is_active = FALSE RETURNING id") via the same cur.execute/cur.fetchone flow that
currently uses checkpoint_id; if RETURNING yields a row, deletion succeeded and
return True, if it yields no row then query the checkpoint existence (SELECT
is_active FROM fine_tune_checkpoints WHERE id = %s) to distinguish “became
active” (raise QueryError using MEMORY_FINE_TUNE_PERSIST_FAILED and the same
messages) versus “not found” (return False). This removes the read-then-delete
race around cur.rowcount while keeping the same error handling and using the
existing symbols checkpoint_id, MEMORY_FINE_TUNE_PERSIST_FAILED, QueryError,
cur.execute, cur.fetchone.
In `@src/synthorg/persistence/postgres/preset_override_repo.py`:
- Around line 38-63: get() currently calls self._row_to_override(row) directly
so any decoding errors (ValidationError/ValueError/TypeError/KeyError) propagate
instead of being translated to QueryError; wrap the row-to-domain conversion in
the same try/except handling used by list_items(): call
self._row_to_override(row) inside a try block and catch ValidationError,
ValueError, TypeError, KeyError (or a tuple of these) and re-raise a QueryError
with an appropriate message (and keep the original exception as the cause) while
preserving the existing PERSISTENCE_PRESET_OVERRIDE_QUERY_FAILED logging pattern
used in the top-level except; update get() to return the converted
PresetOverride on success or None if row is None.
In `@src/synthorg/persistence/postgres/preset_repo.py`:
- Around line 91-117: In the save method, the config_json value is being bound
directly to the JSONB column—change Preset.save to parse/validate the JSON
string with json.loads() and wrap the resulting Python object with psycopg
Jsonb() before passing it as the parameter for config_json in the UPSERT; update
the parameters tuple used in save (and mirror the same pattern used in
parked_context_repo.py, checkpoint_repo.py, escalation_repo.py) so psycopg can
adapt the value correctly and raise a QueryError if json.loads() fails.
In `@src/synthorg/persistence/postgres/session_repo.py`:
- Around line 90-129: Wrap all Postgres driver/pool calls in the session
repository (methods save, get, list_items, query, count, delete) in a try/except
that catches the DB-related exception(s), logs the structured event
API_AUTH_SESSION_PERSISTENCE_ERROR with contextual fields (e.g., session_id,
query params, exception info) via the repository logger, and then re-raise the
repository’s standard domain persistence error used by other Postgres repos (the
same exception class raised elsewhere in this package) so callers get the
consistent error type; apply this pattern inside save(), get(), list_items(),
query(), count(), and delete() (and the surrounding connection/cursor blocks) so
all driver failures are logged and translated before propagating.
In `@src/synthorg/persistence/sqlite/checkpoint_repo.py`:
- Around line 61-82: The append method currently uses
checkpoint.model_dump(mode="json") which may serialize created_at differently
than purge_before's format_iso_utc threshold comparison; update append (in
function append of Checkpoint repository) to serialize checkpoint.created_at
explicitly using format_iso_utc from persistence._shared (or normalize_utc if
the field is already a datetime) and inject that value into the data dict as the
context_json/created_at wire value before executing the INSERT, ensuring
created_at is always stored in the same ISO-UTC TEXT format that purge_before
expects.
In `@src/synthorg/persistence/sqlite/connection_repo.py`:
- Around line 233-267: Replace the current permissive pagination handling in
list_items (and similarly in query) with the shared validator: call
validate_pagination_args(limit, offset,
event=PERSISTENCE_CONNECTION_LIST_FAILED) at the start of each function to
validate/normalize inputs (and capture returned normalized limit/offset if the
helper returns them), remove the manual if limit <= 0/ max(0, int(offset))
logic, and keep the rest of the SQL/exception handling identical; ensure you
import validate_pagination_args and use the same event constant for the query
function (use the matching PERSISTENCE_CONNECTION_QUERY_FAILED event constant
there) so invalid pagination now raises a validation error consistently across
repositories.
In `@src/synthorg/persistence/sqlite/preset_repo.py`:
- Around line 137-178: The list_items method accepts limit and offset and passes
them directly into the SQL query, allowing negative values (e.g., LIMIT -1) to
change behavior; call the existing validate_pagination_args(limit, offset) at
the start of PresetRepo.list_items (or otherwise sanitize/normalize limit and
offset) to ensure non-negative, bounded pagination parameters before using them
in the execute call, and use the validated values in the SQL parameter tuple;
keep the existing error handling (QueryError) and logging
(PRESET_CUSTOM_LIST_FAILED, PRESET_CUSTOM_LISTED) intact.
In `@src/synthorg/persistence/sqlite/principle_override_repo.py`:
- Around line 130-145: The list_items method in PrincipleOverrideRepo passes
limit/offset directly to SQLite (in the SQL executed in list_items) without
validation, allowing values like LIMIT -1 to produce unbounded scans; add the
same validation/clamping used in sibling repos (e.g., ensure limit is an int
within 1..DEFAULT_PAGE_SIZE and offset is non-negative) before calling
self._db.execute, coerce types with int(), clamp/raise for out-of-range values,
and use the validated variables in the SELECT query so the SQL never receives a
negative or overly large limit/offset.
🪄 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
Run ID: 3ae26ff4-21bc-4807-9883-4f8440a0aab8
📒 Files selected for processing (300)
.claude/settings.json.claude/skills/babysit-pr/SKILL.md.claude/skills/pre-pr-review/SKILL.md.pre-commit-config.yamlCLAUDE.mddata/runtime_stats.yamldocs/decisions/0001-repository-protocol-consolidation.mddocs/design/persistence.mddocs/reference/convention-gates.mddocs/reference/persistence-boundary.mdscripts/check_image_signatures.pyscripts/check_no_api_dto_in_persistence_or_service.pyscripts/check_no_git_no_verify.shscripts/convention_gate_map.yamlscripts/schema_drift_baseline.txtsrc/synthorg/api/app.pysrc/synthorg/api/approval_store.pysrc/synthorg/api/auth/controller_helpers.pysrc/synthorg/api/auth/user_service.pysrc/synthorg/api/auto_wire.pysrc/synthorg/api/controllers/analytics.pysrc/synthorg/api/controllers/meta.pysrc/synthorg/api/controllers/webhooks.pysrc/synthorg/api/dto_org.pysrc/synthorg/api/dto_workflow.pysrc/synthorg/api/lifecycle_helpers/bootstrap.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/api/services/project_service.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/communication/bus/_nats_state.pysrc/synthorg/communication/bus/errors.pysrc/synthorg/communication/bus/memory.pysrc/synthorg/communication/bus/nats.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/communication/message.pysrc/synthorg/engine/artifacts/service.pysrc/synthorg/engine/checkpoint/callback_factory.pysrc/synthorg/engine/classification/loaders.pysrc/synthorg/engine/task_engine.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/hr/offboarding_service.pysrc/synthorg/integrations/connections/catalog.pysrc/synthorg/memory/backends/inmemory/adapter.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/memory/service.pysrc/synthorg/meta/rollout/mutators/prompt_mutator.pysrc/synthorg/meta/rules/custom.pysrc/synthorg/meta/rules/service.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/prometheus_collector.pysrc/synthorg/ontology/drift/layered.pysrc/synthorg/ontology/drift/passive.pysrc/synthorg/ontology/drift/service.pysrc/synthorg/ontology/injection/tool.pysrc/synthorg/ontology/memory_wrapper.pysrc/synthorg/ontology/service.pysrc/synthorg/organization/models.pysrc/synthorg/persistence/__init__.pysrc/synthorg/persistence/_generics.pysrc/synthorg/persistence/_shared/__init__.pysrc/synthorg/persistence/_shared/pagination.pysrc/synthorg/persistence/agent_state_protocol.pysrc/synthorg/persistence/approval_protocol.pysrc/synthorg/persistence/artifact_protocol.pysrc/synthorg/persistence/audit_protocol.pysrc/synthorg/persistence/auth_protocol.pysrc/synthorg/persistence/ceremony_scheduler_state_protocol.pysrc/synthorg/persistence/checkpoint_protocol.pysrc/synthorg/persistence/circuit_breaker_protocol.pysrc/synthorg/persistence/connection_protocol.pysrc/synthorg/persistence/cost_record_protocol.pysrc/synthorg/persistence/custom_rule_protocol.pysrc/synthorg/persistence/decision_protocol.pysrc/synthorg/persistence/fine_tune_protocol.pysrc/synthorg/persistence/integration_stubs.pysrc/synthorg/persistence/mcp_protocol.pysrc/synthorg/persistence/meeting_cooldown_protocol.pysrc/synthorg/persistence/memory_protocol.pysrc/synthorg/persistence/message_protocol.pysrc/synthorg/persistence/ontology_protocol.pysrc/synthorg/persistence/parked_context_protocol.pysrc/synthorg/persistence/postgres/agent_state_repo.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/persistence/postgres/artifact_repo.pysrc/synthorg/persistence/postgres/audit_repository.pysrc/synthorg/persistence/postgres/backend.pysrc/synthorg/persistence/postgres/ceremony_scheduler_state_repo.pysrc/synthorg/persistence/postgres/checkpoint_repo.pysrc/synthorg/persistence/postgres/circuit_breaker_repo.pysrc/synthorg/persistence/postgres/connection_repo.pysrc/synthorg/persistence/postgres/custom_rule_repo.pysrc/synthorg/persistence/postgres/decision_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/meeting_cooldown_repo.pysrc/synthorg/persistence/postgres/oauth_state_repo.pysrc/synthorg/persistence/postgres/ontology_drift_repo.pysrc/synthorg/persistence/postgres/ontology_entity_repo.pysrc/synthorg/persistence/postgres/org_fact_repo.pysrc/synthorg/persistence/postgres/parked_context_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/principle_override_repo.pysrc/synthorg/persistence/postgres/project_repo.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/persistence/postgres/repositories.pysrc/synthorg/persistence/postgres/revisions/20260515000001_ceremony_scheduler_state.sqlsrc/synthorg/persistence/postgres/risk_override_repo.pysrc/synthorg/persistence/postgres/schema.sqlsrc/synthorg/persistence/postgres/session_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/postgres/ssrf_violation_repo.pysrc/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/persistence/postgres/tracked_container_repo.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/postgres/training_result_repo.pysrc/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/preset_override_protocol.pysrc/synthorg/persistence/preset_protocol.pysrc/synthorg/persistence/principle_override_protocol.pysrc/synthorg/persistence/project_protocol.pysrc/synthorg/persistence/protocol.pysrc/synthorg/persistence/provider_audit_protocol.pysrc/synthorg/persistence/risk_override_protocol.pysrc/synthorg/persistence/settings_protocol.pysrc/synthorg/persistence/sqlite/_backend_accessors.pysrc/synthorg/persistence/sqlite/agent_state_repo.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/persistence/sqlite/artifact_repo.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/persistence/sqlite/ceremony_scheduler_state_repo.pysrc/synthorg/persistence/sqlite/checkpoint_repo.pysrc/synthorg/persistence/sqlite/circuit_breaker_repo.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/persistence/sqlite/custom_rule_repo.pysrc/synthorg/persistence/sqlite/decision_repo.pysrc/synthorg/persistence/sqlite/fine_tune_repo.pysrc/synthorg/persistence/sqlite/lockout_repo.pysrc/synthorg/persistence/sqlite/meeting_cooldown_repo.pysrc/synthorg/persistence/sqlite/oauth_state_repo.pysrc/synthorg/persistence/sqlite/ontology_drift_repo.pysrc/synthorg/persistence/sqlite/ontology_entity_repo.pysrc/synthorg/persistence/sqlite/org_fact_repo.pysrc/synthorg/persistence/sqlite/parked_context_repo.pysrc/synthorg/persistence/sqlite/preset_override_repo.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/sqlite/principle_override_repo.pysrc/synthorg/persistence/sqlite/project_repo.pysrc/synthorg/persistence/sqlite/provider_audit_repo.pysrc/synthorg/persistence/sqlite/repositories.pysrc/synthorg/persistence/sqlite/revisions/20260515000001_ceremony_scheduler_state.sqlsrc/synthorg/persistence/sqlite/risk_override_repo.pysrc/synthorg/persistence/sqlite/schema.sqlsrc/synthorg/persistence/sqlite/session_repo.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/persistence/sqlite/tracked_container_repo.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/persistence/sqlite/training_result_repo.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/persistence/sqlite/webhook_receipt_repo.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/ssrf_violation_protocol.pysrc/synthorg/persistence/subworkflow_protocol.pysrc/synthorg/persistence/task_protocol.pysrc/synthorg/persistence/tracked_container_protocol.pysrc/synthorg/persistence/training_protocol.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/version_protocol.pysrc/synthorg/persistence/workflow_definition_protocol.pysrc/synthorg/persistence/workflow_execution_protocol.pysrc/synthorg/providers/management/preset_override_service.pysrc/synthorg/settings/service.pysrc/synthorg/templates/preset_service.pysrc/synthorg/tools/sandbox/docker_sandbox.pysrc/synthorg/tools/sandbox/factory.pysrc/synthorg/tools/sandbox/reconciliation.pysrc/synthorg/versioning/models.pysrc/synthorg/workers/claim.pytests/conformance/persistence/test_agent_state_repository.pytests/conformance/persistence/test_approval_repository.pytests/conformance/persistence/test_artifact_repository.pytests/conformance/persistence/test_audit_repository.pytests/conformance/persistence/test_auth_repositories.pytests/conformance/persistence/test_ceremony_scheduler_state_repository.pytests/conformance/persistence/test_checkpoint_repository.pytests/conformance/persistence/test_circuit_breaker_repository.pytests/conformance/persistence/test_connection_repositories.pytests/conformance/persistence/test_core_repositories.pytests/conformance/persistence/test_custom_rule_repository.pytests/conformance/persistence/test_decision_repository.pytests/conformance/persistence/test_fine_tune_checkpoint_repository.pytests/conformance/persistence/test_fine_tune_run_repository.pytests/conformance/persistence/test_meeting_cooldown_repository.pytests/conformance/persistence/test_ontology_repositories.pytests/conformance/persistence/test_parked_context_repository.pytests/conformance/persistence/test_preset_override_repository.pytests/conformance/persistence/test_preset_repository.pytests/conformance/persistence/test_principle_override_repository.pytests/conformance/persistence/test_project_repository.pytests/conformance/persistence/test_settings_repository.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_tracked_container_repository.pytests/conformance/persistence/test_training_plan_repository.pytests/conformance/persistence/test_training_result_repository.pytests/conformance/persistence/test_user_repository.pytests/conformance/persistence/test_workflow_definition_repository.pytests/conformance/persistence/test_workflow_execution_repository.pytests/integration/api/test_user_deletion_cascade.pytests/integration/engine/test_error_taxonomy_integration.pytests/integration/engine/workflow/test_subworkflows_e2e.pytests/integration/mcp/test_tool_surface.pytests/integration/persistence/test_perf_indices_postgres.pytests/integration/persistence/test_perf_indices_sqlite.pytests/integration/persistence/test_postgres_backend.pytests/integration/persistence/test_postgres_jsonb_queries.pytests/integration/persistence/test_postgres_timescaledb.pytests/integration/persistence/test_settings_set_many.pytests/integration/persistence/test_sqlite_integration.pytests/integration/persistence/test_wp1_restart_safety.pytests/unit/api/auth/test_postgres_session_store.pytests/unit/api/auth/test_session_store.pytests/unit/api/auth/test_system_user.pytests/unit/api/controllers/test_messages.pytests/unit/api/controllers/test_setup_embedding.pytests/unit/api/controllers/test_webhooks_retry.pytests/unit/api/fake_user_repository.pytests/unit/api/fakes.pytests/unit/api/fakes_backend.pytests/unit/api/fakes_workflow.pytests/unit/api/services/test_org_mutations_atomic.pytests/unit/api/services/test_project_service.pytests/unit/api/services/test_ssrf_violation_service.pytests/unit/api/test_approval_store_concurrency.pytests/unit/api/test_auto_promote.pytests/unit/api/test_kill_switches.pytests/unit/api/test_webhook_receipt_cleanup_loop.pytests/unit/communication/conftest.pytests/unit/engine/artifacts/test_service.pytests/unit/engine/checkpoint/test_callback_factory.pytests/unit/engine/classification/test_loaders.pytests/unit/engine/task_engine_helpers.pytests/unit/engine/test_task_engine_coverage.pytests/unit/engine/test_task_engine_extended.pytests/unit/engine/test_task_engine_integration.pytests/unit/engine/test_task_engine_mutations.pytests/unit/engine/test_task_engine_review_fixes.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_execution_service.pytests/unit/engine/workflow/test_subworkflow_registry.pytests/unit/hr/test_offboarding_service.pytests/unit/integrations/test_connection_catalog_rebind.pytests/unit/memory/backends/inmemory/test_adapter.pytests/unit/memory/backends/mem0/test_adapter.pytests/unit/memory/embedding/test_fine_tune_orchestrator.pytests/unit/memory/org/conftest.pytests/unit/memory/test_service.pytests/unit/memory/test_service_fine_tune.pytests/unit/meta/mcp/test_all_handlers_wired.pytests/unit/meta/rollout/mutators/test_prompt_mutator.pytests/unit/meta/test_approval_repo.pytests/unit/meta/test_rules_service.pytests/unit/ontology/drift/test_service.pytests/unit/persistence/postgres/test_custom_rule_repo.pytests/unit/persistence/sqlite/test_artifact_repo.pytests/unit/persistence/sqlite/test_audit_repository.pytests/unit/persistence/sqlite/test_checkpoint_repo.pytests/unit/persistence/sqlite/test_circuit_breaker_repo.pytests/unit/persistence/sqlite/test_custom_rule_repo.pytests/unit/persistence/sqlite/test_preset_override_repo.pytests/unit/persistence/sqlite/test_preset_repo.pytests/unit/persistence/sqlite/test_project_repo.pytests/unit/persistence/sqlite/test_repositories.pytests/unit/persistence/sqlite/test_settings_repo.pytests/unit/persistence/sqlite/test_subworkflow_repo.pytests/unit/persistence/sqlite/test_user_repo.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/unit/persistence/test_factory.pytests/unit/persistence/test_generics_runtime_checkable.pytests/unit/persistence/test_protocol.pytests/unit/providers/test_audit_service.pytests/unit/providers/test_capability_mutations.pytests/unit/scripts/test_check_no_api_dto_in_persistence_or_service.pytests/unit/settings/test_bootstrap_only_api_entries.pytests/unit/settings/test_new_registry_entries.pytests/unit/settings/test_precedence_chain.pytests/unit/settings/test_readonly_init_settings.py
… translation, fail-closed deser, idempotent stubs, framing scrubs)
…ation (CodeRabbit pre-push sweep)
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/synthorg/persistence/sqlite/audit_repository.py (1)
80-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalise audit timestamps to UTC before persisting them.
dt.isoformat()preserves the caller's offset. Once mixed-offset rows land inaudit_entries, the newORDER BY timestamp DESCpath and time-window filters can return records in the wrong chronological order because SQLite is sorting/comparing text, not instants.Suggested fix
+from synthorg.persistence._shared import format_iso_utc, normalize_utc ... payload = audit_entry_to_payload( entry, json_serializer=json.dumps, - timestamp_serializer=lambda dt: dt.isoformat(), + timestamp_serializer=lambda dt: format_iso_utc(normalize_utc(dt)), )As per coding guidelines
src/synthorg/persistence/**/*.py: “Datetime in persistence: useparse_iso_utc/format_iso_utcfrompersistence._shared(reject naive datetimes); usenormalize_utcfor already-typed datetimes”.🤖 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/persistence/sqlite/audit_repository.py` around lines 80 - 84, The timestamp serializer currently uses dt.isoformat(), which preserves offsets and leads to mixed-offset rows; update the call to audit_entry_to_payload to normalize datetimes to UTC and format them using the persistence helpers: import and use normalize_utc and format_iso_utc (or for string inputs use parse_iso_utc then format_iso_utc) so the timestamp_serializer becomes a one-liner that normalizes the datetime to UTC and returns format_iso_utc(normalize_utc(dt)); ensure naive datetimes are rejected per guidelines by using parse_iso_utc where appropriate and reference audit_entry_to_payload, normalize_utc, format_iso_utc, and parse_iso_utc in your change.src/synthorg/persistence/postgres/preset_repo.py (1)
208-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd pagination validation to
list_items.Unlike other repository implementations in this PR (e.g.,
checkpoint_repo.py,connection_repo.py),list_itemsdoes not callvalidate_pagination_argsbefore executing the query. This allows invalidlimit/offsetvalues to reach the database instead of failing fast with a repository-level error.🛡️ Proposed fix
+from synthorg.persistence._shared import validate_pagination_args ... async def list_items( self, *, limit: int = DEFAULT_PAGE_SIZE, offset: int = 0, ) -> tuple[Preset, ...]: """List custom presets ordered by name. Args: limit: Maximum presets to return. offset: Rows to skip before the window. Raises: QueryError: If the database query fails. """ + limit = validate_pagination_args( + limit, offset, event=PRESET_CUSTOM_LIST_FAILED + ) try: async with self._pool.connection() as conn, conn.cursor() as cur:🤖 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/persistence/postgres/preset_repo.py` around lines 208 - 260, The list_items method is missing pagination validation; call the existing validate_pagination_args(limit, offset) at the start of the async def list_items(...) before touching self._pool so bad limit/offset values fail fast at the repo level; keep the existing try/except blocks and logging (PRESET_CUSTOM_LIST_FAILED, PRESET_CUSTOM_LISTED) intact and ensure any ValidationError from validate_pagination_args propagates rather than running the DB query or normalizing rows for Preset instances.src/synthorg/persistence/postgres/fine_tune_repo.py (1)
293-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a unique tie-breaker to both paginated recency sorts.
ORDER BY started_at DESCandORDER BY created_at DESCare not deterministic when multiple rows share the same timestamp, soLIMIT/OFFSETcan skip or repeat records across pages. Append a unique secondary key such asid DESCin both queries.Suggested fix
- "SELECT * FROM fine_tune_runs " - "ORDER BY started_at DESC LIMIT %s OFFSET %s", + "SELECT * FROM fine_tune_runs " + "ORDER BY started_at DESC, id DESC LIMIT %s OFFSET %s",- "SELECT * FROM fine_tune_checkpoints " - "ORDER BY created_at DESC LIMIT %s OFFSET %s", + "SELECT * FROM fine_tune_checkpoints " + "ORDER BY created_at DESC, id DESC LIMIT %s OFFSET %s",Also applies to: 558-561
🤖 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/persistence/postgres/fine_tune_repo.py` around lines 293 - 297, The ORDER BY clauses using only started_at or created_at are non-deterministic when timestamps tie; update the SQL in fine_tune_repo.py so both paginated queries append a unique secondary key (e.g. ", id DESC") to the ORDER BY — replace "ORDER BY started_at DESC" with "ORDER BY started_at DESC, id DESC" and likewise change "ORDER BY created_at DESC" to "ORDER BY created_at DESC, id DESC" (apply to the query shown with started_at and the other occurrence around the created_at query referenced at 558-561).
♻️ Duplicate comments (2)
.pre-commit-config.yaml (1)
683-689:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHook trigger regex misses
service/directory trees.Line 687 uses
.*\/service\.py, which only matches files literally namedservice.py. It does not match files under aservice/directory (for example,.../service/foo.py), so service-layer violations can bypass this gate.As per coding guidelines, ".pre-commit-config.yaml: **Use pre-commit/pre-push hooks via `.pre-commit-config.yaml`; tool-call gates via `.claude/settings.json` PreToolUse** (`scripts/check_*.sh`/`.py`)."Proposed fix
- files: ^(src/synthorg/(persistence/.*\.py|api/services/.*\.py|.*_service\.py|.*/service\.py)|scripts/check_no_api_dto_in_persistence_or_service\.py|\.pre-commit-config\.yaml)$ + files: ^(src/synthorg/(persistence/.*\.py|service/.*\.py|api/services/.*\.py|.*_service\.py|.*/service/.*\.py)|scripts/check_no_api_dto_in_persistence_or_service\.py|\.pre-commit-config\.yaml)$🤖 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 @.pre-commit-config.yaml around lines 683 - 689, The hook's files regex currently only matches files literally named service.py (the token ".*/service\.py") and therefore misses files under a service/ directory; update the regex for the hook id "no-api-dto-in-persistence-or-service" to also match service directory trees (for example replace ".*/service\.py" with a pattern that captures both a file named service.py and any files under a service/ directory such as "(.*/service(?:/.*)?\.py)"), so the files entry becomes ^(src/synthorg/(persistence/.*\.py|api/services/.*\.py|.*_service\.py|.*/service(?:/.*)?\.py)|scripts/check_no_api_dto_in_persistence_or_service\.py|\.pre-commit-config\.yaml)$ ensuring service/* files are inspected.src/synthorg/persistence/postgres/provider_audit_repo.py (1)
269-270: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove migration framing from docstring.
Line 270 still contains "(bespoke D7)" which is migration/ADR framing. As per coding guidelines, comments must explain WHY only with no migration framing.
♻️ Proposed fix
async def purge_before_id(self, *, before_id: int) -> int: - """Delete events with ``id < before_id`` (bespoke D7).""" + """Delete events with ``id < before_id``."""🤖 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/persistence/postgres/provider_audit_repo.py` around lines 269 - 270, The docstring for the function purge_before_id currently contains migration/ADR framing "(bespoke D7)"; remove that phrase so the docstring only describes what the function does and why (e.g., "Delete events with id < before_id")—update the purge_before_id docstring to drop the migration tag and keep a concise explanation of purpose and behavior.
🤖 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 `@scripts/check_no_git_no_verify.sh`:
- Around line 5-8: The header comment is out of sync with the implementation:
SHORT_NO_VERIFY_RE intentionally excludes `git push -n` while the comment claims
`-n` is blocked for both commit and push; update the header to accurately state
that `-n` is blocked for commit but `git push -n` is excluded by
`SHORT_NO_VERIFY_RE` (or alternatively change `SHORT_NO_VERIFY_RE` if you want
to actually block push -n); specifically reference and adjust the comment near
the `SHORT_NO_VERIFY_RE` definition and any surrounding lines so the prose
matches the implemented policy.
In `@src/synthorg/communication/bus/_nats_connection.py`:
- Around line 233-259: The TimeoutError handler in the stop logic raises
BusStopTimeoutError before clearing retained NATS handles, which leaks the
timed-out client; modify the TimeoutError except block in _nats_connection.py so
that it sets state.client = None, state.js = None, and state.kv = None (same
cleanup already done after the general except) before raising
BusStopTimeoutError (retain the existing logging and message built from
state.stop_drain_timeout_seconds) so the old connection is released even on
timeout.
In `@src/synthorg/engine/workflow/ceremony_scheduler.py`:
- Around line 387-389: The call to json.dumps for fired_once_triggers_json is
passing sort_keys=True which has no effect for list serialization; update the
code around fired_once_triggers_json (which serializes
sorted(self._fired_once_triggers)) to remove the misleading sort_keys=True
argument and just call json.dumps(sorted(self._fired_once_triggers)) so the list
ordering is controlled by sorted() and the json.dumps options are correct.
In `@src/synthorg/persistence/postgres/preset_override_repo.py`:
- Around line 83-85: The save() method currently raises QueryError when
PresetOverride.updated_at or updated_by are None without logging; before
raising, log a warning/error with context (include PresetOverride id,
updated_at, updated_by and any correlation ids) using the existing logger (e.g.,
module logger or self.logger) so the precondition failure is observable, then
raise QueryError as before; update the block that checks
override.updated_at/updated_by to emit the log entry immediately before the
raise.
In `@src/synthorg/persistence/postgres/session_repo.py`:
- Around line 152-164: The repository methods get, list_items, and query
currently call _row_to_session() inside a psycopg error translation context but
do not catch model-deserialisation errors (e.g., corrupted role or bad row
shape), so those exceptions escape instead of being translated to the repo's
QueryError; wrap the call to _row_to_session() (and any list/map of rows ->
_row_to_session) in a try/except that catches Exception (or ValueError/TypeError
as appropriate) and re-raise using the same translation/context used by
_translate_errors (include the same event message like "Failed to fetch session"
/ "Failed to list sessions" / "Failed to query sessions" and the session_id or
query params) so deserialisation failures are converted to the repository
QueryError consistent with other DB errors.
- Around line 243-253: The list_by_user and list_all implementations currently
only sanitize offset; update both to call validate_pagination_args(limit,
offset, event=API_AUTH_SESSION_PERSISTENCE_ERROR) (or equivalent) to enforce
min/max limits, then use the returned/validated limit and offset (e.g.,
effective_limit and effective_offset) in the SQL params tuple instead of
int(limit) and max(0,int(offset)); ensure you import/reference
validate_pagination_args and API_AUTH_SESSION_PERSISTENCE_ERROR and apply the
same change to the other session-listing block (the second occurrence around the
list_all logic).
In `@src/synthorg/persistence/postgres/webhook_receipt_repo.py`:
- Around line 309-328: The delete method in webhook_receipt_repo.py is logging
the wrong event constant (PERSISTENCE_WEBHOOK_RECEIPT_LIST_FAILED) on failure;
update the logger.warning call inside async def delete(self, entity_id:
NotBlankStr) to use a delete-specific event
(PERSISTENCE_WEBHOOK_RECEIPT_DELETE_FAILED), and if that constant does not exist
add it to the events/constants module so the logging call remains consistent
with other fields (keep the same kwargs: receipt_id, error_type, error) and
re-run tests to ensure no missing import or reference errors.
In `@src/synthorg/persistence/postgres/workflow_definition_repo.py`:
- Around line 496-511: list_items currently delegates to
query(WorkflowDefinitionFilterSpec()) which applies updated_at DESC, id DESC
ordering and breaks the documented id-ordered scan; change list_items to
explicitly perform an id-ordered scan by calling the repo-level query/selection
with an ORDER BY id ASC (or equivalent API flag) so the returned tuple is sorted
by id ascending, e.g. build a WorkflowDefinitionFilterSpec or SQL that sets
ordering to id ASC and pass limit/offset through, or if the intent was recency
ordering, rename list_items to something like list_items_by_recency to match the
new contract; key symbols: list_items, query, WorkflowDefinitionFilterSpec,
ordering.
In `@src/synthorg/persistence/sqlite/preset_repo.py`:
- Around line 129-136: The code currently constructs Preset(...) directly in
both get() and list_items(), which lets model-validation errors escape instead
of being translated to QueryError; add a shared helper (e.g.,
_row_to_preset(row, entity_id=None)) that attempts to build and return a Preset,
catches any Exception from the model construction, logs contextual details (use
logger and the PRESET_CUSTOM_FETCHED or similar constants) and rethrows a
QueryError with a clear message and context; then replace the direct Preset(...)
calls in get() and list_items() to call _row_to_preset so all row->model
failures are consistently logged and converted to QueryError.
In `@src/synthorg/persistence/sqlite/ssrf_violation_repo.py`:
- Around line 206-227: In the delete method of ssrf_violation_repo.py, the
exception handler logs PERSISTENCE_SSRF_VIOLATION_SAVE_FAILED which is
semantically wrong for a delete operation; change the event constant to a more
appropriate one (e.g. PERSISTENCE_SSRF_VIOLATION_QUERY_FAILED or add and use a
new PERSISTENCE_SSRF_VIOLATION_DELETE_FAILED) in the logger.warning call inside
delete(self, violation_id: NotBlankStr), keeping the same context kwargs
(violation_id, error_type, error) and raising PersistenceError as before.
---
Outside diff comments:
In `@src/synthorg/persistence/postgres/fine_tune_repo.py`:
- Around line 293-297: The ORDER BY clauses using only started_at or created_at
are non-deterministic when timestamps tie; update the SQL in fine_tune_repo.py
so both paginated queries append a unique secondary key (e.g. ", id DESC") to
the ORDER BY — replace "ORDER BY started_at DESC" with "ORDER BY started_at
DESC, id DESC" and likewise change "ORDER BY created_at DESC" to "ORDER BY
created_at DESC, id DESC" (apply to the query shown with started_at and the
other occurrence around the created_at query referenced at 558-561).
In `@src/synthorg/persistence/postgres/preset_repo.py`:
- Around line 208-260: The list_items method is missing pagination validation;
call the existing validate_pagination_args(limit, offset) at the start of the
async def list_items(...) before touching self._pool so bad limit/offset values
fail fast at the repo level; keep the existing try/except blocks and logging
(PRESET_CUSTOM_LIST_FAILED, PRESET_CUSTOM_LISTED) intact and ensure any
ValidationError from validate_pagination_args propagates rather than running the
DB query or normalizing rows for Preset instances.
In `@src/synthorg/persistence/sqlite/audit_repository.py`:
- Around line 80-84: The timestamp serializer currently uses dt.isoformat(),
which preserves offsets and leads to mixed-offset rows; update the call to
audit_entry_to_payload to normalize datetimes to UTC and format them using the
persistence helpers: import and use normalize_utc and format_iso_utc (or for
string inputs use parse_iso_utc then format_iso_utc) so the timestamp_serializer
becomes a one-liner that normalizes the datetime to UTC and returns
format_iso_utc(normalize_utc(dt)); ensure naive datetimes are rejected per
guidelines by using parse_iso_utc where appropriate and reference
audit_entry_to_payload, normalize_utc, format_iso_utc, and parse_iso_utc in your
change.
---
Duplicate comments:
In @.pre-commit-config.yaml:
- Around line 683-689: The hook's files regex currently only matches files
literally named service.py (the token ".*/service\.py") and therefore misses
files under a service/ directory; update the regex for the hook id
"no-api-dto-in-persistence-or-service" to also match service directory trees
(for example replace ".*/service\.py" with a pattern that captures both a file
named service.py and any files under a service/ directory such as
"(.*/service(?:/.*)?\.py)"), so the files entry becomes
^(src/synthorg/(persistence/.*\.py|api/services/.*\.py|.*_service\.py|.*/service(?:/.*)?\.py)|scripts/check_no_api_dto_in_persistence_or_service\.py|\.pre-commit-config\.yaml)$
ensuring service/* files are inspected.
In `@src/synthorg/persistence/postgres/provider_audit_repo.py`:
- Around line 269-270: The docstring for the function purge_before_id currently
contains migration/ADR framing "(bespoke D7)"; remove that phrase so the
docstring only describes what the function does and why (e.g., "Delete events
with id < before_id")—update the purge_before_id docstring to drop the migration
tag and keep a concise explanation of purpose and behavior.
🪄 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
Run ID: 284c4100-6a4e-4439-8b82-2116e7c5b272
📒 Files selected for processing (52)
.claude/settings.json.pre-commit-config.yamlCLAUDE.mddata/runtime_stats.yamldocs/reference/convention-gates.mdscripts/check_no_api_dto_in_persistence_or_service.pyscripts/check_no_git_no_verify.shsrc/synthorg/api/lifecycle_helpers/bootstrap.pysrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/communication/bus/_nats_state.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/__init__.pysrc/synthorg/persistence/_generics.pysrc/synthorg/persistence/integration_stubs.pysrc/synthorg/persistence/meeting_cooldown_protocol.pysrc/synthorg/persistence/postgres/agent_state_repo.pysrc/synthorg/persistence/postgres/connection_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/parked_context_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/persistence/postgres/session_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/agent_state_repo.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/persistence/sqlite/checkpoint_repo.pysrc/synthorg/persistence/sqlite/circuit_breaker_repo.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/persistence/sqlite/parked_context_repo.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/sqlite/principle_override_repo.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/sqlite/training_plan_repo.pytests/conformance/persistence/test_ceremony_scheduler_state_repository.pytests/conformance/persistence/test_connection_repositories.pytests/conformance/persistence/test_fine_tune_checkpoint_repository.pytests/conformance/persistence/test_meeting_cooldown_repository.pytests/conformance/persistence/test_project_repository.pytests/conformance/persistence/test_tracked_container_repository.pytests/unit/api/auth/test_postgres_session_store.pytests/unit/api/test_auto_promote.pytests/unit/communication/meeting/test_scheduler.pytests/unit/engine/workflow/test_ceremony_scheduler.pytests/unit/persistence/postgres/test_preset_override_repo.pytests/unit/persistence/postgres/test_preset_repo.pytests/unit/persistence/test_integration_stubs.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
- GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
- GitHub Check: Dashboard Test
- GitHub Check: Test Integration
- GitHub Check: Test Unit
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{md,rst}
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers
Files:
CLAUDE.mddocs/reference/convention-gates.md
**/*.{md,mermaid}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Mermaid for flowcharts / sequence / pipelines; use markdown tables for tabular data
Files:
CLAUDE.mddocs/reference/convention-gates.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Read
docs/design/page before implementing; deviations need approvalReuse
web/src/components/ui/design tokens in web dashboard components; design tokens onlyNo region/currency/locale privileged; use metric units; use British English
Every convention PR ships its enforcement gate
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 are pure env at the boot site; usesettings.bootstrap_resolver.resolve_init_valuefor pre-init Cat-2 reads; noos.environ.getoutside startupNo hardcoded numeric values outside
settings/definitions/; allow only 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] = literalTimeout/slow test failures indicate source-code regression; never edit
tests/baselines/unit_timing.jsonor anyscripts/*_baseline.{txt,json}/scripts/_*_baseline.pyfiles; useALLOW_BASELINE_GROWTH=1 git commit ...for explicit per-invocation bypassAfter issue implementation: create branch + commit + push without auto-PR; use
/pre-pr-reviewinsteadComments explain WHY only; no reviewer citations / issue back-references / migration framing
Use PEP 758 exception syntax:
except A, B:without parens unless binding; do not usefrom __future__ import annotations(Python 3.14 has PEP 649)Type hints on public functions; mypy strict mode; Google-style docstrings; line length 88; functions <50 lines; files <800 lines
Define custom errors as
<Domain><Condition>Errorinheriting fromDomainError, never directly fromException/RuntimeError/etc.Pydantic v2: use frozen models +
extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes); use@computed_fieldfor derived values; useNotBlankStrfor identifiersUse args models at every system boundary; use
parse_typed()for eve...
Files:
src/synthorg/communication/bus/_nats_state.pysrc/synthorg/persistence/postgres/parked_context_repo.pysrc/synthorg/persistence/sqlite/agent_state_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/api/lifecycle_helpers/bootstrap.pysrc/synthorg/persistence/postgres/agent_state_repo.pysrc/synthorg/persistence/sqlite/parked_context_repo.pysrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/persistence/meeting_cooldown_protocol.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/__init__.pysrc/synthorg/persistence/sqlite/checkpoint_repo.pysrc/synthorg/persistence/sqlite/principle_override_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/_generics.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/persistence/sqlite/circuit_breaker_repo.pysrc/synthorg/persistence/postgres/connection_repo.pysrc/synthorg/persistence/integration_stubs.pyscripts/check_no_api_dto_in_persistence_or_service.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/postgres/session_repo.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/communication/bus/_nats_state.pysrc/synthorg/persistence/postgres/parked_context_repo.pysrc/synthorg/persistence/sqlite/agent_state_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/api/lifecycle_helpers/bootstrap.pysrc/synthorg/persistence/postgres/agent_state_repo.pysrc/synthorg/persistence/sqlite/parked_context_repo.pysrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/persistence/meeting_cooldown_protocol.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/__init__.pysrc/synthorg/persistence/sqlite/checkpoint_repo.pysrc/synthorg/persistence/sqlite/principle_override_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/_generics.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/persistence/sqlite/circuit_breaker_repo.pysrc/synthorg/persistence/postgres/connection_repo.pysrc/synthorg/persistence/integration_stubs.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/postgres/session_repo.py
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQL; new repository protocols inherit from generic categories inpersistence/_generics.py(SingletonRepository/IdKeyedRepository/FilteredQueryRepository/AppendOnlyRepository/StatefulRepository/MVCCRepository); bespoke methods permitted only under ADR-0001 D7Keep repository protocols as
@runtime_checkableRepository CRUD methods:
save(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuplesDatetime in persistence: use
parse_iso_utc/format_iso_utcfrompersistence._shared(reject naive datetimes); usenormalize_utcfor already-typed datetimes
Files:
src/synthorg/persistence/postgres/parked_context_repo.pysrc/synthorg/persistence/sqlite/agent_state_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/persistence/postgres/agent_state_repo.pysrc/synthorg/persistence/sqlite/parked_context_repo.pysrc/synthorg/persistence/meeting_cooldown_protocol.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/__init__.pysrc/synthorg/persistence/sqlite/checkpoint_repo.pysrc/synthorg/persistence/sqlite/principle_override_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/_generics.pysrc/synthorg/persistence/sqlite/circuit_breaker_repo.pysrc/synthorg/persistence/postgres/connection_repo.pysrc/synthorg/persistence/integration_stubs.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/postgres/session_repo.py
.pre-commit-config.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Use pre-commit/pre-push hooks via
.pre-commit-config.yaml; tool-call gates via.claude/settings.jsonPreToolUse (scripts/check_*.sh/.py)
Files:
.pre-commit-config.yaml
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Construction-phase ordering invariant:
agent_registrymust be built BEFOREauto_wire_meetings;tunnel_provideris wired unconditionally (not gated byintegrations.enabled)On-startup ordering invariant:
SettingsServiceauto-wire must precedeWorkflowExecutionObserverregistration;OntologyServicewires afterpersistence.connect()via_wire_ontology_service
Files:
src/synthorg/api/lifecycle_helpers/bootstrap.py
**/*.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Follow canonical Bash rules from
~/.claude/rules/common/bash.mdforcd/git -C/ Bash file-write rules
Files:
scripts/check_no_git_no_verify.sh
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T00:50:45.917Z
Learning: Two-phase API startup lifecycle: construction phase (`create_app` body) wires synchronous services; on-startup phase (`_build_lifecycle.on_startup`) wires services needing connected persistence backend
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T00:50:45.917Z
Learning: Commits: `<type>: <description>` format (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T00:50:45.917Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T00:50:45.917Z
Learning: Branches: `<type>/<slug>` format from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T00:50:45.917Z
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-05-17T00:50:45.917Z
Learning: After every squash merge, run `/post-merge-cleanup`
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, do not require adding `<!--RS:...-->` “Doc Numeric Claims (MANDATORY)” numeric macros for Python version numbers mentioned in documentation prose (e.g., “Python 3.14”, “Python 3.15”). The `scripts/check_doc_numeric_macros.py` gate only applies to `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`, and it only flags digits adjacent to specific stat nouns (tests/providers/agents/stars/releases), not language version mentions like “Python 3.14”.
Applied to files:
CLAUDE.mddocs/reference/convention-gates.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo, account for the CI gate `check_doc_numeric_macros.py`: it skips fenced code blocks entirely, and it only flags digits that are adjacent to these stat nouns: `tests`, `providers`, `agents`, `stars`, `releases`. Therefore, numeric examples such as CLI flag values (e.g., `--num-workers=4` in fenced bash blocks) and prose version numbers (e.g., `3.14`/`3.15`) are not expected to trigger this check; prioritize changes only when digits appear next to one of the listed nouns (e.g., “5 tests”, “10 stars”, etc.).
Applied to files:
CLAUDE.mddocs/reference/convention-gates.md
📚 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/communication/bus/_nats_state.pysrc/synthorg/persistence/postgres/parked_context_repo.pysrc/synthorg/persistence/sqlite/agent_state_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/backend.pysrc/synthorg/api/lifecycle_helpers/bootstrap.pysrc/synthorg/persistence/postgres/agent_state_repo.pysrc/synthorg/persistence/sqlite/parked_context_repo.pysrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/persistence/meeting_cooldown_protocol.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/__init__.pysrc/synthorg/persistence/sqlite/checkpoint_repo.pysrc/synthorg/persistence/sqlite/principle_override_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/_generics.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/persistence/sqlite/circuit_breaker_repo.pysrc/synthorg/persistence/postgres/connection_repo.pysrc/synthorg/persistence/integration_stubs.pyscripts/check_no_api_dto_in_persistence_or_service.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/settings_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/postgres/session_repo.py
📚 Learning: 2026-05-16T18:36:19.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/guides/contributing.md:95-95
Timestamp: 2026-05-16T18:36:19.195Z
Learning: In the SynthOrg repo, the “Doc Numeric Claims (MANDATORY)” RS-marker rule should be applied only to these docs: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. This rule is enforced by scripts/check_doc_numeric_macros.py (with runtime substitution by scripts/inject_runtime_stats.py), so reviewers should not flag similar numeric-claim issues in other paths (e.g., anything under docs/guides/). When checking those scoped files, the rule skips fenced code blocks and only flags digits that are adjacent to stat nouns (tests/providers/agents/stars/releases). Numeric CLI flags like “--num-workers=4” inside fenced bash code blocks are not subject to this rule.
Applied to files:
docs/reference/convention-gates.md
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, follow the `Doc Numeric Claims (MANDATORY)` rule enforced by `scripts/check_doc_numeric_macros.py` only for these markdown files: `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`. The gate flags digits that appear adjacent to the stat nouns `tests`, `providers`, `agents`, `stars`, and `releases`—those numeric claims must use the required `<!--RS:...-->` macro format. Do not apply this rule to prose that mentions Python version numbers (e.g., “Python 3.14” / “Python 3.15”); those should not be flagged as requiring `<!--RS:...-->`.
Applied to files:
docs/reference/convention-gates.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: In the synthorg repo, the “Doc Numeric Claims (MANDATORY)” RS-marker rule is enforced only for this exact set of Markdown files: README.md, docs/index.md, docs/roadmap/index.md, docs/architecture/decisions.md, and docs/reference/convention-gates.md. During code reviews, do not raise RS-marker/numeric-claims findings for numeric values in any other files (e.g., docs/getting_started.md, docs/guides/*, docs/reference/conventions.md), since they are not checked or injected by scripts/check_doc_numeric_macros.py or scripts/inject_runtime_stats.py.
Applied to files:
docs/reference/convention-gates.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing markdown files for the "Doc Numeric Claims (MANDATORY)" RS-marker rule, only require/flag missing RS markers in the files that are actually in-scope for the rule. The scope is enforced via an identical _SCOPED_FILES allowlist in scripts/check_doc_numeric_macros.py and scripts/inject_runtime_stats.py, and currently includes: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. For any other markdown files (e.g., docs/getting_started.md, docs/guides/*), missing RS markers for numeric claims are no-ops and should NOT be flagged.
Applied to files:
docs/reference/convention-gates.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo against the `check_doc_numeric_macros.py` gate, account for its documented behavior: it skips fenced code blocks entirely, and it only flags digits that are adjacent to specific stat nouns (`tests`, `providers`, `agents`, `stars`, `releases`). As a result, CLI-style numbers (e.g., `--num-workers=4`) inside fenced bash code blocks should never be treated as violations of this gate; only non-fenced text needs checking, and only around those specific nouns.
Applied to files:
docs/reference/convention-gates.md
🔇 Additional comments (63)
.claude/settings.json (1)
42-46: LGTM!data/runtime_stats.yaml (1)
2-3: LGTM!Also applies to: 6-6, 10-10, 23-24
CLAUDE.md (1)
13-13: LGTM!src/synthorg/communication/bus/_nats_state.py (1)
29-34: LGTM!Also applies to: 65-74
src/synthorg/persistence/postgres/parked_context_repo.py (1)
26-27: LGTM!Also applies to: 136-169
src/synthorg/persistence/sqlite/agent_state_repo.py (1)
21-27: LGTM!Also applies to: 120-150
src/synthorg/persistence/postgres/preset_override_repo.py (1)
8-22: LGTM!Also applies to: 55-79, 81-82, 119-126, 128-179, 193-193
.pre-commit-config.yaml (1)
624-635: LGTM!src/synthorg/persistence/sqlite/backend.py (1)
36-36: LGTM!Also applies to: 49-51, 89-91, 149-151, 237-241, 303-305, 565-576, 822-843
src/synthorg/api/lifecycle_helpers/bootstrap.py (1)
19-24: LGTM!Also applies to: 28-79
src/synthorg/persistence/postgres/agent_state_repo.py (1)
21-27: LGTM!Also applies to: 131-165
src/synthorg/persistence/sqlite/parked_context_repo.py (1)
20-21: LGTM!Also applies to: 108-137
src/synthorg/persistence/meeting_cooldown_protocol.py (1)
1-129: LGTM!scripts/check_no_git_no_verify.sh (1)
20-76: LGTM!src/synthorg/persistence/sqlite/audit_repository.py (1)
124-228: LGTM!src/synthorg/persistence/__init__.py (1)
19-98: LGTM!src/synthorg/persistence/sqlite/checkpoint_repo.py (1)
62-112: LGTM!Also applies to: 181-253
src/synthorg/persistence/sqlite/principle_override_repo.py (1)
41-76: LGTM!Also applies to: 131-169
src/synthorg/persistence/postgres/webhook_receipt_repo.py (1)
83-147: LGTM!Also applies to: 266-307
src/synthorg/persistence/sqlite/ssrf_violation_repo.py (1)
166-204: LGTM!src/synthorg/persistence/postgres/provider_audit_repo.py (1)
174-180: LGTM!Also applies to: 182-247, 249-267
src/synthorg/persistence/sqlite/connection_repo.py (1)
234-329: LGTM!src/synthorg/persistence/postgres/preset_repo.py (1)
62-79: LGTM!Also applies to: 112-149, 151-206, 262-346
src/synthorg/engine/workflow/ceremony_scheduler.py (8)
12-18: LGTM!
49-65: LGTM!
109-156: LGTM!
273-294: LGTM!
309-368: LGTM!
370-408: LGTM!
467-483: LGTM!
551-553: LGTM!src/synthorg/persistence/_generics.py (7)
1-57: LGTM!
60-91: LGTM!
93-126: LGTM!
128-160: LGTM!
162-203: LGTM!
205-256: LGTM!
258-292: LGTM!docs/reference/convention-gates.md (2)
33-33: LGTM!
59-59: LGTM!src/synthorg/communication/meeting/scheduler.py (7)
44-46: LGTM!Also applies to: 86-102, 104-157
167-192: LGTM!
215-261: LGTM!
263-297: LGTM!
351-351: LGTM!
474-478: LGTM!
566-571: LGTM!src/synthorg/persistence/sqlite/circuit_breaker_repo.py (4)
16-21: LGTM!
92-137: LGTM!
139-186: LGTM!
230-258: LGTM!src/synthorg/persistence/postgres/connection_repo.py (4)
35-41: LGTM!
215-254: LGTM!
256-298: LGTM!
300-322: LGTM!src/synthorg/persistence/integration_stubs.py (4)
31-33: LGTM!
51-89: LGTM!
139-166: LGTM!
175-238: LGTM!scripts/check_no_api_dto_in_persistence_or_service.py (4)
1-81: LGTM!
83-130: LGTM!
132-165: LGTM!
168-221: LGTM!
… singleton error-path tests)
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
…ntime_stats regen
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/ceremony_scheduler.py (1)
280-295:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep activation atomic when the first snapshot write fails.
_save_state_unlocked()now runs after the rollback-protected activation block. If that write fails,activate_sprint()raises even though_runningis already true, the strategy hook has run, and sprint-start ceremonies may already have fired. A retry can then double-trigger startup work against a scheduler that never rolled back.🔧 Suggested fix
try: await strategy.on_sprint_activated(sprint, config) await self._fire_sprint_start_ceremonies(sprint, config) + await self._save_state_unlocked(sprint.id) except MemoryError, RecursionError: raise except Exception: logger.exception( SPRINT_CEREMONY_SCHEDULER_START_FAILED, @@ await self._deactivate_sprint_unlocked() raise - - await self._save_state_unlocked(sprint.id) logger.info( SPRINT_CEREMONY_SCHEDULER_STARTED, sprint_id=sprint.id,🤖 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/ceremony_scheduler.py` around lines 280 - 295, The activation sequence is not atomic because _save_state_unlocked() is called after the try/except that protects rollback, so if that save fails the sprint remains active and startup hooks/ceremonies have already run; move the await self._save_state_unlocked(sprint.id) call inside the protected try block immediately after await self._fire_sprint_start_ceremonies(sprint, config) (keeping the existing except MemoryError, RecursionError re-raises and the generic Exception handler that logs SPRINT_CEREMONY_SCHEDULER_START_FAILED and calls await self._deactivate_sprint_unlocked()), so any failure to persist state triggers the same rollback path and the exception is re-raised.
🤖 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 `@scripts/check_no_git_no_verify.sh`:
- Around line 48-64: The bypass detection misses Git's
--config-env=<key>=<envvar> form, so update the BYPASS_RE regex (used where
COMMAND is tested) to also detect --config-env=core.hooksPath= and
--config-env=commit.gpgsign= (case‑insensitive), e.g. add an alternation
matching
--config-env[[:space:]]*=[^[:space:]]*(core\.hooksPath|commit\.gpgsign)[[:space:]=]|$
so that COMMAND checks using BYPASS_RE will catch attempts to disable hooks or
signing via --config-env; keep the grep -qiE usage so casing is ignored.
In `@src/synthorg/engine/workflow/ceremony_scheduler.py`:
- Around line 336-367: The decoded payloads need structural validation before
mutating state: ensure `counters` is a mapping of string keys to integer (or
int-convertible, non-negative) values and `triggers` is an iterable of hashable
string identifiers; if validation fails log the failure (similar to the existing
warning) and return without touching `_completion_counters`,
`_fired_once_triggers`, or `_total_completions`. Implement validation after JSON
decoding and before the merge loop (use the same `sprint_id`/logger context and
`SPRINT_CEREMONY_SCHEDULER_START_FAILED` note), only then update
`_completion_counters` by assigning validated ints for keys that already exist,
set `_fired_once_triggers` to the validated set, and assign
`record.total_completions` to `_total_completions`.
In `@src/synthorg/persistence/postgres/fine_tune_repo.py`:
- Around line 309-333: In delete (async def delete) move the read of
cur.rowcount inside the async with block so it's captured before the
cursor/context closes: after await cur.execute(...), set a local boolean (e.g.,
deleted = cur.rowcount > 0) while still inside the async with
self._pool.connection() as conn, conn.cursor() as cur: block, then await
conn.commit() and return that local boolean after the context; keep the existing
psycopg.Error except block and QueryError raising unchanged.
---
Outside diff comments:
In `@src/synthorg/engine/workflow/ceremony_scheduler.py`:
- Around line 280-295: The activation sequence is not atomic because
_save_state_unlocked() is called after the try/except that protects rollback, so
if that save fails the sprint remains active and startup hooks/ceremonies have
already run; move the await self._save_state_unlocked(sprint.id) call inside the
protected try block immediately after await
self._fire_sprint_start_ceremonies(sprint, config) (keeping the existing except
MemoryError, RecursionError re-raises and the generic Exception handler that
logs SPRINT_CEREMONY_SCHEDULER_START_FAILED and calls await
self._deactivate_sprint_unlocked()), so any failure to persist state triggers
the same rollback path and the exception is re-raised.
🪄 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
Run ID: e355dbb6-c394-4e24-9d5c-f7878d45055c
📒 Files selected for processing (18)
.pre-commit-config.yamldata/runtime_stats.yamlscripts/check_no_git_no_verify.shsrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/persistence/postgres/session_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pytests/conformance/persistence/test_pagination_validation_conformance.pytests/unit/persistence/sqlite/test_singleton_repos_error_paths.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
- GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: Dashboard Test
- GitHub Check: Test Integration
- GitHub Check: Test Unit
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,json,yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
d2for architecture / nested containers; usemermaidfor flowcharts / sequence / pipelines; use Markdown tables for tabular data; D2 theme 200 (Dark Mauve), D2 CLI pinned to v0.7.1 in CI
Files:
data/runtime_stats.yamlsrc/synthorg/communication/bus/_nats_connection.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/session_repo.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Read design spec in
docs/design/before implementing; deviations require approval per DESIGN_SPEC.mdPresent every plan for accept/deny before coding
No region/currency/locale should be privileged; use metric units and British English per docs/reference/regional-defaults.md
Every convention PR must ship its enforcement gate per docs/reference/convention-gates.md
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 are pure env at boot site; noos.environ.getoutside startup; pre-init Cat-2 reads usesettings.bootstrap_resolver.resolve_init_value; YAML is company-template ingestion format, not a precedence tierNo hardcoded numeric values; 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 byscripts/check_no_magic_numbers.pyTest regression: timeout/slow failures indicate source-code regression; never edit
tests/baselines/unit_timing.jsonor anyscripts/*_baseline.{txt,json}/scripts/_*_baseline.py; per-invocation bypass for gate baselines:ALLOW_BASELINE_GROWTH=1 git commit ...After issue: create branch + commit + push (no auto-PR); use
/pre-pr-review(gh pr createis blocked); after PR: use/aurelio-review-prfor external feedback; fix EVERYTHING valid; no deferringComments should explain WHY only; no reviewer citations / issue back-refs / migration framing; enforced by
check_no_review_origin_in_code.py+check_no_migration_framing.pyNo
from __future__ import annotations(Python 3.14 has PEP 649); use PEP 758 except:except A, B:requires parens when bindingType hints on public functions; mypy strict mode; Google-style docstrings; line length 88; functions <50 lines; files <800 lines
Error classes must follow
<Domain><Condition>Errorpat...
Files:
src/synthorg/communication/bus/_nats_connection.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/session_repo.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/communication/bus/_nats_connection.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/session_repo.py
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
See
~/.claude/rules/common/bash.mdfor canonicalcd/git -C/ Bash file-write rules
Files:
scripts/check_no_git_no_verify.sh
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQL; new repository protocols must inherit frompersistence/_generics.py(SingletonRepository/IdKeyedRepository/FilteredQueryRepository/AppendOnlyRepository/StatefulRepository/MVCCRepository); bespoke methods only under ADR-0001 D7; keep protocols@runtime_checkableRepository CRUD: implement
save(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuplesDatetime in persistence: use
parse_iso_utc/format_iso_utcfrompersistence._shared(reject naive datetimes); usenormalize_utcfor already-typed
Files:
src/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/session_repo.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:35:55.194Z
Learning: API startup lifecycle: two phases - construction (`create_app` body) wires synchronous services; on_startup (`_build_lifecycle.on_startup`) wires services needing connected persistence
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:35:55.194Z
Learning: Construction-phase ordering: `agent_registry` BEFORE `auto_wire_meetings`; `tunnel_provider` wired unconditionally (not gated by `integrations.enabled`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:35:55.194Z
Learning: On-startup ordering: `SettingsService` auto-wire precedes `WorkflowExecutionObserver` registration (so it picks up resolver-driven `max_subworkflow_depth`); `OntologyService` wires after `persistence.connect()` via `_wire_ontology_service`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:35:55.194Z
Learning: `post_setup_reinit()` (provider reload + agent bootstrap) propagates failures; `settings_svc.set("api", "setup_complete", "true")` only after reinit succeeds; entire check/validate/reinit/persist sequence serialized under `COMPLETE_LOCK` in `src/synthorg/api/controllers/setup/agent_helpers.py` to prevent race conditions on flag write
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:35:55.194Z
Learning: WebSocket: per-frame timeout closes silent peers (1008); revalidation saturation closes (4011)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:35:55.194Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:35:55.194Z
Learning: After every squash merge → run `/post-merge-cleanup`
📚 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/communication/bus/_nats_connection.pysrc/synthorg/persistence/sqlite/ssrf_violation_repo.pysrc/synthorg/persistence/sqlite/audit_repository.pysrc/synthorg/persistence/postgres/provider_audit_repo.pysrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/webhook_receipt_repo.pysrc/synthorg/persistence/postgres/preset_override_repo.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/session_repo.py
🔇 Additional comments (25)
data/runtime_stats.yaml (1)
2-24: LGTM!.pre-commit-config.yaml (1)
624-635: LGTM!Also applies to: 683-689
src/synthorg/communication/bus/_nats_connection.py (1)
18-18: LGTM!Also applies to: 189-266
src/synthorg/persistence/sqlite/ssrf_violation_repo.py (1)
3-3: LGTM!Also applies to: 16-22, 166-227
src/synthorg/persistence/sqlite/audit_repository.py (1)
16-18: LGTM!Also applies to: 71-85, 125-239
src/synthorg/persistence/postgres/provider_audit_repo.py (1)
22-31: LGTM!Also applies to: 174-267
src/synthorg/persistence/postgres/workflow_definition_repo.py (2)
432-494: LGTM!
496-561: LGTM!src/synthorg/persistence/postgres/webhook_receipt_repo.py (3)
84-148: LGTM!
267-329: LGTM!
331-337: LGTM!src/synthorg/persistence/postgres/preset_override_repo.py (1)
55-79: LGTM!Also applies to: 81-133, 135-185, 187-206
src/synthorg/persistence/sqlite/preset_repo.py (3)
30-53: LGTM!Also applies to: 81-158
160-246: LGTM!
248-279: LGTM!src/synthorg/persistence/postgres/preset_repo.py (3)
63-80: LGTM!Also applies to: 113-207
209-317: LGTM!
319-349: LGTM!src/synthorg/persistence/postgres/fine_tune_repo.py (3)
144-216: LGTM!Also applies to: 244-307
425-533: LGTM!
535-572: LGTM!Also applies to: 640-693
src/synthorg/persistence/postgres/session_repo.py (4)
67-91: LGTM!Also applies to: 109-167
169-236: LGTM!
246-265: LGTM!Also applies to: 274-292
294-311: LGTM!Also applies to: 313-366, 395-418
…review 4304640018 (4 findings)
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/persistence/postgres/repositories.py`:
- Around line 723-744: The query() method currently only checks limit and can
accept negative offsets; update it to use the shared pagination validator
(validate_pagination_args) the same way PostgresTaskRepository.list_items does:
call validate_pagination_args(limit, offset) at the start of query() and remove
the ad-hoc limit check, ensuring both limit and offset are validated before
building the SQL and using params (refer to the query() signature and
PostgresTaskRepository.list_items for the validator usage).
- Around line 270-298: The query() method is missing pagination validation which
allows negative limit/offset to reach the SQL; call the existing
validate_pagination_args(limit, offset, ...) at the start of query() (same
validation used by list_items()) and remove the redundant int() casts when
appending limit and offset to params so validated values are used directly;
update references in query() (function name: query, and uses of params.extend
and the final query string) to use the validated values.
🪄 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
Run ID: 1322d0ab-08a0-4a03-9bda-dd553116c890
📒 Files selected for processing (5)
scripts/check_no_git_no_verify.shsrc/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/fine_tune_repo.pysrc/synthorg/persistence/postgres/principle_override_repo.pysrc/synthorg/persistence/postgres/repositories.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
- GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: Test E2E
- GitHub Check: Test Unit
- GitHub Check: Test Integration
- GitHub Check: Dashboard Test
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Bash rules and file-write conventions: see
~/.claude/rules/common/bash.md(canonical forcd/git -C/ file-write rules)
Files:
scripts/check_no_git_no_verify.sh
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQLNumerics live in
settings/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)Error classes must follow
<Domain><Condition>Errornaming fromDomainError; never inheritException/RuntimeErrordirectlyPydantic v2 DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes) must be frozen with
extra="forbid"; use@computed_fieldfor derived fields; useNotBlankStrfor identifiersArgs models required at every system boundary; use
parse_typed()for every external dict ingestionImmutability: use
model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundariesAsync: use
asyncio.TaskGroupfor fan-out/fan-in; helpers catchExceptionand re-raiseMemoryError/RecursionErrorClock seam: inject
clock: Clock | None = None; tests useFakeClock. Lifecycle: services own_lifecycle_lock; timed-out stops mark unrestartableUntrusted content (SEC-1): wrap with
wrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTMLImport logger via
from synthorg.observability import get_logger; variable name alwayslogger; never useimport loggingorprint()in app codeEvent names from
observability.events.<domain>constants; use structured kwargs for logging (logger.info(EVENT, key=value))Error paths log WARNING/ERROR with context before raising; state transitions log INFO via
*_STATUS_TRANSITIONEDAFTER persistence writeSecret-log redaction (SEC-1): never log
error=str(exc)or interpolate{exc}; useerror_type=type(exc).__name__+error=safe_error_description(exc). Neverexc_info=True. OTel: forbidspan.record_exception(exc); usespan.set_attribute("exception.message", safe_error_description(exc))withrecord_exception=False, set_status_on_exception=False
Files:
src/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/principle_override_repo.pysrc/synthorg/persistence/postgres/repositories.pysrc/synthorg/persistence/postgres/fine_tune_repo.py
src/**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Comments explain WHY only; no reviewer citations, issue back-refs, or migration framing
Never use real vendor names in project code/tests; use
example-provider,test-provider,example-{large,medium,small}-001. Allowed in.claude/, third-party imports,providers/presets.py,web/public/provider-logos/
Files:
src/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/principle_override_repo.pysrc/synthorg/persistence/postgres/repositories.pysrc/synthorg/persistence/postgres/fine_tune_repo.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
No
from __future__ import annotations(Python 3.14 has PEP 649); PEP 758 exception handling except:except A, B:requires parens if bindingType hints required on public functions; mypy strict mode; Google-style docstrings; line length 88; functions <50 lines; files <800 lines
Files:
src/synthorg/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/principle_override_repo.pysrc/synthorg/persistence/postgres/repositories.pysrc/synthorg/persistence/postgres/fine_tune_repo.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/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/principle_override_repo.pysrc/synthorg/persistence/postgres/repositories.pysrc/synthorg/persistence/postgres/fine_tune_repo.py
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
New repository protocols must inherit from
persistence/_generics.pygeneric categories (SingletonRepository,IdKeyedRepository,FilteredQueryRepository,AppendOnlyRepository,StatefulRepository,MVCCRepository); bespoke methods only under ADR-0001 D7Repository CRUD methods:
save(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuplesDatetime in persistence: use
parse_iso_utc/format_iso_utcfrompersistence._shared; reject naive datetimes; usenormalize_utcfor already-typed
Files:
src/synthorg/persistence/postgres/principle_override_repo.pysrc/synthorg/persistence/postgres/repositories.pysrc/synthorg/persistence/postgres/fine_tune_repo.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Read `docs/design/` page before implementing; deviations need approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: No region/currency/locale privileged; use metric units; use British English
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Every convention PR must ship its enforcement gate via `scripts/check_*.py` or `.sh` hooks
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Timeout/slow test failures indicate source-code regression; never edit `tests/baselines/unit_timing.json` or any `scripts/*_baseline.{txt,json}` / `scripts/_*_baseline.py`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Use `/pre-pr-review` command after issue; branch + commit + push before PR (no auto-PR). Use `/aurelio-review-pr` for external feedback after PR creation
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: xdist: `-n 8 --dist=loadfile` applied via pyproject addopts (loadfile prevents 3.14+ Windows ProactorEventLoop leak)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: Pre-commit/pre-push hooks via `.pre-commit-config.yaml`; tool-call gates via `.claude/settings.json` PreToolUse hooks
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
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-05-17T01:53:52.384Z
Learning: GitHub queries: use `gh issue list` via Bash, NOT MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
Learning: After every squash merge → run `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-17T01:53:52.384Z
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/engine/workflow/ceremony_scheduler.pysrc/synthorg/persistence/postgres/principle_override_repo.pysrc/synthorg/persistence/postgres/repositories.pysrc/synthorg/persistence/postgres/fine_tune_repo.py
🔇 Additional comments (33)
scripts/check_no_git_no_verify.sh (4)
1-31: LGTM!
33-42: LGTM!
44-59: LGTM!
61-78: LGTM!src/synthorg/engine/workflow/ceremony_scheduler.py (9)
12-18: LGTM!
49-65: LGTM!
109-109: LGTM!
120-146: LGTM!
273-287: LGTM!
313-394: LGTM!
396-432: LGTM!
491-507: LGTM!
575-577: LGTM!src/synthorg/persistence/postgres/principle_override_repo.py (3)
16-18: LGTM!
115-138: LGTM!
46-52: 💤 Low value[No action required —
restored_fromis correctly typed asNotBlankStr(non-optional), making it always non-null.]The
PrincipleOverridemodel definesrestored_from: NotBlankStrwithout a default value, which Pydantic v2 enforces as a required non-nullable field. The typeNotBlankStr(per coding guidelines) is reserved for non-blank string identifiers that cannot beNone. Therefore,str(entity.restored_from)will never produce the string"None"— the value is always a valid non-blank string. The code at line 49 is correct.> Likely an incorrect or invalid review comment.src/synthorg/persistence/postgres/repositories.py (8)
44-58: LGTM!
229-268: LGTM!
319-354: LGTM!
385-424: LGTM!
426-453: LGTM!
565-583: LGTM!
600-644: LGTM!
763-781: LGTM!src/synthorg/persistence/postgres/fine_tune_repo.py (9)
32-32: LGTM!
144-190: LGTM!
192-216: LGTM!
244-270: LGTM!
272-307: LGTM!
309-336: LGTM!
428-481: LGTM!
509-575: LGTM!
643-696: LGTM!
…tRecord query pagination validation)
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
… unit error-path + pagination tests)
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
…nit error-path tests)
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
added a commit
that referenced
this pull request
May 17, 2026
# WP-2: Security + Audit Chain Completion Closes #1917. One PR per the issue. ## Context WP-1 (#1945, this branch's base) already landed most of the audit-chain emission refactor. Verifying each issue checkbox against current source showed the genuinely-remaining work was narrower than the issue text (written pre-WP-1) implied. This PR completes the real gaps and records the already-satisfied / dismissed items so nothing is silently dropped. User decisions (AskUserQuestion): full-OIDC nonce binding; build the missing refresh-rotation endpoint; converge SSE onto the WS revalidation model. All three are the best-in-class options. ## Deliverables ### Done in this PR - **Redact URLs in `http_request.py` logs** (4 sites) + **remove `exc_info` in `probing.py`** (`5ef64929d`). - **OAuth full-OIDC nonce binding**: `OAuthState.nonce` persisted (model + schema + repos, new nullable `oauth_states.nonce` column, yoyo revision per backend) `32700e3be`; nonce generated in `start_flow`, `id_token` surfaced `094e92c6b`; `oidc_verify.py` (asymmetric-only alg allowlist, issuer + audience + exp + nonce verification, JWKS via `PyJWKClient` off-loaded with `asyncio.to_thread`, leeway) `74f19908c`; fail-closed callback matrix `297f4e00c`. - **OAuthStateService on the callback path** (`get` / `expire` / `mark_consumed`; `handle_oauth_callback` takes the service, not the bare repo) `8db5bb7b6`. - **`POST /auth/refresh` single-use rotation endpoint** + `RefreshTokenInvalidError` (1005) + `create_token` session-id reuse + auth-exclude-path fail-safe `c38cfe46e`. - **Unified SSE/WS revalidation** onto one cadence constant (`AUTH_REVALIDATE_INTERVAL_SECONDS`) + one sliding-window failure model + shared `api.auth_revalidate_window_seconds` / `api.auth_revalidate_max_failures` settings `48a095674`. - **`LHCI_GITHUB_APP_TOKEN` scoped to a dedicated `lighthouse` GitHub Environment** `15b20b72c` (reworked in `6a888b2ee`; see note below). ### Already satisfied in WP-1 (no code; evidence) - User create / role-update / delete signed audit events: `users.py` routes through `UserService` (`src/synthorg/api/auth/user_service.py:151-247`) emitting `SECURITY_USER_CREATED` / `_UPDATED` / `_DELETED` / `_DELETE_FAILED`. The audit-chain sink allowlist (`_AUDITED_PREFIXES = ("security.", "tool.registry.integrity.")`) signs every `security.user.*`. - `providers/probing.py` URL redaction: already `_redact_url(url)`; only the `exc_info=True` removal remained (done here). - Approval status transitions: `approvals.py:357-373` emits `SECURITY_APPROVAL_APPROVED` / `_REJECTED`. - Autonomy promotions: `autonomy.py:116-131` emits `SECURITY_AUTONOMY_PROMOTION_REQUESTED` / `_DENIED` (all changes route through human approval; no auto-apply path exists). - Refresh-token CREATED / REVOKED: `service.py:339` / `user_service.py:233`. CONSUMED / REJECTED had no call site because rotation was unwired; this PR builds the endpoint that exercises them. ### Dismissed (FALSE_POSITIVE, per the issue, no code) - 3 "Python 2 syntax" findings (`api/auth/controller.py:757`, `oauth/state_service.py:74`, `oauth/callback_handler.py:56`): `except A, B:` is valid Python 3.14 (PEP 758). Not touched. - `init --encrypt-secrets` "undocumented": it IS documented in `cli/CLAUDE.md:107`. No change. ## Operator / reviewer notes - **Manual repo-settings step**: create a GitHub Environment named `lighthouse` and define `LHCI_GITHUB_APP_TOKEN` as an environment secret there. It carries **no deployment branch policy** by design: the lighthouse jobs run on `pull_request` (ref `refs/pull/N/merge`), which GitHub branch policies cannot match, and `Lighthouse Pass` is a REQUIRED check, so a `main` policy would wedge every web/site PR. The control is a workflow-level fork-PR `if:` gate (same-repo / `workflow_dispatch` only), the sanctioned `cloudflare-preview` pattern. `scripts/configure_environments.sh` deliberately omits `lighthouse` from `ENV_CONFIG`; see `docs/reference/github-environments.md`. (The issue's "with branch policy" wording predates this repo's documented PR-ref constraint; the no-policy + fork-gate pattern is the correct realisation: the token is no longer exposed to arbitrary workflow runs.) - **Schema change**: `oauth_states.nonce` (nullable TEXT) ships as a new yoyo revision per backend (`20260517000001_oauth_state_nonce.sql`). Schema-drift gate passes on SQLite and Postgres. - **OIDC connection credentials**: connections may now carry `jwks_uri` and `oidc_issuer` in the credentials blob (no schema change). A connection *without* `jwks_uri` is plain OAuth2 and gets state-binding only; *with* `jwks_uri` gets full OIDC verification and must also set `oidc_issuer` (fail-closed otherwise). An id_token without `jwks_uri`, or `jwks_uri` configured but no id_token returned, both fail closed. Nonce binding is per-connection by configuration, not universal. - **SSE failover semantic shift**: `api.sse_revalidate_max_failures` (runtime-tunable streak counter) is replaced by `api.auth_revalidate_max_failures` / `api.auth_revalidate_window_seconds` (sliding window, shared with WS, `restart_required` + `read_only_post_init`). Operators no longer hot-tune SSE failover tolerance; resolved at startup. - **"429" in the acceptance line**: 429 `Retry-After` is per-policy and intentionally not coupled to the revalidation cadence. The unified cadence governs WS + SSE auth revalidation only. Documented in `docs/design/security.md`. - **Revocation window**: revocation takes effect within at most one revalidation interval (10 minutes), documented operationally in `docs/design/security.md`. ## Pre-PR review 17 review agents run (security, python, code, persistence, async-concurrency, api-contract, type-design, docs, logging, resilience, conventions, comment-quality, test-quality, silent-failure, infra, issue-resolution, audit mini-pass). 8 valid findings fixed (`6a888b2ee`, `2b5b91ce1`): 1. **Critical**: the LHCI `environment:` change as first written would have wedged every web/site PR (branch policy cannot match PR refs); reworked to the no-policy + fork-gate pattern. 2. JWKS client cache made atomic (`dict.setdefault`). 3. OIDC nonce binding documented in `docs/design/integrations.md`. 4. `oidc_verify` gained a module logger + a dedicated `OAUTH_OIDC_VERIFICATION_FAILED` forensic event on failure. 5. Added an SSE test proving an interleaved success does not reset the sliding-window failure budget. 6. Replaced an `assert` on the refresh security path with explicit fail-closed handling. 7. Clarified the per-connection-limiter docstring. 8. Documented the nullable `nonce` column in both `schema.sql`. Ratified no-action: the `except A, B:` PEP 758 cluster (FALSE_POSITIVE, valid Python 3.14, issue-dismissed); the consume-then-load ordering (intentional fail-closed); three pre-existing-not-a-bug items (WS cancel handling, flat-dict shallow copy, `handle_oauth_callback` length). `issue-resolution-verifier` confirmed all 8 acceptance criteria of #1917 RESOLVED. ## Verification - Full unit suite: 29194 passed, 0 failures (~114s, within baseline). - Dual-backend persistence conformance + schema-drift: clean on SQLite and Postgres (new `nonce` column round-trips). - `pre-commit run --all-files`: all hooks pass. - mypy strict: clean on every touched module.
Aureliolo
pushed a commit
that referenced
this pull request
May 17, 2026
<!-- HIGHLIGHTS_START --> ## Highlights > _AI-generated summary (model: `openai/gpt-4.1-mini` via GitHub Models). Commit-based changelog below._ ### What you'll notice - Frontend WP-6 update with UX polish improves user interface and workflow. - Dashboard and training endpoint improvements enhance observability and dispatch behavior. - Web storybook now supports change detection for more responsive UI interactions. - Git hooks now isolated per worktree for cleaner repository management. - Providers automatically detect native streaming support in Litellm models. ### What's new - Added a new pipeline to convert Pydantic DTOs to TypeScript for better front-end compatibility. ### Under the hood - Refactored settings to three precedence categories, removing YAML tier for simpler configuration. - Completed RootConfig mirror coverage for enhanced configuration consistency. - Adopted API conventions with better query performance and forbidden extra fields for stricter validation. - Improved persistence, layer discipline, and restart safety in core work packages. - CI updated with split test jobs and tightened coverage gates for better test quality. - Switched to direct Trivy binary for security scans, removing previous Trivy action dependency. - Enhanced memory management with per-call processing options and better observability during speech-to-text encoding. - Various dependency updates for Python, infrastructure, and lock files maintain security and stability. - Removed TypeScript DTO type-tightening overlays to simplify type management. - Codebase audit tightened skill sets to prevent false positivity in class detection by 2026. <!-- HIGHLIGHTS_END --> :robot: I have created a release *beep* *boop* --- ## [0.8.5](v0.8.4...v0.8.5) (2026-05-17) ### Features * **codegen:** pydantic-to-typescript DTO pipeline + parity gate (closes [#1889](#1889)) ([#1909](#1909)) ([0265ef5](0265ef5)) * **storybook:** enable changeDetection + trim web/CLAUDE.md ([#1939](#1939)) ([3b1f4c0](3b1f4c0)) * **web,setup:** WP-6 frontend + UX polish ([#1941](#1941)) ([d9ca76d](d9ca76d)) ### Bug Fixes * correct invalid git for-each-ref syntax in post-merge-cleanup skill ([#1946](#1946)) ([69a1649](69a1649)) * dashboard polish, training endpoint dispatch, and observability cleanup ([#1911](#1911)) ([b61e9e8](b61e9e8)) * per-worktree git-hook isolation + hookify gate migration + MSW drift fix ([#1949](#1949)) ([e3f8495](e3f8495)) * **providers:** read supports_native_streaming from litellm model info ([#1942](#1942)) ([60364ca](60364ca)) * security and audit coverage (closes [#1883](#1883)) ([#1904](#1904)) ([d8ebf55](d8ebf55)) ### Performance * **ci:** mypy --num-workers=4 + enable ruff TID255 ([#1944](#1944)) ([484c1d3](484c1d3)) ### Refactoring * **ci:** drop aquasecurity/trivy-action, use direct trivy binary ([#1940](#1940)) ([df1f946](df1f946)) * **memory:** per-call processing_kwargs + observability for ST encode ([#1943](#1943)) ([3aa9d20](3aa9d20)) * Phase 7 follow-up — complete RootConfig mirror coverage (closes [#1907](#1907)) ([#1914](#1914)) ([605500b](605500b)) * **settings:** collapse precedence to three categories; drop YAML tier (closes [#1890](#1890)) ([#1910](#1910)) ([efd54c9](efd54c9)) * WP-3 API conventions + query performance + project-wide extra=forbid ([#1953](#1953)) ([504d579](504d579)), closes [#1918](#1918) * WP-4 settings + cross-cutting (clock seam, contextvars, dispatch, plugin surfaces) ([#1954](#1954)) ([7207d92](7207d92)) * **wp1:** persistence + layer discipline + restart safety ([#1945](#1945)) ([57586fb](57586fb)) ### Documentation * **wp5:** public-facing truth refresh ([#1924](#1924)) ([afb5cc5](afb5cc5)) ### CI/CD * split test job by marker with airtight aggregate coverage gate ([#1948](#1948)) ([0b818d5](0b818d5)), closes [#1938](#1938) [#1937](#1937) ### Maintenance * **codebase-audit:** tighten skill to prevent 2026-05-15 FP classes ([#1923](#1923)) ([9317ed1](9317ed1)) * Lock file maintenance ([#1913](#1913)) ([c08a355](c08a355)) * Lock file maintenance ([#1950](#1950)) ([8940ab1](8940ab1)) * remove TS DTO type-tightening overlays ([#1915](#1915)) ([d296214](d296214)), closes [#1906](#1906) * Update Infrastructure dependencies ([#1928](#1928)) ([d19fae5](d19fae5)) * Update Python dependencies ([#1929](#1929)) ([75cc2c8](75cc2c8)) * **wp7:** hygiene, stubs, test/CI/tooling, doc gaps, boundary patterns doc ([#1926](#1926)) ([c29eb32](c29eb32)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: synthorg-repo-bot[bot] <279117679+synthorg-repo-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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 #1916.
What changed
WP-1 is the prerequisite work-package for WP-2/3/4/6: persistence-layer discipline, the four critical restart-safety gaps, and ADR-0001 (repository protocol consolidation by query pattern).
Critical restart-safety persistence
engine/workflow/ceremony_scheduler.py): hydrates_completion_counters/_fired_once_triggers/_total_completions/_velocity_historyon sprint activation; atomic save on every mutation.communication/meeting/scheduler.py): durable per-meeting-type cooldowns with hydration onstart().tools/sandbox/): every_tracked_containersmutation inDockerSandboxnow mirrors toTrackedContainerRepository(when wired); orphan reconciliation viasynthorg.managedlabel._store_locknow coversdelete/count/clear, matching the existingstore/retrievediscipline.Lifecycle correctness
JetStreamTaskQueue: dedicated_lifecycle_lockseparate from hot-path lock (prevents duplicate NATS consumer spawn under concurrentstart()).InMemoryMessageBus: split lifecycle and hot-path locks (no longer serialises normal traffic against lifecycle ops).JetStreamMessageBus: shutdown drain aligned withdocs/reference/lifecycle-sync.md(holds state lock through drain deadline; raisesBusStopTimeoutErroron timeout and marks state unrestartable).api/controllers/webhooks.py): wrapped inIdempotencyService.run_idempotent()so operator double-click no longer double-publishes.Layer discipline (REWORK #5)
api.dto_*toproviders.management.capability_dtos.scripts/check_no_api_dto_in_persistence_or_service.pyforbids the layer violation going forward; wired in.pre-commit-config.yamlat[pre-commit, pre-push].tests/unit/memory/org/conftest.py(migrations are the only schema source).REWORK #1: repository protocol consolidation (ADR-0001)
src/synthorg/persistence/_generics.py: six generic categories —SingletonRepository,IdKeyedRepository,FilteredQueryRepository,AppendOnlyRepository,StatefulRepository,MVCCRepository. All@runtime_checkable. CanonicalDEFAULT_PAGE_SIZE: Final[int] = 100.docs/decisions/0001-repository-protocol-consolidation.md) captures composition rules, the bespoke-method policy (D7), and the per-entity inventory.Task,Message,CostRecord,Approval,Project,User,Audit,WorkflowDefinition,Checkpoint,OrgFact,FineTuneRun,Settings,ProviderAudit,PresetOverride,Preset,AgentState,Artifact,Connection,CustomRule,McpInstallation,Subworkflow,TrainingPlan,TrainingResult,Session,OntologyEntity,OntologyDriftReport,WorkflowExecution,PersonalityPreset,Version,ParkedContext,PrincipleOverride,RiskOverride,CircuitBreakerState,Decision,SsrfViolation,LockoutRepository,RefreshToken,ConnectionSecret,FineTuneCheckpointcompose generics.CeremonySchedulerState,MeetingCooldown,TrackedContainer) each composeIdKeyedRepository[T, NotBlankStr]plus a D7load_allfor cold-start hydration. Listed as rows 46-48 in the ADR inventory.Wave-2 pre-PR triage (this commit on top of the WP-1 base)
TrackedContainerRepository(helper-method pattern; optional repo so tests stay zero-cost).tests/integration/persistence/and rewritten to do a realopen → write → disconnect → reopen → readcycle (the previous version re-used the same backend instance)._generics.pyinvariant docstrings: frozenTrequirement, ordering semantics divergence betweenFilteredQuery(concrete-documented) andAppendOnly(mandated newest-first),MVCCRepository.get_operation_logappend-order rule,StatefulRepository.transition_ifTypedDict guidance,SingletonRepositorysingle-row invariant,DEFAULT_PAGE_SIZEjustification consolidated.persistence/__init__.pyexports the three new repos + record types._rollback_quietlyhandlers (real exception viasafe_error_description, not bare"rollback failed").integrations/connections/catalog.py:124hardcodedlist_items(limit=10_000)carries a justifying comment +lint-allow.Test plan
uv run python -m pytest tests/ -m unit -n 8: 28 985 passed, 18 skipped (Windows + logfire-extra-not-installed skips only).uv run mypy src/ tests/: clean across 3 834 source files.uv run ruff check src/ tests/+ruff format --check: clean.tests/integration/persistence/test_wp1_restart_safety.py(SQLite arm, Postgres arm gated on Docker availability): 4 passed.dual-backend test parity,typed-boundary,cost-recording chokepoint,convention-gate inventory,no reviewer citations,no migration framing,forbid synthorg.api.dto_* imports).Review coverage
Two waves of pre-PR review:
7ae984b5e): 10 agents (docs-consistency, code-reviewer, python-reviewer, type-design-analyzer, persistence-reviewer, security-reviewer, async-concurrency-reviewer, api-contract-drift, frontend-reviewer, issue-resolution-verifier). All M/Md/Mn findings addressed in-PR.scripts/schema_drift_baseline.txtplaceholder text) is hookify-blocked and listed below for follow-up.Known follow-up (hookify-blocked from this session)
scripts/schema_drift_baseline.txtlines 30 / 86 / 144 carry the literal placeholderauto-generated; replace with audit-cited justification before commit. Functionally inert (the gate uses thecolumn:table:type:typesignature; the description is human prose only), but it should read like the other ~140 entries. The fix is to replace the placeholder withSQLite has no TIMESTAMPTZ; column stores TEXT carrying ISO-8601 with explicit +00:00/Z suffix, normalised to UTC at write timefor each of the three rows. The local edit is blocked byscripts/check_no_edit_baseline.sh, which requires an out-of-session step.