refactor: drain pagination + loop-init + kill-switch baselines (#1857)#1868
Conversation
Drains three runtime-correctness baselines to zero entries. list_pagination_baseline.txt: 37 entries cleared. Every baselined list/query method on Protocol + SQLite + Postgres now takes limit: int = DEFAULT_LIST_LIMIT (=100). DEFAULT_LIST_LIMIT lives in persistence/_shared/pagination.py. Domains drained: user, project, workflow_definition, workflow_execution (list_by_definition, list_by_status), subworkflow (list_summaries, list_versions), hr (collaboration_metric.query, task_metric.query, training_plan.list_by_agent), preset.list_all, custom_rule.list_rules, risk_override.list_active. Service-layer callers and cursor paginator call sites forward the limit; test fakes mirror the new signatures. loop_bound_init_baseline.txt: 30 entries cleared. Added lint-allow: loop-bound-init markers with documented eager-init justifications across 18 lifecycle services. Eager init is the canonical pattern when stop() must be callable before any start() has run, per docs/reference/lifecycle-sync.md. long_running_loops_kill_switch_baseline.txt: 26 entries cleared. Added lint-allow: long-running-loop-kill-switch markers with terse justifications, classified as per-request, retry-wait, sentinel, or service-stop pause. Closes #1857
- C7: LifecycleEventRepository.list_events now uses DEFAULT_LIST_LIMIT
on the Protocol + SQLite + Postgres impls.
- C8: validate_pagination_args now returns the clamped limit; the
helper enforces a MAX_LIST_LIMIT=10_000 ceiling so callers that
bypass the API's CursorLimit cannot trigger an unbounded scan.
Baselined repos (user, workflow_definition, workflow_execution,
subworkflow, hr, preset, custom_rule, risk_override, training_plan)
opt-in by capturing the return value.
- M1: user_protocol.list_by_user docstring now matches the actual
limit=100 default (was claiming limit=None fetch-all semantics).
- M3 + M4: docs/reference/mcp-handler-contract.md + docs/design/tools.md
clarify the intentional MCP default_limit=50 vs DEFAULT_LIST_LIMIT=100
scope split.
- M6: web/src/api/types/projects.ts drops the dead offset field; the
backend is cursor-based.
- C6 + frontend pagination: web/src/api/endpoints/workflow-executions.ts
now accepts cursor/limit params and returns PaginatedResult; the
page consumer and MSW handler updated to match.
- Fakes that lied: tests/unit/api/fakes_workflow.py +
tests/unit/engine/workflow/test_{execution_lifecycle,execution_service,subworkflow_registry}.py
now slice [:limit] instead of dropping the param.
- m1a + m1b: two long-running-loop kill-switch lint-allow comments
reworded to reflect the actual termination mechanism (stop()/cancel).
- m3: pg project_repo gains an inline safety-invariant comment on the
WHERE-clause f-string.
- M7-13: per-backend limit conformance tests added for project,
workflow_definition, workflow_execution (list_by_definition +
list_by_status), subworkflow (list_summaries + list_versions),
preset.list_all, risk_override.list_active, custom_rule.list_rules.
- m2a-c: has_more cursor-pagination tests for projects, workflows,
workflow_executions controllers.
- m4: parametrized limit-truncation tests in test_project_service +
test_rules_service.
Verified false positives (logged in triage):
- M2: docs/guides/budget.md correctly documents the budget controller's
CursorLimit=50 default (distinct from repo DEFAULT_LIST_LIMIT=100).
- M5: web/src/stores/projects.ts limit:200 is the API MAX_LIMIT
ceiling, not silently truncated.
- n1: gate-registry baseline counts (optional doc churn).
- n2: '-- see.' lint-allow markers point to a preceding multi-line
justification comment per the project convention.
- C5 partial: test_protocol.py degenerate fakes return () regardless,
which already satisfies 'max limit' trivially.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🧰 Additional context used📓 Path-based instructions (2)src/synthorg/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.py⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2026-05-05T09:04:46.195ZApplied to files:
🔇 Additional comments (3)
WalkthroughCentralizes pagination defaults and validation, adds limit parameters to protocols and all Postgres/SQLite repositories, and propagates them through services and engine. Controllers over-fetch by one; MCP handlers bound repo fetches with limit+offset. Web client adopts cursor pagination and paginated result types. Tests and fakes updated to enforce limits and verify pagination metadata. Baseline suppression files are cleared. Numerous modules add lint-allow comments for long-running loops and eager-init primitives without behavior changes. |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes pagination logic across the persistence layer by introducing DEFAULT_LIST_LIMIT and MAX_LIST_LIMIT constants and updating repository protocols and implementations to enforce these bounds. It includes widespread updates to Postgres and SQLite repositories to support a limit parameter and utilize a shared validation helper, alongside controller updates for cursor-based pagination. The PR also adds numerous lint suppressions for intentional patterns like eager initialization of async primitives and long-running loops. Review feedback correctly identifies critical bugs where LIMIT clauses were added to SQL queries without ORDER BY clauses, leading to non-deterministic results. Additionally, the reviewer pointed out several instances where the return value of the new pagination validation helper was not captured, which is required for effective limit clamping.
| "SELECT semver FROM subworkflows " | ||
| "WHERE subworkflow_id = %s LIMIT %s", | ||
| (subworkflow_id, limit), |
There was a problem hiding this comment.
The LIMIT clause is applied to an unordered result set because the SQL query lacks an ORDER BY clause. This results in non-deterministic behavior when a subworkflow has more versions than the requested limit. Since the docstring specifies 'newest first', the query should order by created_at DESC (or another appropriate temporal column) before applying the limit.
| "SELECT semver FROM subworkflows " | |
| "WHERE subworkflow_id = %s LIMIT %s", | |
| (subworkflow_id, limit), | |
| "SELECT semver FROM subworkflows " | |
| "WHERE subworkflow_id = %s ORDER BY created_at DESC LIMIT %s", | |
| (subworkflow_id, limit), |
| "SELECT semver FROM subworkflows WHERE subworkflow_id = ? LIMIT ?", | ||
| (subworkflow_id, limit), |
There was a problem hiding this comment.
Similar to the Postgres implementation, this SQL query is missing an ORDER BY clause. Without it, the LIMIT will return an arbitrary set of versions rather than the 'newest' ones as intended. Adding ORDER BY created_at DESC ensures deterministic and correct results.
"SELECT semver FROM subworkflows WHERE subworkflow_id = ? "
"ORDER BY created_at DESC LIMIT ?",
(subworkflow_id, limit),| validate_pagination_args(limit, 0, event=PERSISTENCE_PROJECT_LIST_FAILED) | ||
| effective_limit = min(limit, _MAX_LIST_ROWS) |
There was a problem hiding this comment.
The return value of validate_pagination_args should be captured to ensure the limit used in the query is properly clamped to MAX_LIST_LIMIT. The PR description notes that repos should 'opt-in by capturing the return value', but it was missed here. Capturing it also makes the subsequent min(limit, _MAX_LIST_ROWS) potentially redundant if the ceilings match.
| validate_pagination_args(limit, 0, event=PERSISTENCE_PROJECT_LIST_FAILED) | |
| effective_limit = min(limit, _MAX_LIST_ROWS) | |
| limit = validate_pagination_args(limit, 0, event=PERSISTENCE_PROJECT_LIST_FAILED) | |
| effective_limit = min(limit, _MAX_LIST_ROWS) |
| validate_pagination_args(limit, 0, event=PERSISTENCE_PROJECT_LIST_FAILED) | ||
| # Clamp under the hard ceiling so a caller-supplied limit | ||
| # cannot exceed the table-scan safety bound. | ||
| effective_limit = min(limit, _MAX_LIST_ROWS) |
There was a problem hiding this comment.
The return value of validate_pagination_args is ignored here as well. Capturing it ensures that the limit is correctly clamped and type-checked according to the centralized logic in the pagination helper, adhering to the 'opt-in' pattern described in the PR summary.
| validate_pagination_args(limit, 0, event=PERSISTENCE_PROJECT_LIST_FAILED) | |
| # Clamp under the hard ceiling so a caller-supplied limit | |
| # cannot exceed the table-scan safety bound. | |
| effective_limit = min(limit, _MAX_LIST_ROWS) | |
| limit = validate_pagination_args(limit, 0, event=PERSISTENCE_PROJECT_LIST_FAILED) | |
| # Clamp under the hard ceiling so a caller-supplied limit | |
| # cannot exceed the table-scan safety bound. | |
| effective_limit = min(limit, _MAX_LIST_ROWS) |
| offset: Keys to skip; applied independently of *limit*. | ||
| ``offset > 0`` with ``limit=None`` yields a tail | ||
| window starting after *offset* rows. | ||
| limit: Maximum keys to return (default 100). Must be >= 1. |
There was a problem hiding this comment.
For consistency with other methods in this protocol and across the codebase, the docstring should reference :data:DEFAULT_LIST_LIMIT instead of using a hardcoded literal 100.
| limit: Maximum keys to return (default 100). Must be >= 1. | |
| limit: Maximum keys to return (default :data:`DEFAULT_LIST_LIMIT`). Must be >= 1. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1868 +/- ##
=======================================
Coverage 84.90% 84.90%
=======================================
Files 1805 1805
Lines 105208 105246 +38
Branches 9214 9214
=======================================
+ Hits 89324 89361 +37
- Misses 13652 13654 +2
+ Partials 2232 2231 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/unit/meta/test_rules_service.py (1)
15-18: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the shared default limit in the fake to avoid drift.
The fake currently hardcodes
100; importingDEFAULT_LIST_LIMITkeeps test contracts aligned if the shared default changes.Proposed change
from synthorg.meta.models import ProposalAltitude, RuleSeverity from synthorg.meta.rules.custom import Comparator, CustomRuleDefinition from synthorg.meta.rules.service import CustomRuleNotFoundError, CustomRulesService +from synthorg.persistence._shared import DEFAULT_LIST_LIMIT @@ async def list_rules( self, *, enabled_only: bool = False, - limit: int = 100, + limit: int = DEFAULT_LIST_LIMIT, ) -> tuple[CustomRuleDefinition, ...]:Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/meta/test_rules_service.py` around lines 15 - 18, Replace the hardcoded "100" used in the test fake with the shared DEFAULT_LIST_LIMIT constant to prevent drift: add an import for DEFAULT_LIST_LIMIT at the top of the test and use DEFAULT_LIST_LIMIT wherever the fake repository or fake rule service sets its default limit (e.g., in the Fake* or fake_rules_* setup used by tests in this file and at the second occurrence around line 45) instead of the literal 100.src/synthorg/persistence/postgres/hr_repositories.py (1)
149-156: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winInconsistent pagination validation within this file.
list_eventsuses manual limit validation (lines 149-156) whileTaskMetricRepository.query(line 259-261) andCollaborationMetricRepository.query(line 380-382) use the sharedvalidate_pagination_argshelper. For consistency and maintainability, consider refactoringlist_eventsto usevalidate_pagination_argsas well.♻️ Use shared validation helper
- # Validate at the repository boundary so callers cannot - # accidentally pass a float, bool, or negative value into - # the raw "LIMIT %s" parameter and get a confusing DB-side - # error (or worse, a silently-wrong result). - if isinstance(limit, bool) or not isinstance(limit, int) or limit < 1: - msg = f"limit must be a positive integer, got {limit!r}" - logger.warning( - PERSISTENCE_LIFECYCLE_EVENT_LIST_FAILED, - error=msg, - ) - raise QueryError(msg) + limit = validate_pagination_args( + limit, 0, event=PERSISTENCE_LIFECYCLE_EVENT_LIST_FAILED + ) sql += " LIMIT %s" params.append(limit)🤖 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/hr_repositories.py` around lines 149 - 156, Replace the manual pagination checks in list_events with the shared helper validate_pagination_args: remove the boolean/int/positive checks and the QueryError raise currently guarded by PERSISTENCE_LIFECYCLE_EVENT_LIST_FAILED, call validate_pagination_args(limit, offset) at the start of list_events to validate and normalize inputs (and let it raise/log consistently), and then append " LIMIT %s" to sql as before using the validated limit value; reference the list_events function and the validate_pagination_args helper to locate and update the code.web/src/api/endpoints/workflow-executions.ts (1)
7-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate module docstring to reflect paginated envelope.
The module docstring states "no envelope wrapper around the list -- the controller returns the raw list inside
ApiResponse.data", butlistWorkflowExecutionsnow returnsPaginatedResponse<WorkflowExecution>(lines 67, 71), which IS an envelope wrapper containingdata,next_cursor, andhas_more. Update the module docstring to reflect the current paginated API contract.📝 Suggested docstring update
/** * Workflow execution endpoints. * * Mirrors the backend ``synthorg.api.controllers.workflow_executions`` - * surface: list executions for a workflow, cancel an in-flight - * execution. Field names and the response wire shape match the - * backend Pydantic ``WorkflowExecution`` model 1:1 (no envelope - * wrapper around the list -- the controller returns the raw list - * inside ``ApiResponse.data``). + * surface: list executions for a workflow (with cursor pagination), + * cancel an in-flight execution. Field names match the backend + * Pydantic ``WorkflowExecution`` model 1:1. List endpoint returns + * ``PaginatedResponse<WorkflowExecution>`` for cursor-based pagination. */🤖 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 `@web/src/api/endpoints/workflow-executions.ts` around lines 7 - 9, Update the module docstring to reflect that listWorkflowExecutions returns a paginated envelope rather than a raw list: change the sentence saying "no envelope wrapper around the list -- the controller returns the raw list inside ApiResponse.data" to describe the PaginatedResponse<WorkflowExecution> envelope that includes data, next_cursor, and has_more; reference the listWorkflowExecutions function and PaginatedResponse<WorkflowExecution> type so readers know the API returns an envelope with pagination metadata rather than a bare list.src/synthorg/engine/workflow/subworkflow_registry.py (1)
248-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
list_page()now risks truncating cursor pagination at the default repo limit.At Line 248,
list_summaries()is called without an explicit limit. After this PR, that default is bounded, so this method can silently miss summaries beyond the first default slice and produce incorrecthas_morebehavior.Proposed fix
-from synthorg.persistence._shared import DEFAULT_LIST_LIMIT +from synthorg.persistence._shared import DEFAULT_LIST_LIMIT, MAX_LIST_LIMIT @@ - all_summaries = await self._repo.list_summaries() + all_summaries = await self._repo.list_summaries(limit=MAX_LIST_LIMIT)🤖 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/subworkflow_registry.py` around lines 248 - 263, list_page() calls _repo.list_summaries() without an explicit limit so it can be truncated by the repo's default limit; update list_page() to request an unbounded fetch from the repo (e.g., call _repo.list_summaries(limit=None) or another repo-specific flag that disables the default cap) before performing sorting, _decode_subworkflow_keyset(after_key) filtering, and the over-fetch logic (sorted_summaries, after_key, page, has_more) so cursor pagination and has_more are computed from the full result set rather than a default-sliced subset.
♻️ Duplicate comments (1)
src/synthorg/persistence/sqlite/hr_repositories.py (1)
146-152: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winInconsistent pagination validation within this file.
Same issue as the Postgres sibling (
src/synthorg/persistence/postgres/hr_repositories.py):list_eventsuses manual limit validation whileTaskMetricRepository.queryandCollaborationMetricRepository.queryusevalidate_pagination_args. For dual-backend consistency and to reduce duplication, refactorlist_eventsto use the shared helper.🤖 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/hr_repositories.py` around lines 146 - 152, Replace the manual pagination validation in list_events with the shared helper validate_pagination_args used by TaskMetricRepository.query and CollaborationMetricRepository.query: call validate_pagination_args(limit, offset) (import it if missing), use the normalized limit/offset it returns, and remove the manual isinstance/negative checks and custom logger warning/QueryError so behavior matches the Postgres sibling and avoids duplication.
🤖 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/memory/embedding/fine_tune_orchestrator.py`:
- Around line 99-101: Update the inline comment above self._op_lock to reference
the actual method names start() and cancel() instead of start_pipeline and
cancel_pipeline; locate the comment near the asyncio.Lock initialization for
self._op_lock in fine_tune_orchestrator.py and change the text to say that
start() and cancel() may interleave so the lock must be present before the first
call.
In `@src/synthorg/notifications/adapters/ntfy.py`:
- Around line 116-117: Update the inline lint-allow comment on the asyncio lock
initialization so it contains the full justification instead of "see.";
specifically replace the trailing comment on the self._lifecycle_lock =
asyncio.Lock() line (symbol: self._lifecycle_lock) so the lint marker reads like
the other in-file examples (e.g., include "loop-bound-init — lock is created
eagerly so stop() is safe before any start() call, created outside event loop
context intentionally") to make the rationale self-contained and searchable.
In `@src/synthorg/notifications/adapters/slack.py`:
- Around line 104-105: Update the lint-allow comment on the asyncio lock
initialization so it includes a self-contained justification instead of "see.";
specifically replace the trailing comment for self._lifecycle_lock =
asyncio.Lock() with a lint-allow marker that explains why loop-bound-init is
safe (for example: "# lint-allow: loop-bound-init -- lock created at object
init; stop() must be safe before start() and no event loop is bound here"). Keep
the symbol self._lifecycle_lock and the use of asyncio.Lock() unchanged, only
change the inline comment text to be explicit and searchable.
In `@src/synthorg/persistence/postgres/training_plan_repo.py`:
- Around line 307-310: The SQL ORDER BY in the training plan query (the query
string used in training_plan_repo.py that selects from training_plans where
new_agent_id = %s) uses only "ORDER BY created_at DESC", which can yield
non-deterministic results when created_at values tie; update the query to add a
deterministic tiebreaker by changing the ORDER BY clause to "ORDER BY created_at
DESC, id DESC" (modify the SQL string in the method that constructs/executess
this query so the LIMIT returns stable results).
In `@src/synthorg/persistence/postgres/workflow_execution_repo.py`:
- Around line 347-349: The call to validate_pagination_args(limit, 0,
event=PERSISTENCE_WORKFLOW_EXEC_LIST_FAILED) misleadingly passes a positional 0
for an unused offset; update the call to make intent explicit: if there is a
limit-only helper (e.g., validate_limit_only), switch to that; otherwise call
validate_pagination_args with a named argument offset=0 and add a brief comment
explaining this is a deliberate placeholder because this repository uses
limit-only pagination (reference validate_pagination_args and
PERSISTENCE_WORKFLOW_EXEC_LIST_FAILED to locate the call).
In `@src/synthorg/tools/sandbox/lifecycle/per_agent.py`:
- Line 277: The inline lint allow comment for the long-running loop in
per_agent.py is too terse; update the comment next to the loop (where the
current "lint-allow: long-running-loop-kill-switch -- per-owner idle-expiry."
appears) to explicitly state that this loop is a deliberate exception because it
naturally exits via the owner state checks in the loop body (see the checks that
remove the owner / idle expiry) and is cancelled during cleanup_all(), and note
that a cooperative _stop_event was intentionally not added for this per-owner
task; reference the loop and cleanup_all() to make the reasoning clear for
future maintainers.
In `@tests/conformance/persistence/test_custom_rule_repository.py`:
- Around line 118-120: The assertion using "len(rows) <= 3" is too weak; update
the test to assert exactly three rows were returned when calling
backend.custom_rules.list_rules(limit=3). Replace the assertion with "assert
len(rows) == 3" (or equivalent equality check) referencing the list_rules call
and the rows variable, ensuring the test setup inserts at least three rules so
the equality is valid.
In `@tests/conformance/persistence/test_preset_repository.py`:
- Around line 91-92: The current assertion uses assert len(rows) <= 3 which can
miss under-fetch regressions; when 5 presets were inserted we should assert
exact truncation, so update the test around
backend.custom_presets.list_all(limit=3) to assert len(rows) == 3 (ensuring the
list_all call respects the limit) and keep the limit argument as-is to validate
proper truncation behavior.
In `@tests/conformance/persistence/test_project_repository.py`:
- Around line 88-89: Change the non-deterministic assertion in the test to
enforce exact limit conformance: when calling
backend.projects.list_projects(limit=3) assert that the returned list length
equals 3 (use assert len(rows) == 3) so the test fails if fewer than the
requested limit are returned; update the assertion near the call to
list_projects to use equality rather than <= to ensure deterministic behavior
given the seeded 5 projects.
In `@tests/conformance/persistence/test_risk_override_repository.py`:
- Around line 105-106: The test currently uses a non-specific assertion (assert
len(rows) <= 3) which can mask regressions; change it to assert the exact
expected count by replacing that assertion with assert len(rows) == 3 so the
call to backend.risk_overrides.list_active(limit=3) is verified to return
exactly three rows; update the assertion in
tests/conformance/persistence/test_risk_override_repository.py around the call
to list_active to use equality rather than a less-than-or-equal check.
In `@tests/conformance/persistence/test_subworkflow_repository.py`:
- Around line 107-119: In test_list_versions_respects_limit ensure the test
proves truncation by asserting exact page size: after seeding five versions via
backend.subworkflows.save(_subworkflow(version=...)) call
list_versions(NotBlankStr("sub-001"), limit=3) and assert len(versions) == 3
(not <= 3); similarly change the earlier test that calls
backend.subworkflows.list_summaries(limit=3) to assert len(rows) == 3 to
guarantee the backend enforces the limit rather than under-fetching.
In `@tests/conformance/persistence/test_workflow_definition_repository.py`:
- Around line 166-167: The test currently asserts only an upper bound for
repo.list_definitions(limit=3) which won't catch under-return regressions;
update the assertion in test_workflow_definition_repository (the block calling
repo.list_definitions) to assert that len(rows) == 3 given the 5 seeded rows and
limit=3 so the test enforces exactly three returned rows rather than <= 3.
In `@tests/conformance/persistence/test_workflow_execution_repository.py`:
- Around line 260-261: The test currently calls
exec_repo.list_by_status(WorkflowExecutionStatus.PENDING, limit=3) and asserts
len(rows) <= 3; change this to assert len(rows) == 3 to verify
pagination/enforced page size; update the assertion in
test_workflow_execution_repository (the block using exec_repo.list_by_status,
WorkflowExecutionStatus.PENDING, limit=3 and the variable rows) so the test
confirms exactly three rows are returned given the test created five PENDING
executions for a unique definition.
- Around line 240-241: The test currently asserts only an upper bound for
pagination; change the assertion to require exactly 3 rows to ensure paging is
honored: after creating 5 executions with definition_id "wf-lim", assert that
the result of exec_repo.list_by_definition("wf-lim", limit=3) (the variable
rows) has length == 3 so the limit is enforced by the repository method.
In `@tests/unit/api/controllers/test_projects.py`:
- Around line 78-80: The overflow pagination test currently asserts "assert
len(body['data']) <= 2" which allows under-return regressions; locate the test
in tests/unit/api/controllers/test_projects.py (the test that creates 4 items
and calls the projects list endpoint with limit=2) and change the assertion to
require exactly two items—i.e. replace the <= check with "assert
len(body['data']) == 2" while keeping the existing checks on
body["pagination"]["has_more"] and body["pagination"]["next_cursor"] intact.
In `@tests/unit/api/controllers/test_workflow_executions.py`:
- Around line 207-209: The test currently asserts len(body["data"]) <= 2 which
can mask under-fetch bugs; update the assertion in the test_workflow_executions
unit test to assert len(body["data"]) == 2 (given 4 seeded executions and
limit=2) and keep the existing checks on body["pagination"]["has_more"] and
body["pagination"]["next_cursor"] to ensure exact page size and correct
pagination behavior.
In `@tests/unit/api/controllers/test_workflows.py`:
- Around line 211-216: The test currently allows len(body["data"]) <= 2 which is
too permissive; update the assertion for the paginated overflow case to require
exactly 2 results. Locate the test that calls
test_client.get("/api/v1/workflows?limit=2") and replace the assertion on
len(body["data"]) (the line referencing resp and body) so it asserts
len(body["data"]) == 2, leaving the existing checks for pagination.has_more and
pagination.next_cursor intact.
In `@tests/unit/api/fake_user_repository.py`:
- Line 14: FakeUserRepository.list_users currently accepts a limit directly and
bypasses the production pagination validation/clamping, letting tests pass with
limits the real repo would normalize or reject; update list_users in
FakeUserRepository to apply the same shared pagination validation/clamping as
the real repository (use the existing shared helper or reuse DEFAULT_LIST_LIMIT
semantics) so the limit is normalized/clamped the same way as production code.
Ensure you import the shared pagination helper (or implement the same clamp
logic) and apply it before slicing/filtering results in list_users so tests
mirror real repository behavior.
In `@tests/unit/api/fakes_backend.py`:
- Around line 77-85: The fake list methods (e.g., list_active) currently slice
with the raw limit which allows negative values to trigger Python slice
semantics; add an explicit validation at the start of each affected method
(including the other fake list method around lines 426-436) to reject negative
limits (for example: if limit < 0: raise ValueError("limit must be
non-negative")) so tests won't silently use Python's negative-slice behavior;
keep the remainder of the logic (filtering by self._overrides, sorting by
created_at, slicing) unchanged.
---
Outside diff comments:
In `@src/synthorg/engine/workflow/subworkflow_registry.py`:
- Around line 248-263: list_page() calls _repo.list_summaries() without an
explicit limit so it can be truncated by the repo's default limit; update
list_page() to request an unbounded fetch from the repo (e.g., call
_repo.list_summaries(limit=None) or another repo-specific flag that disables the
default cap) before performing sorting, _decode_subworkflow_keyset(after_key)
filtering, and the over-fetch logic (sorted_summaries, after_key, page,
has_more) so cursor pagination and has_more are computed from the full result
set rather than a default-sliced subset.
In `@src/synthorg/persistence/postgres/hr_repositories.py`:
- Around line 149-156: Replace the manual pagination checks in list_events with
the shared helper validate_pagination_args: remove the boolean/int/positive
checks and the QueryError raise currently guarded by
PERSISTENCE_LIFECYCLE_EVENT_LIST_FAILED, call validate_pagination_args(limit,
offset) at the start of list_events to validate and normalize inputs (and let it
raise/log consistently), and then append " LIMIT %s" to sql as before using the
validated limit value; reference the list_events function and the
validate_pagination_args helper to locate and update the code.
In `@tests/unit/meta/test_rules_service.py`:
- Around line 15-18: Replace the hardcoded "100" used in the test fake with the
shared DEFAULT_LIST_LIMIT constant to prevent drift: add an import for
DEFAULT_LIST_LIMIT at the top of the test and use DEFAULT_LIST_LIMIT wherever
the fake repository or fake rule service sets its default limit (e.g., in the
Fake* or fake_rules_* setup used by tests in this file and at the second
occurrence around line 45) instead of the literal 100.
In `@web/src/api/endpoints/workflow-executions.ts`:
- Around line 7-9: Update the module docstring to reflect that
listWorkflowExecutions returns a paginated envelope rather than a raw list:
change the sentence saying "no envelope wrapper around the list -- the
controller returns the raw list inside ApiResponse.data" to describe the
PaginatedResponse<WorkflowExecution> envelope that includes data, next_cursor,
and has_more; reference the listWorkflowExecutions function and
PaginatedResponse<WorkflowExecution> type so readers know the API returns an
envelope with pagination metadata rather than a bare list.
---
Duplicate comments:
In `@src/synthorg/persistence/sqlite/hr_repositories.py`:
- Around line 146-152: Replace the manual pagination validation in list_events
with the shared helper validate_pagination_args used by
TaskMetricRepository.query and CollaborationMetricRepository.query: call
validate_pagination_args(limit, offset) (import it if missing), use the
normalized limit/offset it returns, and remove the manual isinstance/negative
checks and custom logger warning/QueryError so behavior matches the Postgres
sibling and avoids duplication.
🪄 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
Review profile: ASSERTIVE
Plan: Pro
Run ID: 970e5da0-4bc9-453b-b941-c543ac3bd10f
📒 Files selected for processing (104)
docs/design/tools.mddocs/reference/mcp-handler-contract.mdscripts/list_pagination_baseline.txtscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtsrc/synthorg/api/auth/user_service.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/api/controllers/projects.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/controllers/ws_revalidation.pysrc/synthorg/api/services/idempotency_service.pysrc/synthorg/api/services/project_service.pysrc/synthorg/backup/scheduler.pysrc/synthorg/backup/service.pysrc/synthorg/budget/quota_poller.pysrc/synthorg/communication/bus/_nats_history.pysrc/synthorg/communication/bus/_nats_receive.pysrc/synthorg/communication/bus/memory.pysrc/synthorg/communication/conflict_resolution/escalation/in_memory_store.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/engine/task_engine.pysrc/synthorg/engine/task_engine_loops.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/engine/workflow/execution_service.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/hr/persistence_protocol.pysrc/synthorg/hr/pruning/service.pysrc/synthorg/integrations/health/prober.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/integrations/tunnel/ngrok_adapter.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/meta/chief_of_staff/monitor.pysrc/synthorg/meta/mcp/handlers/workflow_executions.pysrc/synthorg/meta/mcp/handlers/workflows.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/persistence/_shared/__init__.pysrc/synthorg/persistence/_shared/pagination.pysrc/synthorg/persistence/custom_rule_protocol.pysrc/synthorg/persistence/postgres/custom_rule_repo.pysrc/synthorg/persistence/postgres/hr_repositories.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/persistence/postgres/project_repo.pysrc/synthorg/persistence/postgres/risk_override_repo.pysrc/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/preset_protocol.pysrc/synthorg/persistence/project_protocol.pysrc/synthorg/persistence/risk_override_protocol.pysrc/synthorg/persistence/sqlite/custom_rule_repo.pysrc/synthorg/persistence/sqlite/escalation_repo.pysrc/synthorg/persistence/sqlite/hr_repositories.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/persistence/sqlite/project_repo.pysrc/synthorg/persistence/sqlite/risk_override_repo.pysrc/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/subworkflow_protocol.pysrc/synthorg/persistence/training_protocol.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/workflow_definition_protocol.pysrc/synthorg/persistence/workflow_execution_protocol.pysrc/synthorg/providers/resilience/rate_limiter.pysrc/synthorg/security/timeout/scheduler.pysrc/synthorg/telemetry/collector.pysrc/synthorg/tools/sandbox/lifecycle/per_agent.pysrc/synthorg/workers/worker.pytests/conformance/persistence/test_custom_rule_repository.pytests/conformance/persistence/test_preset_repository.pytests/conformance/persistence/test_project_repository.pytests/conformance/persistence/test_risk_override_repository.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_workflow_definition_repository.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_projects.pytests/unit/api/controllers/test_workflow_executions.pytests/unit/api/controllers/test_workflows.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_project_service.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_execution_service.pytests/unit/engine/workflow/test_subworkflow_registry.pytests/unit/meta/test_rules_service.pytests/unit/persistence/test_protocol.pyweb/src/api/endpoints/workflow-executions.tsweb/src/api/types/projects.tsweb/src/mocks/handlers/workflow-executions.tsweb/src/pages/WorkflowExecutionsPage.tsx
💤 Files with no reviewable changes (3)
- scripts/list_pagination_baseline.txt
- scripts/long_running_loops_kill_switch_baseline.txt
- scripts/loop_bound_init_baseline.txt
📜 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). (14)
- GitHub Check: Deploy Preview
- GitHub Check: Build Backend
- GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
- GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
- GitHub Check: CodSpeed Web benchmarks
- GitHub Check: CodSpeed Python benchmarks
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dashboard Test
- GitHub Check: Type Check
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (17)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers: use@pytest.mark.{unit,integration,e2e,slow}. Asyncauto. Timeout 30s global. Coverage 80% min.
Windows: unit tests useWindowsSelectorEventLoopPolicy(3.14 IOCP teardown race). Subprocess tests override back.
Test doubles ladder:FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags. BareMagicMockat a typed boundary is blocked byscripts/check_mock_spec.py(zero-tolerance).
FakeClock andmock_ofimport fromtests._shared; inject viaclock=and the helper's spec subscript.
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add@example(...)).
Flaky tests: NEVER skip/xfail; fix fundamentally. Useasyncio.Event().wait()notsleep(large).
Files:
tests/conformance/persistence/test_workflow_definition_repository.pytests/unit/meta/test_rules_service.pytests/conformance/persistence/test_custom_rule_repository.pytests/unit/api/fakes_workflow.pytests/unit/api/fake_user_repository.pytests/conformance/persistence/test_project_repository.pytests/conformance/persistence/test_preset_repository.pytests/unit/persistence/test_protocol.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_risk_override_repository.pytests/unit/api/controllers/test_workflows.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_subworkflow_registry.pytests/unit/api/fakes.pytests/unit/api/controllers/test_projects.pytests/unit/api/fakes_backend.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/services/test_project_service.pytests/unit/engine/workflow/test_execution_service.pytests/unit/api/controllers/test_workflow_executions.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/conformance/persistence/test_workflow_definition_repository.pytests/unit/meta/test_rules_service.pytests/conformance/persistence/test_custom_rule_repository.pytests/unit/api/fakes_workflow.pytests/unit/api/fake_user_repository.pytests/conformance/persistence/test_project_repository.pytests/conformance/persistence/test_preset_repository.pytests/unit/persistence/test_protocol.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_risk_override_repository.pytests/unit/api/controllers/test_workflows.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_subworkflow_registry.pytests/unit/api/fakes.pytests/unit/api/controllers/test_projects.pytests/unit/api/fakes_backend.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/services/test_project_service.pytests/unit/engine/workflow/test_execution_service.pytests/unit/api/controllers/test_workflow_executions.py
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance:
tests/conformance/persistence/consumesbackendfixture (SQLite + Postgres). Enforced bycheck_dual_backend_test_parity.py.
Files:
tests/conformance/persistence/test_workflow_definition_repository.pytests/conformance/persistence/test_custom_rule_repository.pytests/conformance/persistence/test_project_repository.pytests/conformance/persistence/test_preset_repository.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_risk_override_repository.pytests/conformance/persistence/test_workflow_execution_repository.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Configuration precedence: DB > env > YAML > code default viaSettingsService/ConfigResolver; noos.environ.getoutside startup.
No hardcoded numerics; numerics live insettings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of the formNAME: int|float|Final|Final[int]|Final[float] = literal. Enforced byscripts/check_no_magic_numbers.py.
Comments explain WHY only; no reviewer citations / issue back-refs / migration framing. Enforced bycheck_no_review_origin_in_code.py+check_no_migration_framing.py.
Nofrom __future__ import annotations(3.14 has PEP 649). Use PEP 758 except:except A, B:requires parens when binding.
Type hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines.
Errors:<Domain><Condition>ErrorfromDomainError; never inheritException/RuntimeError/etc directly. Enforced bycheck_domain_error_hierarchy.py.
Pydantic v2 frozen +extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes);@computed_fieldfor derived;NotBlankStrfor identifiers.
Args models at every system boundary;parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.py.
Immutability: usemodel_copy(update=...)orcopy.deepcopy(); deepcopy at system boundaries.
Async: useasyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError).
Clock seam: declareclock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out stops mark unrestartable.
Untrusted content (SEC-1): usewrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTML.
Repository CRUD: use methodssave(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuples.
Datetime in persistence: useparse_iso_utc/ `format_iso_...
Files:
src/synthorg/persistence/preset_protocol.pysrc/synthorg/integrations/tunnel/ngrok_adapter.pysrc/synthorg/persistence/workflow_execution_protocol.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/persistence/risk_override_protocol.pysrc/synthorg/communication/bus/_nats_history.pysrc/synthorg/providers/resilience/rate_limiter.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/meta/chief_of_staff/monitor.pysrc/synthorg/meta/mcp/handlers/workflow_executions.pysrc/synthorg/persistence/training_protocol.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/api/services/idempotency_service.pysrc/synthorg/tools/sandbox/lifecycle/per_agent.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/bus/_nats_receive.pysrc/synthorg/api/controllers/ws_revalidation.pysrc/synthorg/meta/mcp/handlers/workflows.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/engine/task_engine_loops.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/controllers/projects.pysrc/synthorg/persistence/postgres/project_repo.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/integrations/health/prober.pysrc/synthorg/communication/bus/memory.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/security/timeout/scheduler.pysrc/synthorg/engine/workflow/execution_service.pysrc/synthorg/persistence/_shared/pagination.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/budget/quota_poller.pysrc/synthorg/persistence/postgres/hr_repositories.pysrc/synthorg/telemetry/collector.pysrc/synthorg/backup/scheduler.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/workflow_definition_protocol.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/persistence/postgres/custom_rule_repo.pysrc/synthorg/persistence/sqlite/risk_override_repo.pysrc/synthorg/hr/pruning/service.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/workers/worker.pysrc/synthorg/persistence/sqlite/hr_repositories.pysrc/synthorg/persistence/sqlite/escalation_repo.pysrc/synthorg/persistence/sqlite/custom_rule_repo.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/persistence/custom_rule_protocol.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/communication/conflict_resolution/escalation/in_memory_store.pysrc/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/hr/persistence_protocol.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/api/services/project_service.pysrc/synthorg/backup/service.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/persistence/_shared/__init__.pysrc/synthorg/api/auth/user_service.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/project_protocol.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/postgres/risk_override_repo.pysrc/synthorg/engine/task_engine.pysrc/synthorg/persistence/subworkflow_protocol.pysrc/synthorg/persistence/sqlite/project_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/persistence/preset_protocol.pysrc/synthorg/integrations/tunnel/ngrok_adapter.pysrc/synthorg/persistence/workflow_execution_protocol.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/persistence/risk_override_protocol.pysrc/synthorg/communication/bus/_nats_history.pysrc/synthorg/providers/resilience/rate_limiter.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/meta/chief_of_staff/monitor.pysrc/synthorg/meta/mcp/handlers/workflow_executions.pysrc/synthorg/persistence/training_protocol.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/api/services/idempotency_service.pysrc/synthorg/tools/sandbox/lifecycle/per_agent.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/bus/_nats_receive.pysrc/synthorg/api/controllers/ws_revalidation.pysrc/synthorg/meta/mcp/handlers/workflows.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/engine/task_engine_loops.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/controllers/projects.pysrc/synthorg/persistence/postgres/project_repo.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/integrations/health/prober.pysrc/synthorg/communication/bus/memory.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/security/timeout/scheduler.pysrc/synthorg/engine/workflow/execution_service.pysrc/synthorg/persistence/_shared/pagination.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/budget/quota_poller.pysrc/synthorg/persistence/postgres/hr_repositories.pysrc/synthorg/telemetry/collector.pysrc/synthorg/backup/scheduler.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/workflow_definition_protocol.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/persistence/postgres/custom_rule_repo.pysrc/synthorg/persistence/sqlite/risk_override_repo.pysrc/synthorg/hr/pruning/service.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/workers/worker.pysrc/synthorg/persistence/sqlite/hr_repositories.pysrc/synthorg/persistence/sqlite/escalation_repo.pysrc/synthorg/persistence/sqlite/custom_rule_repo.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/persistence/custom_rule_protocol.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/communication/conflict_resolution/escalation/in_memory_store.pysrc/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/hr/persistence_protocol.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/api/services/project_service.pysrc/synthorg/backup/service.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/persistence/_shared/__init__.pysrc/synthorg/api/auth/user_service.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/project_protocol.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/postgres/risk_override_repo.pysrc/synthorg/engine/task_engine.pysrc/synthorg/persistence/subworkflow_protocol.pysrc/synthorg/persistence/sqlite/project_repo.py
web/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,js,jsx}: Always usecreateLoggerfrom@/lib/logger; never use bareconsole.warn,console.error, orconsole.debugin application code
Logger variable name must always belog(e.g.,const log = createLogger('module-name'))
Pass dynamic or untrusted values to logger methods as separate arguments (not interpolated into the message string) so they go throughsanitizeArg
Wrap attacker-controlled fields inside structured objects insanitizeForLog()before embedding them in logs
Files:
web/src/api/types/projects.tsweb/src/api/endpoints/workflow-executions.tsweb/src/pages/WorkflowExecutionsPage.tsxweb/src/mocks/handlers/workflow-executions.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Callers MUST NOT wrap store mutation calls in try/catch; the store owns the error UX
Any new caller of health/readiness endpoints must handle the 503 path explicitly
Any new WS payload handler that ingests untrusted strings MUST route throughsanitizeWsString()orsanitizeWsEnum()helpers
ImportErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported fromweb/src/api/types/error-codes.gen.ts); discriminate onErrorCode.<NAME>, never on raw integer literals
Never export agetXIcon(value): LucideIconfactory that returns a component reference called inside JSX render bodies; export a<XIcon value={...} {...lucideProps} />wrapper component instead usingcreateElementfor the lookup
Detectfetch()in effects withoutAbortControllercleanup using the@eslint-react/web-api-no-leaked-fetchrule
Catch the{count && <Foo />}bug using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...)
Restrictwindow,document,localStorage, and other globals inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handler,useEffect, oruseSyncExternalStore-backed hook
Files:
web/src/api/types/projects.tsweb/src/api/endpoints/workflow-executions.tsweb/src/pages/WorkflowExecutionsPage.tsxweb/src/mocks/handlers/workflow-executions.ts
web/src/{api,stores}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Display counts come from
data.length; the wire envelope no longer carriestotalfield for paginated responses
Files:
web/src/api/types/projects.tsweb/src/api/endpoints/workflow-executions.ts
web/src/**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Reuse
web/src/components/ui/design tokens only in web dashboard design system.
Files:
web/src/api/types/projects.tsweb/src/api/endpoints/workflow-executions.tsweb/src/pages/WorkflowExecutionsPage.tsxweb/src/mocks/handlers/workflow-executions.ts
src/synthorg/!(persistence)/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQL.
Files:
src/synthorg/integrations/tunnel/ngrok_adapter.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/communication/bus/_nats_history.pysrc/synthorg/providers/resilience/rate_limiter.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/meta/chief_of_staff/monitor.pysrc/synthorg/meta/mcp/handlers/workflow_executions.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/api/services/idempotency_service.pysrc/synthorg/tools/sandbox/lifecycle/per_agent.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/bus/_nats_receive.pysrc/synthorg/api/controllers/ws_revalidation.pysrc/synthorg/meta/mcp/handlers/workflows.pysrc/synthorg/engine/task_engine_loops.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/controllers/projects.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/integrations/health/prober.pysrc/synthorg/communication/bus/memory.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/security/timeout/scheduler.pysrc/synthorg/engine/workflow/execution_service.pysrc/synthorg/budget/quota_poller.pysrc/synthorg/telemetry/collector.pysrc/synthorg/backup/scheduler.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/hr/pruning/service.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/workers/worker.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/communication/conflict_resolution/escalation/in_memory_store.pysrc/synthorg/hr/persistence_protocol.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/api/services/project_service.pysrc/synthorg/backup/service.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/api/auth/user_service.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/engine/task_engine.py
{README.md,docs/**/*.md}
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README and public docs sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers.
Files:
docs/reference/mcp-handler-contract.mddocs/design/tools.md
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use
d2for architecture / nested containers,mermaidfor flowcharts / sequence / pipelines. Markdown tables for tabular data. D2 theme 200 (Dark Mauve), D2 CLI pinned to v0.7.1 in CI.
Files:
docs/reference/mcp-handler-contract.mddocs/design/tools.md
web/src/api/endpoints/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state)
Files:
web/src/api/endpoints/workflow-executions.ts
src/synthorg/meta/mcp/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define
ToolHandler+args_modelfor MCP tools; callrequire_admin_guardrails()on admin tools; route through service layers. See mcp-handler-contract.md.
Files:
src/synthorg/meta/mcp/handlers/workflow_executions.pysrc/synthorg/meta/mcp/handlers/workflows.py
web/src/{components,pages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/{components,pages}/**/*.{ts,tsx}: UseuseViewportSize()from@/hooks/useViewportSize(useSyncExternalStoreoverwindowresize) instead of readingwindow.innerWidth/window.innerHeightdirectly
Never readwindow.innerWidth/window.innerHeightdirectly inside a component render body oruseMemo
Files:
web/src/pages/WorkflowExecutionsPage.tsx
web/src/{pages,components}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/{pages,components}/**/*.{ts,tsx}: Status badge display should use<StatusBadge>component with role="img" + aria-label by default; usedecorativefor adjacent-labeled,announcefor live WebSocket updates
KPI displays should use<MetricCard>,<Sparkline>,<ProgressGauge>, or<TokenUsageBar>instead of creating custom implementations
Card containers should use<SectionCard>(titled wrapper with icon and action slot),<AgentCard>, or<DeptHealthBar>for domain-specific cards instead of recreating inline
Form fields should use<InputField>,<SelectField>,<SliderField>,<ToggleField>,<SegmentedControl>,<TagInput>, or<SearchInput>instead of creating custom field implementations
Slide-in panels must use<Drawer width="compact|narrow|default|wide">(Base UI) without inlinew-[40vw]overrides
Loading, empty, and error states should use<Skeleton>family,<EmptyState>,<ErrorBoundary>,<ErrorBanner>, or<ProgressIndicator>instead of custom implementations
UseuseEmptyStateProps({ filteredCount, totalCount, filterActive, empty, filtered })from@/hooks/use-empty-state-propsto branch on a single value instead of duplicating the 'no data ever' / 'no data after filter' discriminator
Status/role/risk/urgency badge classes must useSTATUS_COLORSfamily from@/styles/status-colors(typed lookups); never define inlineRecord<EnumValue, string>constants per page
Use<ConfirmDialog>or<Toast>(Zustand-backed queue) for confirmation and toast feedback instead of Base UI's Toast
Use<CommandPalette>,<KeyboardShortcutHint>, or<CommandCheatsheet>for command palette and keyboard shortcut UI instead of custom implementations
Files:
web/src/pages/WorkflowExecutionsPage.tsx
web/src/pages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/pages/**/*.{ts,tsx}: List page primitives should use<ListHeader>,<SearchFilterSort>,<Pagination>,<BulkActionBar>,<MetadataGrid>, or<Breadcrumbs>instead of custom implementations
List page root container must usespace-y-section-gap(the majority pattern;flex flex-col gap-section-gapis equivalent but discouraged)
<ErrorBanner>must land immediately after<ListHeader>, before any filter/pagination row
List pages with a one-line mission statement must pass it via<ListHeader description="..." />
Files:
web/src/pages/WorkflowExecutionsPage.tsx
web/src/mocks/handlers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/mocks/handlers/**/*.{ts,tsx}: MSW handlers inweb/src/mocks/handlers/must mirrorweb/src/api/endpoints/*.ts1:1 with a default happy-path handler for every exported endpoint
Use typed envelope helpers (successFor,paginatedFor,voidSuccess) in MSW handlers to keep handlers in lockstep with endpoint return types
Files:
web/src/mocks/handlers/workflow-executions.ts
🧠 Learnings (1)
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
tests/conformance/persistence/test_workflow_definition_repository.pysrc/synthorg/persistence/preset_protocol.pytests/unit/meta/test_rules_service.pysrc/synthorg/integrations/tunnel/ngrok_adapter.pysrc/synthorg/persistence/workflow_execution_protocol.pysrc/synthorg/communication/meeting/scheduler.pysrc/synthorg/persistence/risk_override_protocol.pysrc/synthorg/communication/bus/_nats_history.pytests/conformance/persistence/test_custom_rule_repository.pysrc/synthorg/providers/resilience/rate_limiter.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/meta/chief_of_staff/monitor.pysrc/synthorg/meta/mcp/handlers/workflow_executions.pysrc/synthorg/persistence/training_protocol.pysrc/synthorg/integrations/rate_limiting/shared_state.pysrc/synthorg/api/services/idempotency_service.pysrc/synthorg/tools/sandbox/lifecycle/per_agent.pytests/unit/api/fakes_workflow.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/bus/_nats_receive.pysrc/synthorg/api/controllers/ws_revalidation.pysrc/synthorg/meta/mcp/handlers/workflows.pytests/unit/api/fake_user_repository.pytests/conformance/persistence/test_project_repository.pysrc/synthorg/persistence/sqlite/preset_repo.pysrc/synthorg/engine/task_engine_loops.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/api/controllers/ws.pytests/conformance/persistence/test_preset_repository.pysrc/synthorg/api/controllers/projects.pysrc/synthorg/persistence/postgres/project_repo.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/integrations/health/prober.pysrc/synthorg/communication/bus/memory.pytests/unit/persistence/test_protocol.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/security/timeout/scheduler.pytests/conformance/persistence/test_subworkflow_repository.pysrc/synthorg/engine/workflow/execution_service.pysrc/synthorg/persistence/_shared/pagination.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/postgres/subworkflow_repo.pytests/conformance/persistence/test_risk_override_repository.pytests/unit/api/controllers/test_workflows.pytests/unit/engine/workflow/test_execution_lifecycle.pysrc/synthorg/budget/quota_poller.pysrc/synthorg/persistence/postgres/hr_repositories.pysrc/synthorg/telemetry/collector.pytests/unit/engine/workflow/test_subworkflow_registry.pytests/unit/api/fakes.pysrc/synthorg/backup/scheduler.pytests/unit/api/controllers/test_projects.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/workflow_definition_protocol.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pytests/unit/api/fakes_backend.pysrc/synthorg/persistence/postgres/custom_rule_repo.pytests/conformance/persistence/test_workflow_execution_repository.pysrc/synthorg/persistence/sqlite/risk_override_repo.pysrc/synthorg/hr/pruning/service.pytests/unit/api/services/test_project_service.pysrc/synthorg/persistence/postgres/preset_repo.pysrc/synthorg/integrations/oauth/token_manager.pysrc/synthorg/workers/worker.pysrc/synthorg/persistence/sqlite/hr_repositories.pysrc/synthorg/persistence/sqlite/escalation_repo.pysrc/synthorg/persistence/sqlite/custom_rule_repo.pytests/unit/engine/workflow/test_execution_service.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/persistence/custom_rule_protocol.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/communication/conflict_resolution/escalation/in_memory_store.pysrc/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/hr/persistence_protocol.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/api/services/project_service.pysrc/synthorg/backup/service.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/persistence/_shared/__init__.pysrc/synthorg/api/auth/user_service.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/postgres/user_repo.pytests/unit/api/controllers/test_workflow_executions.pysrc/synthorg/persistence/project_protocol.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/postgres/risk_override_repo.pysrc/synthorg/engine/task_engine.pysrc/synthorg/persistence/subworkflow_protocol.pysrc/synthorg/persistence/sqlite/project_repo.py
🪛 LanguageTool
docs/reference/mcp-handler-contract.md
[style] ~62-~62: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nation(arguments, *, default_limit=50)` for offset/limit with bool rejection and bo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
| # Eager init: stop() must be safe before any start() call. | ||
| self._lifecycle_lock = asyncio.Lock() # lint-allow: loop-bound-init -- see. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider inlining the justification in the lint-allow marker.
The # lint-allow: loop-bound-init -- see. marker defers to the prior line's explanation, but self-contained markers improve searchability and maintainability. Consider replacing see. with the inline reason:
♻️ Suggested refinement
- # Eager init: stop() must be safe before any start() call.
- self._lifecycle_lock = asyncio.Lock() # lint-allow: loop-bound-init -- see.
+ self._lifecycle_lock = asyncio.Lock() # lint-allow: loop-bound-init -- lifecycle safety: stop() must be safe pre-start.Or more concisely:
- # Eager init: stop() must be safe before any start() call.
- self._lifecycle_lock = asyncio.Lock() # lint-allow: loop-bound-init -- see.
+ self._lifecycle_lock = asyncio.Lock() # lint-allow: loop-bound-init -- stop() must be safe before start().This mirrors the inline justification pattern used for the long-running-loop-kill-switch markers throughout this PR (e.g., "stop_event drives shutdown" at line 279 of backup/scheduler.py).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Eager init: stop() must be safe before any start() call. | |
| self._lifecycle_lock = asyncio.Lock() # lint-allow: loop-bound-init -- see. | |
| self._lifecycle_lock = asyncio.Lock() # lint-allow: loop-bound-init -- lifecycle safety: stop() must be safe pre-start. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/synthorg/notifications/adapters/slack.py` around lines 104 - 105, Update
the lint-allow comment on the asyncio lock initialization so it includes a
self-contained justification instead of "see."; specifically replace the
trailing comment for self._lifecycle_lock = asyncio.Lock() with a lint-allow
marker that explains why loop-bound-init is safe (for example: "# lint-allow:
loop-bound-init -- lock created at object init; stop() must be safe before
start() and no event loop is bound here"). Keep the symbol self._lifecycle_lock
and the use of asyncio.Lock() unchanged, only change the inline comment text to
be explicit and searchable.
Persistence: - Add ORDER BY created_at DESC to subworkflow list_versions (Postgres + SQLite). - Add id DESC tiebreaker to training_plan list_by_agent (Postgres + SQLite). - Capture validate_pagination_args return in project_repo list_projects (Postgres + SQLite). - Refactor hr_repositories list_events to use validate_pagination_args (Postgres + SQLite). - Name offset=0 explicitly and add intent comment in workflow_execution_repo list_by_definition/status (Postgres + SQLite). - Update user_protocol docstring to reference :data:DEFAULT_LIST_LIMIT. Engine: - subworkflow_registry.list_page() now requests MAX_LIST_LIMIT so cursor pagination sees the full population. Comments: - Replace 'see.' placeholder lint-allow markers with self-contained leading comment + opt-out marker (ntfy, slack, fine_tune_orchestrator). - Fix fine_tune_orchestrator comment that named the wrong method pair. - Expand per_agent idle-expiry lint-allow with kill-switch rationale. Tests: - Tighten conformance limit assertions == N for 7 repository test files. - Tighten unit controller pagination assertions == 2 (projects, workflow_executions, workflows). - Align FakeUserRepository.list_users and FakeRiskOverrideRepository.list_active / FakeCustomRuleRepository.list_rules with shared validate_pagination_args. - test_rules_service fake repo: import DEFAULT_LIST_LIMIT instead of hardcoding 100. Web: - workflow-executions.ts module docstring updated to describe the PaginatedResponse envelope.
Pre-push gate require's the marker on the while line itself or one of the two preceding source lines; move the descriptive comment block above the inline opt-out marker.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
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 (2)
src/synthorg/persistence/user_protocol.py (1)
198-215:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
list_by_userdefault withDEFAULT_LIST_LIMIT(Line 202).The docstring now says default is
DEFAULT_LIST_LIMIT(Lines 209-210), but the signature still hardcodes100(Line 202). This creates contract drift and leaves a magic number insrc/.Suggested fix
async def list_by_user( self, user_id: NotBlankStr, *, - limit: int = 100, + limit: int = DEFAULT_LIST_LIMIT, offset: int = 0, ) -> tuple[ApiKey, ...]:As per coding guidelines, “No hardcoded numerics; numerics live in
settings/definitions/… and module-level annotated named constants …”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/persistence/user_protocol.py` around lines 198 - 215, The default value for limit in the list_by_user function is hardcoded to 100 but the docstring references DEFAULT_LIST_LIMIT; change the signature to use DEFAULT_LIST_LIMIT (replace the literal 100 with DEFAULT_LIST_LIMIT) and add the necessary import for DEFAULT_LIST_LIMIT at the module top so the symbol is available; keep the docstring as-is and ensure no other magic numbers remain in list_by_user.tests/unit/meta/test_rules_service.py (1)
42-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate fake
limitbefore slicing to avoid contract drift.At Line [49], raw slicing allows negative
limitsemantics (e.g.,[:-1]), which can hide pagination bugs and diverges from repository behavior that validates pagination args.Proposed fix
from synthorg.meta.rules.service import CustomRuleNotFoundError, CustomRulesService from synthorg.persistence._shared import DEFAULT_LIST_LIMIT +from synthorg.persistence._shared.pagination import validate_pagination_args @@ async def list_rules( self, *, enabled_only: bool = False, limit: int = DEFAULT_LIST_LIMIT, ) -> tuple[CustomRuleDefinition, ...]: + limit = validate_pagination_args( + limit, offset=0, event="fake.custom_rules.list_rules" + ) rows = [r for r in self._rows.values() if not enabled_only or r.enabled] return tuple(sorted(rows, key=lambda r: r.name)[:limit])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/meta/test_rules_service.py` around lines 42 - 50, The fake list_rules implementation accepts a negative limit which causes Python slice negative semantics (hiding pagination bugs); in the list_rules function validate the limit argument (the limit parameter and DEFAULT_LIST_LIMIT usage) before slicing: ensure limit is an int and raise ValueError for negative values (or normalize to 0 if project convention prefers), then apply slicing on the sorted rows tuple; reference the list_rules method and self._rows to locate the change.
🤖 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/subworkflow_repo.py`:
- Around line 306-310: In list_versions, the SQL applies LIMIT before
semantic-version ordering which can return wrong top-N semver results; update
the queries in subworkflow_repo.list_versions (the SELECT semver ... WHERE
subworkflow_id = %s ... LIMIT %s blocks) to ORDER BY the _semver_sort_key (or
the computed semver sort expression) and then apply LIMIT, or alternatively
fetch all matching semver rows and perform sorting via _semver_sort_key in
Python and then slice by the limit; ensure you change both occurrences (the
block around semver selection at lines ~306-310 and the similar block at
~322-324) and keep using the same subworkflow_id and limit parameters.
In `@src/synthorg/persistence/sqlite/subworkflow_repo.py`:
- Around line 367-370: The list_versions query currently applies LIMIT before
semver ordering which can drop higher-version rows; update the SQL in
list_versions to perform the semver ORDER BY prior to applying LIMIT (e.g.,
ORDER BY semver DESC, or wrap a subquery that orders by semver then applies
LIMIT) so paging returns the highest semver values; apply the same change to the
other similar query instance noted around the second occurrence (the SELECT
semver FROM subworkflows ... LIMIT ...) to ensure both places order by semver
before limiting.
In `@tests/conformance/persistence/test_subworkflow_repository.py`:
- Around line 113-119: The test currently only checks the count of versions
returned from backend.subworkflows.list_versions and should assert the actual
semver-ordered top-3 values; update the assertion to compare the returned
versions list to the expected slice (e.g., the latest three semver-sorted
versions from the saved inputs) so the call to
backend.subworkflows.list_versions(NotBlankStr("sub-001"), limit=3) is validated
for correct ordering and contents; use the same _subworkflow(version=...) inputs
to build the expected list and assert equality rather than just len(versions) ==
3.
---
Outside diff comments:
In `@src/synthorg/persistence/user_protocol.py`:
- Around line 198-215: The default value for limit in the list_by_user function
is hardcoded to 100 but the docstring references DEFAULT_LIST_LIMIT; change the
signature to use DEFAULT_LIST_LIMIT (replace the literal 100 with
DEFAULT_LIST_LIMIT) and add the necessary import for DEFAULT_LIST_LIMIT at the
module top so the symbol is available; keep the docstring as-is and ensure no
other magic numbers remain in list_by_user.
In `@tests/unit/meta/test_rules_service.py`:
- Around line 42-50: The fake list_rules implementation accepts a negative limit
which causes Python slice negative semantics (hiding pagination bugs); in the
list_rules function validate the limit argument (the limit parameter and
DEFAULT_LIST_LIMIT usage) before slicing: ensure limit is an int and raise
ValueError for negative values (or normalize to 0 if project convention
prefers), then apply slicing on the sorted rows tuple; reference the list_rules
method and self._rows to locate the change.
🪄 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
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0573667-046d-46b3-b57a-62f90bfd9eee
📒 Files selected for processing (30)
src/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/persistence/postgres/hr_repositories.pysrc/synthorg/persistence/postgres/project_repo.pysrc/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/hr_repositories.pysrc/synthorg/persistence/sqlite/project_repo.pysrc/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/tools/sandbox/lifecycle/per_agent.pytests/conformance/persistence/test_custom_rule_repository.pytests/conformance/persistence/test_preset_repository.pytests/conformance/persistence/test_project_repository.pytests/conformance/persistence/test_risk_override_repository.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_workflow_definition_repository.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_projects.pytests/unit/api/controllers/test_workflow_executions.pytests/unit/api/controllers/test_workflows.pytests/unit/api/fake_user_repository.pytests/unit/api/fakes_backend.pytests/unit/meta/test_rules_service.pyweb/src/api/endpoints/workflow-executions.ts
📜 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). (14)
- GitHub Check: Deploy Preview
- GitHub Check: Build Backend
- GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
- GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: CodSpeed Web benchmarks
- GitHub Check: CodSpeed Python benchmarks
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dashboard Test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (10)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers: use@pytest.mark.{unit,integration,e2e,slow}. Asyncauto. Timeout 30s global. Coverage 80% min.
Windows: unit tests useWindowsSelectorEventLoopPolicy(3.14 IOCP teardown race). Subprocess tests override back.
Test doubles ladder:FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags. BareMagicMockat a typed boundary is blocked byscripts/check_mock_spec.py(zero-tolerance).
FakeClock andmock_ofimport fromtests._shared; inject viaclock=and the helper's spec subscript.
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add@example(...)).
Flaky tests: NEVER skip/xfail; fix fundamentally. Useasyncio.Event().wait()notsleep(large).
Files:
tests/conformance/persistence/test_preset_repository.pytests/conformance/persistence/test_risk_override_repository.pytests/conformance/persistence/test_project_repository.pytests/unit/api/fake_user_repository.pytests/unit/api/controllers/test_workflows.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_custom_rule_repository.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_workflow_executions.pytests/unit/meta/test_rules_service.pytests/unit/api/controllers/test_projects.pytests/conformance/persistence/test_workflow_definition_repository.pytests/unit/api/fakes_backend.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/conformance/persistence/test_preset_repository.pytests/conformance/persistence/test_risk_override_repository.pytests/conformance/persistence/test_project_repository.pytests/unit/api/fake_user_repository.pytests/unit/api/controllers/test_workflows.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_custom_rule_repository.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_workflow_executions.pytests/unit/meta/test_rules_service.pytests/unit/api/controllers/test_projects.pytests/conformance/persistence/test_workflow_definition_repository.pytests/unit/api/fakes_backend.py
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance:
tests/conformance/persistence/consumesbackendfixture (SQLite + Postgres). Enforced bycheck_dual_backend_test_parity.py.
Files:
tests/conformance/persistence/test_preset_repository.pytests/conformance/persistence/test_risk_override_repository.pytests/conformance/persistence/test_project_repository.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_custom_rule_repository.pytests/conformance/persistence/test_workflow_execution_repository.pytests/conformance/persistence/test_workflow_definition_repository.py
src/synthorg/!(persistence)/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQL.
Files:
src/synthorg/tools/sandbox/lifecycle/per_agent.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/notifications/adapters/ntfy.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Configuration precedence: DB > env > YAML > code default viaSettingsService/ConfigResolver; noos.environ.getoutside startup.
No hardcoded numerics; numerics live insettings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of the formNAME: int|float|Final|Final[int]|Final[float] = literal. Enforced byscripts/check_no_magic_numbers.py.
Comments explain WHY only; no reviewer citations / issue back-refs / migration framing. Enforced bycheck_no_review_origin_in_code.py+check_no_migration_framing.py.
Nofrom __future__ import annotations(3.14 has PEP 649). Use PEP 758 except:except A, B:requires parens when binding.
Type hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines.
Errors:<Domain><Condition>ErrorfromDomainError; never inheritException/RuntimeError/etc directly. Enforced bycheck_domain_error_hierarchy.py.
Pydantic v2 frozen +extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes);@computed_fieldfor derived;NotBlankStrfor identifiers.
Args models at every system boundary;parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.py.
Immutability: usemodel_copy(update=...)orcopy.deepcopy(); deepcopy at system boundaries.
Async: useasyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError).
Clock seam: declareclock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out stops mark unrestartable.
Untrusted content (SEC-1): usewrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTML.
Repository CRUD: use methodssave(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuples.
Datetime in persistence: useparse_iso_utc/ `format_iso_...
Files:
src/synthorg/tools/sandbox/lifecycle/per_agent.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/persistence/postgres/project_repo.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/hr_repositories.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/sqlite/project_repo.pysrc/synthorg/persistence/postgres/hr_repositories.pysrc/synthorg/persistence/sqlite/subworkflow_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/tools/sandbox/lifecycle/per_agent.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/persistence/postgres/project_repo.pysrc/synthorg/notifications/adapters/slack.pysrc/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/hr_repositories.pysrc/synthorg/engine/workflow/subworkflow_registry.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/sqlite/project_repo.pysrc/synthorg/persistence/postgres/hr_repositories.pysrc/synthorg/persistence/sqlite/subworkflow_repo.py
web/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,js,jsx}: Always usecreateLoggerfrom@/lib/logger; never use bareconsole.warn,console.error, orconsole.debugin application code
Logger variable name must always belog(e.g.,const log = createLogger('module-name'))
Pass dynamic or untrusted values to logger methods as separate arguments (not interpolated into the message string) so they go throughsanitizeArg
Wrap attacker-controlled fields inside structured objects insanitizeForLog()before embedding them in logs
Files:
web/src/api/endpoints/workflow-executions.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Callers MUST NOT wrap store mutation calls in try/catch; the store owns the error UX
Any new caller of health/readiness endpoints must handle the 503 path explicitly
Any new WS payload handler that ingests untrusted strings MUST route throughsanitizeWsString()orsanitizeWsEnum()helpers
ImportErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported fromweb/src/api/types/error-codes.gen.ts); discriminate onErrorCode.<NAME>, never on raw integer literals
Never export agetXIcon(value): LucideIconfactory that returns a component reference called inside JSX render bodies; export a<XIcon value={...} {...lucideProps} />wrapper component instead usingcreateElementfor the lookup
Detectfetch()in effects withoutAbortControllercleanup using the@eslint-react/web-api-no-leaked-fetchrule
Catch the{count && <Foo />}bug using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...)
Restrictwindow,document,localStorage, and other globals inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handler,useEffect, oruseSyncExternalStore-backed hook
Files:
web/src/api/endpoints/workflow-executions.ts
web/src/{api,stores}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Display counts come from
data.length; the wire envelope no longer carriestotalfield for paginated responses
Files:
web/src/api/endpoints/workflow-executions.ts
web/src/api/endpoints/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state)
Files:
web/src/api/endpoints/workflow-executions.ts
web/src/**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Reuse
web/src/components/ui/design tokens only in web dashboard design system.
Files:
web/src/api/endpoints/workflow-executions.ts
🧠 Learnings (1)
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
tests/conformance/persistence/test_preset_repository.pytests/conformance/persistence/test_risk_override_repository.pysrc/synthorg/tools/sandbox/lifecycle/per_agent.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pytests/conformance/persistence/test_project_repository.pytests/unit/api/fake_user_repository.pytests/unit/api/controllers/test_workflows.pytests/conformance/persistence/test_subworkflow_repository.pytests/conformance/persistence/test_custom_rule_repository.pysrc/synthorg/memory/embedding/fine_tune_orchestrator.pysrc/synthorg/persistence/postgres/project_repo.pytests/conformance/persistence/test_workflow_execution_repository.pysrc/synthorg/notifications/adapters/slack.pytests/unit/api/controllers/test_workflow_executions.pytests/unit/meta/test_rules_service.pysrc/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/persistence/postgres/training_plan_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/hr_repositories.pytests/unit/api/controllers/test_projects.pytests/conformance/persistence/test_workflow_definition_repository.pysrc/synthorg/engine/workflow/subworkflow_registry.pytests/unit/api/fakes_backend.pysrc/synthorg/persistence/sqlite/training_plan_repo.pysrc/synthorg/notifications/adapters/ntfy.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/sqlite/project_repo.pysrc/synthorg/persistence/postgres/hr_repositories.pysrc/synthorg/persistence/sqlite/subworkflow_repo.py
🔇 Additional comments (34)
src/synthorg/tools/sandbox/lifecycle/per_agent.py (1)
277-280: Kill-switch lint-allow justification is now clear and sufficient.This comment now documents the intentional exception and shutdown behavior clearly, matching the loop and
cleanup_all()control flow.src/synthorg/memory/embedding/fine_tune_orchestrator.py (1)
99-101: Comment and eager-init rationale look correct.The updated wording now matches the real methods (
start()/cancel()) and provides a clear WHY for the eager lock initialization.src/synthorg/notifications/adapters/slack.py (1)
104-106: Eager lifecycle-lock init is well justified and consistent.The added rationale clearly explains why lock initialization happens in
__init__, and it aligns with the idempotentstart()/close()lifecycle behavior.src/synthorg/notifications/adapters/ntfy.py (1)
116-118: Lifecycle lock eager-init comment is clear and appropriate.This change cleanly documents the pre-start
close()safety rationale while preserving the existing idempotent lifecycle pattern.src/synthorg/persistence/postgres/training_plan_repo.py (2)
26-29: Shared pagination utilities are integrated cleanly.Using
DEFAULT_LIST_LIMITplusvalidate_pagination_argshere keeps repository pagination behavior consistent and centrally enforced.
285-310:list_by_agentpagination hardening looks correct.The keyword-only
limit, shared validation, deterministic ordering, and parameterizedLIMITform a solid bounded-list implementation.src/synthorg/persistence/sqlite/training_plan_repo.py (2)
28-31: Import changes are aligned with shared pagination policy.Good use of shared pagination constants/helpers to keep SQLite behavior consistent with the rest of persistence.
316-337: SQLitelist_by_agentupdate is solid and parity-friendly.Bounded
limit, shared validation, deterministic order, and SQLLIMITparameterization are implemented correctly.src/synthorg/persistence/postgres/hr_repositories.py (4)
39-42: Shared pagination import usage is clean and consistent.Centralizing defaults/validation through the shared module improves parity across backends.
120-152:list_eventsnow enforces bounded reads correctly.Using
DEFAULT_LIST_LIMIT, validating arguments, and bindingLIMIT %scloses the unbounded-query gap in this method.
248-277:TaskMetricquery pagination wiring looks correct.The validated
limitis now enforced in SQL with bound parameters, matching the protocol intent.
369-395:CollaborationMetricquery now correctly applies limit bounds.Good alignment with shared pagination behavior and backend conformance expectations.
src/synthorg/persistence/sqlite/hr_repositories.py (4)
36-39: Shared pagination helpers are integrated correctly.This removes local drift and keeps defaults/validation behavior centralized.
117-153:SQLiteLifecycleEventRepository.list_eventspagination is now properly bounded.Validated limit plus
LIMIT ?binding gives consistent and safe enforcement.
242-272:SQLiteTaskMetricRepository.querylimit handling looks good.The method now validates and applies
limitdirectly in SQL, matching the updated protocol contract.
363-389:SQLiteCollaborationMetricRepository.queryapplies pagination correctly.Good parity with the Postgres-side implementation and shared validator behavior.
src/synthorg/persistence/user_protocol.py (1)
58-75: Good pagination hardening onUserRepository.list_users.Using a keyword-only
limitwithDEFAULT_LIST_LIMITand documenting cursor-stable pagination improves safety and contract clarity.tests/unit/api/fake_user_repository.py (1)
101-110: Fake repo pagination behavior now matches production expectations.
validate_pagination_args(...)plus bounded slicing keeps tests aligned with real repository semantics.tests/unit/api/controllers/test_workflow_executions.py (1)
190-209: Good overflow pagination coverage.This test now enforces exact page size and verifies both
has_moreandnext_cursor, which tightens the cursor-pagination contract.tests/conformance/persistence/test_workflow_definition_repository.py (1)
158-167: Limit conformance assertion is correctly strict.Seeding 5 and asserting exactly 3 rows for
limit=3is the right regression guard.tests/conformance/persistence/test_risk_override_repository.py (1)
99-106: Solid limit-respect conformance test.The setup and exact-size assertion provide a clear contract check for
list_active(limit=3).tests/unit/api/controllers/test_workflows.py (1)
205-216: Overflow pagination behavior is validated well.Exact data length plus
has_more/next_cursorchecks make this test appropriately strict.tests/conformance/persistence/test_custom_rule_repository.py (1)
114-119: Nice contract test for list truncation.The exact-size assertion after seeding more rows is the right conformance pattern.
tests/conformance/persistence/test_preset_repository.py (1)
81-92: Good pagination conformance coverage for presets.This is a clear and strict
limitenforcement test.tests/unit/meta/test_rules_service.py (1)
92-108: Parametrized limit test improves service pagination confidence.Checking both truncated page size and untruncated
totalis exactly what this service contract needs.tests/unit/api/fakes_backend.py (1)
78-87: Great parity update for fake repositories.Validating
limitvia shared pagination logic before slicing keeps unit fakes aligned with repository contract behavior.Also applies to: 428-438
src/synthorg/persistence/sqlite/project_repo.py (1)
322-365: Pagination limit handling is correctly enforced and safely parameterized.Nice update:
limitis validated/clamped before query execution, andLIMITremains bound via parameters.web/src/api/endpoints/workflow-executions.ts (1)
64-73: Cursor-pagination API migration looks consistent.The endpoint now returns a typed
PaginatedResultand correctly unwraps the paginated envelope.src/synthorg/persistence/postgres/project_repo.py (1)
235-270:list_projectspagination update is solid.Validation + hard-cap + bound
LIMITkeeps this path both safe and predictable.tests/unit/api/controllers/test_projects.py (1)
65-80: Good overflow pagination regression test.The exact-size assertion plus
has_more/next_cursorchecks makes this behavior deterministic.tests/conformance/persistence/test_project_repository.py (1)
82-89: Conformance limit check is clear and deterministic.Asserting exactly 3 rows here is the right guard for backend parity.
src/synthorg/persistence/postgres/workflow_execution_repo.py (1)
326-364: Both listing methods now implement bounded pagination cleanly.The explicit validator call, hard-cap, and bound SQL
LIMITare all in good shape.Also applies to: 386-424
src/synthorg/persistence/sqlite/workflow_execution_repo.py (1)
371-405: SQLite workflow execution pagination changes look correct.Validation, cap enforcement, and parameterized
LIMITare consistently applied in both list paths.Also applies to: 427-461
tests/conformance/persistence/test_workflow_execution_repository.py (1)
223-241: Great conformance additions for limit enforcement.Both tests now verify exact page-size behavior, which is the right regression guard.
Also applies to: 243-261
Persistence:
- subworkflow_repo list_versions (Postgres + SQLite): drop SQL LIMIT, fetch all matching rows, sort by _semver_sort_key in Python, then slice to limit. The previous 'ORDER BY created_at DESC LIMIT' could exclude higher semver versions when semver order diverges from created_at order.
- user_protocol.list_by_user and the matching Postgres/SQLite implementations: replace literal default 100 with DEFAULT_LIST_LIMIT, matching the docstring contract.
Tests:
- test_subworkflow_repository: assert versions == ('1.4.0', '1.3.0', '1.2.0') instead of just len() == 3 to lock the semver-ordered slice.
- test_rules_service _FakeCustomRuleRepository.list_rules: validate limit via shared validate_pagination_args before slicing so negative-slice semantics cannot mask bugs.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthorg/persistence/sqlite/user_repo.py (1)
714-724:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the validated/clamped
limitfor the SQL queryLine 714 validates pagination but ignores the returned
limit. Line 723 then passes the original value to SQL, which can bypass max-limit clamping.Suggested fix
- validate_pagination_args( + limit = validate_pagination_args( limit, offset, event=PERSISTENCE_API_KEY_LIST_FAILED, user_id=user_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/sqlite/user_repo.py` around lines 714 - 724, The pagination validation call to validate_pagination_args in the api key listing path returns the clamped/validated limit and should be used in the SQL parameters instead of the original limit; update the call site (validate_pagination_args) to capture its returned validated limit/offset (or destructure its return) and then use that validated value when building params for the query that sets " LIMIT ? OFFSET ?" (referencing validate_pagination_args, PERSISTENCE_API_KEY_LIST_FAILED, user_id, sql, params) so the executed query honors the enforced max-limit.src/synthorg/persistence/postgres/user_repo.py (1)
543-553:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the clamped
limitreturned by pagination validationLine 543 validates
limit, but the returned value is discarded, so Line 552 can still send an unclamped limit to SQL. This bypasses the max-limit guard and can reintroduce oversized fetches.Suggested fix
- validate_pagination_args( + limit = validate_pagination_args( limit, offset, event=PERSISTENCE_API_KEY_LIST_FAILED, user_id=user_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/user_repo.py` around lines 543 - 553, validate_pagination_args returns the clamped limit but the code ignores it; update the call to capture its return (e.g., clamped_limit = validate_pagination_args(...)) and then use that clamped_limit when appending to params and building the SQL LIMIT clause instead of the original limit variable so api_keys query in the function (around validate_pagination_args, sql, params) always enforces the max-limit guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/synthorg/persistence/postgres/user_repo.py`:
- Around line 543-553: validate_pagination_args returns the clamped limit but
the code ignores it; update the call to capture its return (e.g., clamped_limit
= validate_pagination_args(...)) and then use that clamped_limit when appending
to params and building the SQL LIMIT clause instead of the original limit
variable so api_keys query in the function (around validate_pagination_args,
sql, params) always enforces the max-limit guard.
In `@src/synthorg/persistence/sqlite/user_repo.py`:
- Around line 714-724: The pagination validation call to
validate_pagination_args in the api key listing path returns the
clamped/validated limit and should be used in the SQL parameters instead of the
original limit; update the call site (validate_pagination_args) to capture its
returned validated limit/offset (or destructure its return) and then use that
validated value when building params for the query that sets " LIMIT ? OFFSET ?"
(referencing validate_pagination_args, PERSISTENCE_API_KEY_LIST_FAILED, user_id,
sql, params) so the executed query honors the enforced max-limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46482e16-c7ed-498c-b128-8e1b3dfcc1c4
📒 Files selected for processing (7)
src/synthorg/persistence/postgres/subworkflow_repo.pysrc/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/persistence/user_protocol.pytests/conformance/persistence/test_subworkflow_repository.pytests/unit/meta/test_rules_service.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Build Backend
- 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 (Python 3.14)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Lighthouse Dashboard
- GitHub Check: CodSpeed Python benchmarks
- GitHub Check: CodSpeed Web benchmarks
- GitHub Check: Build Preview
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers: use@pytest.mark.{unit,integration,e2e,slow}. Asyncauto. Timeout 30s global. Coverage 80% min.
Windows: unit tests useWindowsSelectorEventLoopPolicy(3.14 IOCP teardown race). Subprocess tests override back.
Test doubles ladder:FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags. BareMagicMockat a typed boundary is blocked byscripts/check_mock_spec.py(zero-tolerance).
FakeClock andmock_ofimport fromtests._shared; inject viaclock=and the helper's spec subscript.
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add@example(...)).
Flaky tests: NEVER skip/xfail; fix fundamentally. Useasyncio.Event().wait()notsleep(large).
Files:
tests/unit/meta/test_rules_service.pytests/conformance/persistence/test_subworkflow_repository.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/meta/test_rules_service.pytests/conformance/persistence/test_subworkflow_repository.py
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance:
tests/conformance/persistence/consumesbackendfixture (SQLite + Postgres). Enforced bycheck_dual_backend_test_parity.py.
Files:
tests/conformance/persistence/test_subworkflow_repository.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Configuration precedence: DB > env > YAML > code default viaSettingsService/ConfigResolver; noos.environ.getoutside startup.
No hardcoded numerics; numerics live insettings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of the formNAME: int|float|Final|Final[int]|Final[float] = literal. Enforced byscripts/check_no_magic_numbers.py.
Comments explain WHY only; no reviewer citations / issue back-refs / migration framing. Enforced bycheck_no_review_origin_in_code.py+check_no_migration_framing.py.
Nofrom __future__ import annotations(3.14 has PEP 649). Use PEP 758 except:except A, B:requires parens when binding.
Type hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines.
Errors:<Domain><Condition>ErrorfromDomainError; never inheritException/RuntimeError/etc directly. Enforced bycheck_domain_error_hierarchy.py.
Pydantic v2 frozen +extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes);@computed_fieldfor derived;NotBlankStrfor identifiers.
Args models at every system boundary;parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.py.
Immutability: usemodel_copy(update=...)orcopy.deepcopy(); deepcopy at system boundaries.
Async: useasyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError).
Clock seam: declareclock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out stops mark unrestartable.
Untrusted content (SEC-1): usewrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTML.
Repository CRUD: use methodssave(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuples.
Datetime in persistence: useparse_iso_utc/ `format_iso_...
Files:
src/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/postgres/subworkflow_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/persistence/sqlite/subworkflow_repo.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/postgres/subworkflow_repo.py
🧠 Learnings (1)
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
tests/unit/meta/test_rules_service.pytests/conformance/persistence/test_subworkflow_repository.pysrc/synthorg/persistence/sqlite/subworkflow_repo.pysrc/synthorg/persistence/sqlite/user_repo.pysrc/synthorg/persistence/user_protocol.pysrc/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/postgres/subworkflow_repo.py
🔇 Additional comments (8)
tests/unit/meta/test_rules_service.py (3)
19-20: Good move to centralize pagination defaults/validation imports.Using shared pagination primitives here keeps the unit fake aligned with the same contract as runtime repositories.
43-53:_FakeCustomRuleRepository.list_rulesnow correctly mirrors production pagination behavior.Adding keyword-only
limit, validating withvalidate_pagination_args, and slicing after deterministic sort makes this fake robust and contract-faithful.
96-112: Strong unit coverage for limit semantics.This parametrized test correctly checks both constraints: page truncation by requested limit and
totalstaying equal to seeded count.src/synthorg/persistence/postgres/subworkflow_repo.py (2)
287-325: Correct limit application after semver ordering inlist_versions.Line 297-Line 299 validates pagination early, and Line 323-Line 325 applies truncation after semver sort. This preserves the “newest first” contract and fixes the prior pre-limit ordering pitfall.
327-355:list_summarieslimit strategy is sound and contract-preserving.Line 334-Line 339 documents the intent clearly, and Line 349-Line 355 correctly limits distinct
subworkflow_idvalues while still loading full version rows for included IDs.src/synthorg/persistence/sqlite/subworkflow_repo.py (2)
346-366:list_versionsnow enforces correct bounded semver paging.Line 364-Line 366 validates the caller limit, and Line 383-Line 385 semver-sorts before slicing, which keeps ordering correctness under pagination.
Also applies to: 383-385
387-415:list_summaries(limit=...)implementation aligns with expected bounded behavior.The inner distinct-id LIMIT and outer full-row fetch pattern (Line 410-Line 415) is a good fit for preserving per-subworkflow aggregation while enforcing a hard cap on returned summaries.
tests/conformance/persistence/test_subworkflow_repository.py (1)
99-119: Great conformance coverage for pagination limits.These tests now prove both behaviors: exact truncation (
len(rows) == 3) and correct semver top-N selection (("1.4.0", "1.3.0", "1.2.0")).
Persistence: - postgres/user_repo.py list_by_user (api_keys path): capture validate_pagination_args return so the clamped limit reaches the SQL LIMIT parameter; previously the unclamped caller value flowed straight through. - sqlite/user_repo.py list_by_user (api_keys path): same fix for SQLite parity.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/persistence/sqlite/user_repo.py (1)
339-341:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
KeyErrorin deserialization paths to keepQueryErrorcontract consistent.If a row is missing expected columns (schema drift/corrupt projection), these blocks currently leak raw
KeyErrorinstead of translating toQueryErrorlike the Postgres implementation.Suggested patch
- except (ValueError, TypeError, ValidationError) as exc: + except (ValueError, TypeError, ValidationError, KeyError) as exc: msg = "Failed to deserialize users" logger.warning( PERSISTENCE_USER_LIST_FAILED, error_type=type(exc).__name__, error=safe_error_description(exc), ) raise QueryError(msg) from exc @@ - except (ValueError, TypeError, ValidationError) as exc: + except (ValueError, TypeError, ValidationError, KeyError) as exc: msg = f"Failed to deserialize API keys for user {user_id!r}" logger.warning( PERSISTENCE_API_KEY_LIST_FAILED, user_id=user_id, error_type=type(exc).__name__, error=safe_error_description(exc), ) raise QueryError(msg) from excAlso applies to: 737-739
🤖 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/user_repo.py` around lines 339 - 341, The deserialization except blocks around building users (using _row_to_user) currently catch (ValueError, TypeError, ValidationError) but miss KeyError, causing raw KeyError to propagate; update those except clauses (the one where users = tuple(_row_to_user(row) for row in rows) and the similar block later around lines ~737-739) to also catch KeyError and map it to the same QueryError handling path so missing columns produce a QueryError consistent with the Postgres implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/synthorg/persistence/sqlite/user_repo.py`:
- Around line 339-341: The deserialization except blocks around building users
(using _row_to_user) currently catch (ValueError, TypeError, ValidationError)
but miss KeyError, causing raw KeyError to propagate; update those except
clauses (the one where users = tuple(_row_to_user(row) for row in rows) and the
similar block later around lines ~737-739) to also catch KeyError and map it to
the same QueryError handling path so missing columns produce a QueryError
consistent with the Postgres implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 380ef53d-a93c-48d3-81c1-760dc1899213
📒 Files selected for processing (2)
src/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/sqlite/user_repo.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). (13)
- GitHub Check: Deploy Preview
- GitHub Check: Build Fine-Tune (gpu, fine-tune-gpu)
- GitHub Check: Build Fine-Tune (cpu, fine-tune-cpu)
- GitHub Check: Build Backend
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Lighthouse Site
- GitHub Check: Lighthouse Dashboard
- GitHub Check: CodSpeed Python benchmarks
- GitHub Check: CodSpeed Web benchmarks
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (2)
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Configuration precedence: DB > env > YAML > code default viaSettingsService/ConfigResolver; noos.environ.getoutside startup.
No hardcoded numerics; numerics live insettings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of the formNAME: int|float|Final|Final[int]|Final[float] = literal. Enforced byscripts/check_no_magic_numbers.py.
Comments explain WHY only; no reviewer citations / issue back-refs / migration framing. Enforced bycheck_no_review_origin_in_code.py+check_no_migration_framing.py.
Nofrom __future__ import annotations(3.14 has PEP 649). Use PEP 758 except:except A, B:requires parens when binding.
Type hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines.
Errors:<Domain><Condition>ErrorfromDomainError; never inheritException/RuntimeError/etc directly. Enforced bycheck_domain_error_hierarchy.py.
Pydantic v2 frozen +extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes);@computed_fieldfor derived;NotBlankStrfor identifiers.
Args models at every system boundary;parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.py.
Immutability: usemodel_copy(update=...)orcopy.deepcopy(); deepcopy at system boundaries.
Async: useasyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError).
Clock seam: declareclock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out stops mark unrestartable.
Untrusted content (SEC-1): usewrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTML.
Repository CRUD: use methodssave(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuples.
Datetime in persistence: useparse_iso_utc/ `format_iso_...
Files:
src/synthorg/persistence/postgres/user_repo.pysrc/synthorg/persistence/sqlite/user_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/persistence/postgres/user_repo.pysrc/synthorg/persistence/sqlite/user_repo.py
🧠 Learnings (1)
📚 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/persistence/postgres/user_repo.pysrc/synthorg/persistence/sqlite/user_repo.py
🔇 Additional comments (2)
src/synthorg/persistence/postgres/user_repo.py (1)
42-45: Pagination bounding is implemented correctly across both list paths.Good change: both
list_usersandlist_by_usernow useDEFAULT_LIST_LIMIT, validate inputs throughvalidate_pagination_args, and apply SQLLIMITwith bound params.Also applies to: 240-269, 531-548
src/synthorg/persistence/sqlite/user_repo.py (1)
41-44: Limit propagation and SQL capping look good in SQLite as well.The default-limit normalization plus shared pagination validation is consistently applied in both user and API-key listing methods.
Also applies to: 299-327, 691-723
Persistence: - sqlite/user_repo.py: add KeyError to the deserialization except clauses so a row with missing columns surfaces as QueryError (matching the Postgres implementation), not raw KeyError. Applied to all six affected blocks for dual-backend parity: get(), get_by_username(), list_users(), get(api_key), get_by_hash(api_key), and list_by_user(api_key).
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
<!-- HIGHLIGHTS_START --> ## Highlights > _AI-generated summary (model: `openai/gpt-4.1-mini` via GitHub Models). Commit-based changelog below._ ### What you'll notice - Password and secret fields now include an eye-toggle for easier visibility control. - Containers running without probes are shown as healthy in the doctor command. - Unloaded and missing PR-review agents are restored and available again. ### What's new - Gate baseline protection is enhanced to block em-dashes during writing. ### Under the hood - Replaced Atlas with yoyo-migrations for persistence management. - Refactored codebase extensively, including context-bound user authentication and registry pattern for enums. - Improved linting by draining magic number usages and tightening mock and constant checks. - Updated CI to retry Docker pushes on network timeout errors. - Updated apko lockfiles for dependency management. <!-- HIGHLIGHTS_END --> :robot: I have created a release *beep* *boop* --- ## [0.8.3](v0.8.2...v0.8.3) (2026-05-12) ### Features * harden gate baseline protection + block em-dashes at write time ([#1860](#1860)) ([b41f151](b41f151)) * **web:** eye-toggle on every password / secret field ([#1873](#1873)) ([9070387](9070387)) ### Bug Fixes * **ci:** retry Docker push on Go net/http deadline + cancellation errors ([#1877](#1877)) ([23a0bfa](23a0bfa)) * **cli:** render running-no-probe containers as healthy in doctor ([#1870](#1870)) ([6263795](6263795)) * restore unloaded and missing PR-review agents ([#1875](#1875)) ([db004fd](db004fd)), closes [#1871](#1871) ### Refactoring * bind authenticated user via ContextVar ([#1858](#1858)) ([57ed0b4](57ed0b4)) * code-structure cleanup (sub-tasks D + F + G + H + I) ([#1859](#1859)) ([362e5c8](362e5c8)) * convert enum dispatch to registry pattern ([#1854](#1854)) ([e90550e](e90550e)) * drain no_magic_numbers baseline to zero via Final hoists ([#1856](#1856) phase 2) ([#1872](#1872)) ([ec8109e](ec8109e)) * drain pagination + loop-init + kill-switch baselines ([#1857](#1857)) ([#1868](#1868)) ([115c3c2](115c3c2)) * **persistence:** replace Atlas with yoyo-migrations ([#1876](#1876)) ([1b7e975](1b7e975)), closes [#1874](#1874) * protocols audit follow-up (REVIEW + fold pass) ([#1869](#1869)) ([af33ddb](af33ddb)) * protocols audit follow-up REMOVE pass ([#1867](#1867)) ([dd1eebc](dd1eebc)) * tighten check_mock_spec gate, add mock_of[T], drain baseline ([#1862](#1862)) ([240a253](240a253)) * tighten check_no_magic_numbers for named module constants ([#1856](#1856)) ([#1866](#1866)) ([90c933b](90c933b)) ### CI/CD * update apko lockfiles ([#1863](#1863)) ([2bd32e6](2bd32e6)) --- 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>
1 participant
Closes #1857.
Summary
Drains three runtime-correctness baselines to zero entries (stretch goal met):
scripts/list_pagination_baseline.txt: 37 -> 0. Every baselinedlist_*/querymethod on Protocol + SQLite + Postgres now takeslimit: int = DEFAULT_LIST_LIMIT(=100). Domains drained: user, project, workflow_definition, workflow_execution (list_by_definition,list_by_status), subworkflow (list_summaries,list_versions), hr (collaboration_metric.query,task_metric.query,training_plan.list_by_agent),preset.list_all,custom_rule.list_rules,risk_override.list_active.scripts/loop_bound_init_baseline.txt: 30 -> 0. Added# lint-allow: loop-bound-initmarkers with documented eager-init justifications across 18 lifecycle services. Eager init is the canonical pattern whenstop()must be callable before anystart()has run, perdocs/reference/lifecycle-sync.md.scripts/long_running_loops_kill_switch_baseline.txt: 26 -> 0. Added# lint-allow: long-running-loop-kill-switchmarkers with terse justifications, classified as per-request, retry-wait, sentinel, or service-stop pause.DEFAULT_LIST_LIMIT = 100lives insrc/synthorg/persistence/_shared/pagination.py. Service-layer callers and cursor paginator call sites forward the limit; test fakes mirror the new signatures.Pre-PR review (19 agents, 31 findings)
The second commit addresses every valid finding from the pre-PR review (see
_audit/pre-pr-review/triage.md). Notable items:Critical / Major fixed:
LifecycleEventRepository.list_events(hr/persistence_protocol.py+ SQLite/Postgres impls) now usesDEFAULT_LIST_LIMITinstead of a bare100literal.validate_pagination_argsnow returns the clamped limit (signature-> int); the helper enforces a newMAX_LIST_LIMIT = 10_000ceiling so internal callers bypassing the API layer'sCursorLimitcannot trigger an unbounded scan. Baselined repos opt-in by capturing the return value.web/src/api/endpoints/workflow-executions.tsnow accepts{cursor?, limit?}params and returnsPaginatedResult<WorkflowExecution>. Page consumer + MSW handler updated.limitthendel limit-d the param now slice[:limit](4 fake repos).user_protocol.list_by_userdocstring corrected to match the actuallimit=100default.docs/reference/mcp-handler-contract.md+docs/design/tools.mdclarify the intentional MCPdefault_limit=50vs repositoryDEFAULT_LIST_LIMIT=100scope split.web/src/api/types/projects.tsdrops the deadoffsetfield (backend is cursor-based).Medium fixed:
long-running-loop-kill-switchlint-allow comments reworded to reflect the actual termination mechanism (stop()/cancel, not flag-gated).has_morecursor-pagination tests for projects, workflows, workflow_executions controllers.test_project_service+test_rules_service.Conformance coverage (M7-13): Per-backend limit tests added to 7 conformance files (project, workflow_definition, workflow_execution, subworkflow, preset, risk_override, custom_rule). Verified locally: 20/20 pass under both SQLite + Postgres arms.
Verified false positives (skipped with disproof)
CursorLimit = 50intentionally;docs/guides/budget.mdis accurate.limit:200silently capped at 100"):MAX_LIMIT = 200is the API ceiling; the request lands within that bound.docs/reference/convention-gates.md— adds noise that would rot as baselines change.# lint-allow: <name> -- see.markers are correctly preceded by multi-line per-site justification comments; the-- see.suffix is a pointer, not missing context.tests/unit/persistence/test_protocol.pydegenerate Protocol-conformance fakes return()regardless oflimit, which trivially satisfies the "max limit" contract. Left as-is.Test plan
uv run python -m pytest tests/ -m unit— 28,400 passed, 18 skipped (87s).uv run python -m pytest tests/conformance/persistence/ -m integration -k "respects_limit or has_more"— 20 passed across SQLite + Postgres arms (19s).uv run ruff check src/ tests/— clean.uv run mypy src/ tests/— clean (3714 source files).npm --prefix web run lint && npm --prefix web run type-check && npm --prefix web run test— clean (3047 tests).