chore: user approval record api #703
Conversation
WalkthroughThis PR introduces a new user approval records feature for deployment versions. It adds OpenAPI schemas for approval records and requests, two new API endpoints for retrieving deployment versions and upserting user approval records, backend handlers with authorization checks and event emission, and corresponding SDK/type definitions across multiple services. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Handler
participant WS as Workspace Store
participant Event as Event Emitter
Client->>API: PUT /user-approval-records (deploymentVersionId)
API->>API: Validate authorization
alt environmentIds provided in request
API->>API: Use provided environmentIds
else environmentIds not provided
API->>WS: getEnvironmentIds(workspaceId, deploymentVersionId)
WS-->>API: Resolved environment IDs
end
API->>API: Construct UserApprovalRecord
API->>Event: Emit UserApprovalRecordCreated (per environment)
Event-->>API: Event persisted
API-->>Client: 200/201 UserApprovalRecord
Client->>API: GET /deploymentversions/{deploymentVersionId}
API->>WS: Lookup deployment version
WS-->>API: DeploymentVersion
API-->>Client: 200 DeploymentVersion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
📊 DB Package Test Coveragepkg/db coverage: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
apps/workspace-engine/oapi/openapi.json (1)
1910-1968: Endpoint naming is inconsistent with existing patterns.Path segment uses "deploymentversions" (no hyphen) while other endpoints use "deployment-versions" or "deployments/{deploymentId}/versions". Consider standardizing segment and param names (e.g., use "deployment-versions" and "versionId") or add an alias path for backward compatibility.
Example change:
- "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}": { + "/v1/workspaces/{workspaceId}/deployment-versions/{versionId}": { "get": { - "operationId": "getDeploymentVersion", + "operationId": "getDeploymentVersion", "parameters": [ { "name": "workspaceId", "in": "path", "required": true, "schema": { "type": "string" } }, - { "name": "deploymentVersionId", "in": "path", "required": true, "schema": { "type": "string" } } + { "name": "versionId", "in": "path", "required": true, "schema": { "type": "string" } } ],apps/api/openapi/schemas/userapprovalrecord.jsonnet (2)
4-12: Clarify request semantics and tighten validation.
- Document that environmentIds is optional and defaults to “all environments for the deployment’s system.”
- Add uniqueItems and optional minItems when provided.
- Optionally bound reason length.
UpsertUserApprovalRecordRequest: { type: 'object', required: ['status'], properties: { - environmentIds: { type: 'array', items: { type: 'string' } }, + environmentIds: { + description: 'If omitted, applies to all environments related to the version’s deployment system.', + type: 'array', + uniqueItems: true, + items: { type: 'string' }, + }, status: openapi.schemaRef('ApprovalStatus'), - reason: { type: 'string' }, + reason: { type: 'string', maxLength: 1024 }, }, },
14-25: Optional: add formats and descriptions to improve client UX.
- If these are UUIDs, add format: 'uuid' to userId/versionId/environmentId.
- Add short descriptions for fields and createdAt semantics.
UserApprovalRecord: { type: 'object', required: ['userId', 'versionId', 'environmentId', 'status', 'createdAt'], properties: { - userId: { type: 'string' }, - versionId: { type: 'string' }, - environmentId: { type: 'string' }, + userId: { type: 'string', description: 'Approver user id' }, + versionId: { type: 'string', description: 'Deployment version id' }, + environmentId: { type: 'string', description: 'Target environment id' }, status: openapi.schemaRef('ApprovalStatus'), reason: { type: 'string' }, - createdAt: { type: 'string', format: 'date-time' }, + createdAt: { type: 'string', format: 'date-time', description: 'Server-generated timestamp' }, }, },apps/api/src/routes/v1/workspaces/deployment-versions.ts (1)
141-148: Emit events in a single batch instead of N awaits.sendGoEvent accepts an array; batching reduces latency and log noise.
- for (const environmentId of environmentIds) - await sendGoEvent({ - workspaceId, - eventType: Event.UserApprovalRecordCreated, - timestamp: Date.now(), - data: { ...record, environmentId }, - }); + await sendGoEvent( + environmentIds.map((environmentId) => ({ + workspaceId, + eventType: Event.UserApprovalRecordCreated, + timestamp: Date.now(), + data: { ...record, environmentId }, + })), + );packages/workspace-engine-sdk/src/schema.ts (2)
164-183: Standardize path segment and param naming.Path uses "deploymentversions" and param "deploymentVersionId", while jobs-list path uses "deployment-versions" and "versionId". Align to one convention or expose both for compatibility.
1336-1378: Types for getDeploymentVersion look correct. Minor: consider reusing param name "versionId".For consistency with getDeploymentVersionJobsList, consider renaming path param to versionId in the schema generation source to keep client code uniform.
apps/api/openapi/openapi.json (2)
839-889: Request/response shapes look fine; add minor validation and docs as in Jsonnet source.Mirror uniqueItems/maxLength/description updates here when you sync from Jsonnet to keep public and source in lockstep.
1677-1734: Naming consistency for GET deployment version.Standardize path to "deployment-versions" and param to "versionId" (or add an alias path) to match adjacent endpoints.
- "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}": { + "/v1/workspaces/{workspaceId}/deployment-versions/{versionId}": { "get": { ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
apps/api/openapi/lib/openapi.libsonnet(1 hunks)apps/api/openapi/main.jsonnet(2 hunks)apps/api/openapi/openapi.json(3 hunks)apps/api/openapi/paths/userapprovalrecords.jsonnet(1 hunks)apps/api/openapi/schemas/userapprovalrecord.jsonnet(1 hunks)apps/api/src/routes/v1/workspaces/deployment-versions.ts(1 hunks)apps/api/src/routes/v1/workspaces/index.ts(2 hunks)apps/api/src/types/openapi.ts(4 hunks)apps/web/app/api/openapi.ts(3 hunks)apps/workspace-engine/oapi/openapi.json(1 hunks)apps/workspace-engine/oapi/spec/lib/openapi.libsonnet(1 hunks)apps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet(1 hunks)apps/workspace-engine/pkg/oapi/oapi.gen.go(3 hunks)apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go(1 hunks)packages/workspace-engine-sdk/src/schema.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/server/openapi/deploymentversions/server.goapps/workspace-engine/pkg/oapi/oapi.gen.go
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/workspace-engine/oapi/openapi.jsonapps/api/src/routes/v1/workspaces/deployment-versions.tsapps/api/src/types/openapi.tsapps/api/src/routes/v1/workspaces/index.tsapps/api/openapi/openapi.jsonpackages/workspace-engine-sdk/src/schema.tsapps/web/app/api/openapi.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/api/src/routes/v1/workspaces/deployment-versions.tsapps/api/src/types/openapi.tsapps/api/src/routes/v1/workspaces/index.tspackages/workspace-engine-sdk/src/schema.tsapps/web/app/api/openapi.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/api/src/routes/v1/workspaces/deployment-versions.tsapps/api/src/types/openapi.tsapps/api/src/routes/v1/workspaces/index.tspackages/workspace-engine-sdk/src/schema.tsapps/web/app/api/openapi.ts
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:49:05.086Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#655
File: apps/workspace-engine/pkg/engine/workspace/fluent.go:166-171
Timestamp: 2025-08-12T20:49:05.086Z
Learning: The UpdateDeploymentVersions() method with OperationCreate case in apps/workspace-engine/pkg/engine/workspace/fluent.go is specifically designed only for creating new deployment versions, not for handling potential duplicates or existing versions.
Applied to files:
apps/workspace-engine/pkg/server/openapi/deploymentversions/server.goapps/workspace-engine/pkg/oapi/oapi.gen.gopackages/workspace-engine-sdk/src/schema.tsapps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet
🧬 Code graph analysis (6)
apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go (1)
apps/workspace-engine/pkg/server/openapi/utils/utils.go (1)
GetWorkspace(12-26)
apps/api/src/routes/v1/workspaces/deployment-versions.ts (3)
packages/workspace-engine-sdk/src/index.ts (1)
getClientFor(22-36)apps/api/src/types/api.ts (3)
ApiError(139-157)AsyncTypedHandler(122-125)asyncHandler(210-219)packages/events/src/kafka/client.ts (1)
sendGoEvent(35-65)
apps/api/src/types/openapi.ts (2)
apps/web/app/api/openapi.ts (2)
operations(601-1403)components(283-599)packages/workspace-engine-sdk/src/schema.ts (2)
operations(964-2281)components(606-962)
apps/api/src/routes/v1/workspaces/index.ts (2)
apps/api/src/routes/v1/workspaces/policies.ts (1)
policiesRouter(92-94)apps/api/src/routes/v1/workspaces/deployment-versions.ts (2)
deploymentVersionsRouter(92-94)deploymentVersionIdRouter(150-153)
packages/workspace-engine-sdk/src/schema.ts (1)
apps/api/src/types/openapi.ts (1)
components(300-614)
apps/web/app/api/openapi.ts (1)
apps/api/src/types/openapi.ts (2)
operations(616-1479)components(300-614)
🔇 Additional comments (11)
apps/api/openapi/lib/openapi.libsonnet (1)
16-16: LGTM!The new parameter helper follows the established pattern and is properly positioned.
apps/workspace-engine/oapi/spec/lib/openapi.libsonnet (1)
17-17: LGTM!The parameter helper is consistent with the existing pattern and matches the implementation in the API OpenAPI library.
apps/api/openapi/main.jsonnet (1)
43-44: LGTM!The new imports for user approval records paths and schemas follow the established pattern and are correctly integrated into the OpenAPI specification.
Also applies to: 58-59
apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go (1)
16-33: LGTM!The implementation follows the established error handling patterns in the codebase and provides clear error messages for both workspace retrieval failures and missing deployment versions.
apps/api/openapi/paths/userapprovalrecords.jsonnet (1)
1-20: OpenAPI definition is well-structured.The endpoint definition properly uses upsert semantics with both 201 (created) and 200 (updated) responses. However, note that this path uses
/deploymentversions(no hyphen), which should be aligned with the resolution of the path naming inconsistency flagged in other files.apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
1302-1304: LGTM - Generated code follows established patterns.The generated OpenAPI server code properly implements the new GetDeploymentVersion endpoint with appropriate parameter binding, middleware support, and error handling. Since this is generated code, any changes should be made to the source OpenAPI specification files.
Also applies to: 1670-1701, 2538-2538
apps/api/openapi/schemas/userapprovalrecord.jsonnet (1)
27-30: LGTM: enum is minimal and clear.apps/api/openapi/openapi.json (1)
167-173: LGTM: DeploymentVersionStatus unchanged; context only.apps/api/src/types/openapi.ts (2)
306-307: Enum introduction LGTMApprovalStatus enum addition is clear and consistent.
138-154: apps/web client is out of sync; regenerate from current specPath mismatch confirmed: apps/web/app/api/openapi.ts has workspace-scoped
/user-approval-recordsbut apps/api/openapi/openapi.json currently defines version-scoped/deploymentversions/{deploymentVersionId}/user-approval-recordswithgetDeploymentVersion.Root cause: apps/web's generated client is stale. Both source specs (apps/api and apps/workspace-engine) already use consistent version-scoped routing. Regenerate apps/web by running
npm run generateinapps/web/to sync with the current spec. No architectural change needed; this is a maintenance sync issue.apps/web/app/api/openapi.ts (1)
85-102: Bulk additions LGTMNew workspace-scoped endpoints (deployments, versions, environments, policies, resource-providers, resources) look consistent with existing patterns and paginated list shapes.
Also applies to: 103-119, 120-137, 138-155, 156-172, 173-190, 191-208, 209-226, 227-243, 244-263
| "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/user-approval-records": { | ||
| "put": { | ||
| "operationId": "upsertUserApprovalRecord", | ||
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/UpsertUserApprovalRecordRequest" | ||
| } | ||
| } | ||
| }, | ||
| "required": true | ||
| }, | ||
| "responses": { | ||
| "200": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/UserApprovalRecord" | ||
| } | ||
| } | ||
| }, | ||
| "description": "OK response" | ||
| }, | ||
| "201": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/UserApprovalRecord" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Resource created successfully" | ||
| }, | ||
| "400": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Invalid request" | ||
| }, | ||
| "404": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Resource not found" | ||
| } | ||
| }, | ||
| "summary": "Upsert user approval record" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Spec/implementation mismatch: response type vs current handler.
Spec returns UserApprovalRecord (200/201). The handler currently returns { success: true } and may emit multiple records when environmentIds > 1.
- Either change the handler to return UserApprovalRecord (single or array) and 201 on create, or
- Update spec to ack-only (e.g., { success: boolean }) and clarify multi-environment behavior.
🏁 Script executed:
#!/bin/bash
# Show handler implementation to compare with spec.
rg -n -C3 'upsertUserApprovalRecord' apps/api/src/routes/v1/workspaces/deployment-versions.tsLength of output: 448
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full implementation of upsertUserApprovalRecord
rg -n -A30 'const upsertUserApprovalRecord: AsyncTypedHandler' apps/api/src/routes/v1/workspaces/deployment-versions.ts
Length of output: 1187
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify spec response definitions for this endpoint
rg -n -A20 'upsertUserApprovalRecord' apps/api/openapi/openapi.json
Length of output: 839
Spec/implementation mismatch confirmed: handler returns wrong response structure and status codes.
Handler at apps/api/src/routes/v1/workspaces/deployment-versions.ts:120-149 returns { success: true } with status 200, but the OpenAPI spec defines both 200 and 201 responses with UserApprovalRecord schema. The handler also loops through multiple environmentIds but returns only a single acknowledgment rather than the created record(s).
Choose one approach:
- Modify handler to return
UserApprovalRecordand use 201 for creation, or - Update spec to reflect ack-only response (
{ success: boolean }) and document multi-environment emission behavior.
🤖 Prompt for AI Agents
In apps/api/openapi/openapi.json around lines 1735-1792, the OpenAPI entry for
PUT
/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/user-approval-records
expects UserApprovalRecord bodies for 200/201, but the implementation returns an
acknowledgement { success: true } with HTTP 200 and emits records across
multiple environmentIds; fix by choosing one of two actions and applying it
consistently: either (A) change the handler to return the created
UserApprovalRecord(s) and use 201 for newly created records (if multiple
environments are created, return an array of UserApprovalRecord and update the
spec responses to reflect an array schema and 201 on creation), or (B) update
the OpenAPI spec to document the acknowledgement response ({ success: boolean })
for 200 and add clear documentation that the handler emits records for multiple
environmentIds (include an example and note that no resource body is returned),
then ensure tests and clients are updated to match the chosen contract.
| const getEnvironmentIds = async ( | ||
| workspaceId: string, | ||
| deploymentVersionId: string, | ||
| ) => { | ||
| const deploymentVersionResponse = await getClientFor(workspaceId).GET( | ||
| "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}", | ||
| { params: { path: { workspaceId, deploymentVersionId } } }, | ||
| ); | ||
|
|
||
| if (deploymentVersionResponse.data == null) | ||
| throw new ApiError("Deployment version not found", 404); | ||
| const { deploymentId } = deploymentVersionResponse.data; | ||
|
|
||
| const systemResponse = await getClientFor(workspaceId).GET( | ||
| "/v1/workspaces/{workspaceId}/systems/{systemId}", | ||
| { params: { path: { workspaceId, systemId: deploymentId } } }, | ||
| ); | ||
|
|
||
| if (systemResponse.data == null) throw new ApiError("System not found", 404); | ||
| const { environments } = systemResponse.data; | ||
|
|
||
| return environments.map((environment) => environment.id); | ||
| }; |
There was a problem hiding this comment.
Bug: using deploymentId where systemId is required; add error checks.
You fetch the version, then call GET /systems/{systemId} with systemId = deploymentId. You must first fetch the deployment to obtain systemId. Also handle engine errors consistently.
const getEnvironmentIds = async (
workspaceId: string,
deploymentVersionId: string,
) => {
const deploymentVersionResponse = await getClientFor(workspaceId).GET(
"/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}",
{ params: { path: { workspaceId, deploymentVersionId } } },
);
- if (deploymentVersionResponse.data == null)
+ if (deploymentVersionResponse.error?.error != null)
+ throw new ApiError(deploymentVersionResponse.error.error, 500);
+ if (deploymentVersionResponse.data == null)
throw new ApiError("Deployment version not found", 404);
- const { deploymentId } = deploymentVersionResponse.data;
+ const { deploymentId } = deploymentVersionResponse.data;
- const systemResponse = await getClientFor(workspaceId).GET(
- "/v1/workspaces/{workspaceId}/systems/{systemId}",
- { params: { path: { workspaceId, systemId: deploymentId } } },
- );
+ // fetch deployment to obtain systemId
+ const deploymentResponse = await getClientFor(workspaceId).GET(
+ "/v1/workspaces/{workspaceId}/deployments/{deploymentId}",
+ { params: { path: { workspaceId, deploymentId } } },
+ );
+ if (deploymentResponse.error?.error != null)
+ throw new ApiError(deploymentResponse.error.error, 500);
+ if (deploymentResponse.data == null)
+ throw new ApiError("Deployment not found", 404);
+ const { systemId } = deploymentResponse.data;
+
+ const systemResponse = await getClientFor(workspaceId).GET(
+ "/v1/workspaces/{workspaceId}/systems/{systemId}",
+ { params: { path: { workspaceId, systemId } } },
+ );
- if (systemResponse.data == null) throw new ApiError("System not found", 404);
+ if (systemResponse.error?.error != null)
+ throw new ApiError(systemResponse.error.error, 500);
+ if (systemResponse.data == null) throw new ApiError("System not found", 404);
const { environments } = systemResponse.data;
return environments.map((environment) => environment.id);
};🤖 Prompt for AI Agents
In apps/api/src/routes/v1/workspaces/deployment-versions.ts around lines 96 to
118, the code incorrectly uses deploymentId as the systemId when calling the
systems endpoint and lacks consistent error handling for engine calls; first
call the deployments endpoint to fetch the deployment by deploymentId and
extract its systemId, validate that both deploymentVersionResponse.data and
deploymentResponse.data exist and throw ApiError(…,404) when missing, then use
the retrieved systemId in the GET /systems/{systemId} request; also wrap or
normalize downstream engine/client errors consistently (e.g., convert non-404
failures into ApiError with appropriate status/message) so all failure paths are
handled uniformly.
| const upsertUserApprovalRecord: AsyncTypedHandler< | ||
| "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/user-approval-records", | ||
| "put" | ||
| > = async (req, res) => { | ||
| const { workspaceId, deploymentVersionId } = req.params; | ||
| if (req.apiContext == null) throw new ApiError("Unauthorized", 401); | ||
| const { user } = req.apiContext; | ||
|
|
||
| const record: WorkspaceEngine["schemas"]["UserApprovalRecord"] = { | ||
| userId: user.id, | ||
| versionId: deploymentVersionId, | ||
| environmentId: "", | ||
| status: req.body.status, | ||
| createdAt: new Date().toISOString(), | ||
| reason: req.body.reason, | ||
| }; | ||
|
|
||
| const environmentIds = | ||
| req.body.environmentIds ?? | ||
| (await getEnvironmentIds(workspaceId, deploymentVersionId)); | ||
|
|
||
| for (const environmentId of environmentIds) | ||
| await sendGoEvent({ | ||
| workspaceId, | ||
| eventType: Event.UserApprovalRecordCreated, | ||
| timestamp: Date.now(), | ||
| data: { ...record, environmentId }, | ||
| }); | ||
| res.status(200).json({ success: true }); | ||
| }; |
There was a problem hiding this comment.
Response does not match OpenAPI; choose one contract and align.
OpenAPI (apps/api/openapi/openapi.json) defines 200/201 returning UserApprovalRecord, but this route returns { success: true }. Either:
- Change code to return a record (or an array when environmentIds > 1) and use 201 on create; or
- Update OpenAPI to return an acknowledgment (e.g., { success: boolean }) instead of a record.
Option A (return records and batch events):
- for (const environmentId of environmentIds)
- await sendGoEvent({
- workspaceId,
- eventType: Event.UserApprovalRecordCreated,
- timestamp: Date.now(),
- data: { ...record, environmentId },
- });
- res.status(200).json({ success: true });
+ const messages = environmentIds.map((environmentId) => ({
+ workspaceId,
+ eventType: Event.UserApprovalRecordCreated,
+ timestamp: Date.now(),
+ data: { ...record, environmentId },
+ }));
+ await sendGoEvent(messages);
+ const records = messages.map((m) => m.data);
+ res.status(records.length === 1 ? 201 : 200).json(records.length === 1 ? records[0] : records);If you prefer to keep { success: true }, please update the OpenAPI response schema accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/routes/v1/workspaces/deployment-versions.ts around lines 120-149
the handler currently responds with { success: true } but OpenAPI requires
returning the created UserApprovalRecord (200/201); pick one contract and align
both code and spec: either (A) change the handler to build and return the
created UserApprovalRecord (or an array of records when multiple environmentIds
are processed) and use 201 for creation, ensuring the JSON shape matches
apps/api/openapi/openapi.json, or (B) update apps/api/openapi/openapi.json to
change the 200/201 response schema to an acknowledgment like { success: boolean
} and keep the current response; implement the chosen change consistently
(update tests/types/comments) so runtime behavior and OpenAPI stay in sync.
| const { workspaceId, deploymentVersionId } = req.params; | ||
| if (req.apiContext == null) throw new ApiError("Unauthorized", 401); | ||
| const { user } = req.apiContext; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Authorization: confirm user can approve within this workspace/version.
You only check presence of apiContext. Ensure the user belongs to workspace and is permitted to approve for this version/environments to avoid cross-workspace writes.
🏁 Script executed:
#!/bin/bash
# Verify router mount path and check for any permission middleware in the chain.
rg -n -C3 'deploymentVersionIdRouter|upsertUserApprovalRecord|mergeParams:\s*true' apps/api/src/routes/v1/workspaces
rg -n -C3 'authorize|permission|canApprove|ensureWorkspaceMember' apps/api/src/routes apps/api/src/middlewareLength of output: 8143
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete upsertUserApprovalRecord function and check for workspace/permission checks
rg -n -A30 'const upsertUserApprovalRecord' apps/api/src/routes/v1/workspaces/deployment-versions.ts
# Check what the WorkspaceEngine schema looks like and if there's any implicit validation
rg -n -C5 'upsertUserApprovalRecord|WorkspaceEngine.*UserApprovalRecord' apps/api/src
Length of output: 5017
🏁 Script executed:
#!/bin/bash
# Check the deploymentVersionIdRouter and parent router setup for middleware
rg -n -B10 -A10 'deploymentVersionIdRouter.*Router.*mergeParams' apps/api/src/routes/v1/workspaces/deployment-versions.ts
# Check other handlers in same file to see if they validate workspace membership
rg -n -B5 'workspaceId.*req.params|ensureWorkspaceMember|workspace.*validation' apps/api/src/routes/v1/workspaces/deployment-versions.ts
# Check how this router is mounted in the parent to see if middleware chains
rg -n -B5 -A5 'deploymentVersionIdRouter' apps/api/src/routes/v1/workspaces/index.tsLength of output: 1830
🏁 Script executed:
#!/bin/bash
# Check the root workspaces router setup and any middleware applied
rg -n -B10 -A5 'workspacesRouter|app\.use.*workspaces' apps/api/src/routes
# Check app setup to see if there's global middleware validating workspace access
rg -n 'app\.use|middleware.*workspace|authorize.*workspace' apps/api/src/index.ts apps/api/src/server.ts apps/api/src/app.ts 2>/dev/null | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find app entry point
fd -type f -name '*.ts' -path '*/api/src/*' | grep -E '(main|index|server|app)\.ts$' | head -5
# Check workspaces router export and mounting
rg -n 'export.*workspacesRouter|workspacesRouter.*Router' apps/api/src/routes/v1/workspaces/index.ts
# Search for any workspace-level authorization patterns
rg -n 'workspaceId.*middleware|workspace.*authenticate|workspace.*authorize' apps/api/src/routes/v1/workspaces/ -t tsLength of output: 279
🏁 Script executed:
#!/bin/bash
# Find app entry point - correct syntax
find apps/api/src -maxdepth 2 -name '*.ts' | grep -E '(main|index|server|app)\.ts$'
# Check workspacesRouter definition
rg -n 'export.*workspacesRouter' apps/api/src/routes/v1/workspaces/index.ts
# Check if handlers.ts has workspace authorization
rg -n 'workspaceId|authorize|permission' apps/api/src/routes/v1/workspaces/handlers.ts -B3 -A3
# Search for how v1 router mounts workspacesRouter
rg -n 'workspacesRouter' apps/api/src/routes/v1/index.tsLength of output: 4075
Based on verification of the codebase, I can confirm the review comment is valid and addresses a real security concern.
The search revealed that other handlers in the same handlers.ts file (such as getWorkspace, updateWorkspace, and deleteWorkspace) implement workspace authorization by checking if the user is an admin or has an entityRole for that specific workspace. However, upsertUserApprovalRecord only verifies that req.apiContext exists—it never validates that the authenticated user actually belongs to the workspace or has permission to approve deployments within it.
This means any authenticated user could construct a request with an arbitrary workspaceId and create approval records for workspaces they don't have access to.
🤖 Prompt for AI Agents
In apps/api/src/routes/v1/workspaces/deployment-versions.ts around lines 124 to
127, the handler only checks req.apiContext exists and does not verify the
authenticated user's permission for the target workspace; add the same workspace
authorization used by neighboring handlers (e.g., check if user.isAdmin or user
has an entityRole for workspaceId) before proceeding, and if the check fails
throw new ApiError("Forbidden", 403); implement this check immediately after
extracting workspaceId and user so unauthorized users cannot upsert approval
records for arbitrary workspaces.
| .use("/:workspaceId/deploymentversions", deploymentVersionsRouter) | ||
| .use( | ||
| "/:workspaceId/deploymentversions/:deploymentVersionId", | ||
| deploymentVersionIdRouter, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Path naming inconsistency detected.
The new routes use /deploymentversions (no hyphen), but the existing deployment version endpoint in apps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet at line 16 uses /deployment-versions (with hyphen). This inconsistency may cause confusion and potential issues with API discoverability.
Please verify which naming convention should be used and ensure consistency across all endpoints. Run the following script to identify all deployment version path patterns:
🏁 Script executed:
#!/bin/bash
# Search for deployment version path patterns across the codebase
echo "=== Paths with hyphen (deployment-versions) ==="
rg -n "deployment-versions" --type-add 'config:*.{jsonnet,ts,go,json}' --type config
echo -e "\n=== Paths without hyphen (deploymentversions) ==="
rg -n "deploymentversions" --type-add 'config:*.{jsonnet,ts,go,json}' --type configLength of output: 15213
Path naming inconsistency requires standardization.
The new routes use /deploymentversions (no hyphen), but the codebase exhibits inconsistent naming conventions:
- All root-level deployment version endpoints use hyphenated paths:
/v1/deployment-versions/... - Workspace-nested endpoints are mixed:
/v1/workspaces/{workspaceId}/deployment-versions/{versionId}/jobs-list(with hyphen)/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}(without hyphen)
The new routes at lines 41–45 follow the non-hyphenated pattern, creating inconsistency alongside existing hyphenated workspace-nested paths. Standardize these to use hyphens (/:workspaceId/deployment-versions) to align with the established REST API naming convention.
🤖 Prompt for AI Agents
In apps/api/src/routes/v1/workspaces/index.ts around lines 41 to 45, the mounted
routes use the non-hyphenated path segment "deploymentversions", causing
inconsistency with the rest of the API; change the routes to use the hyphenated
form "/:workspaceId/deployment-versions" and
"/:workspaceId/deployment-versions/:deploymentVersionId" to match existing
endpoints, and update any related route references, tests, and docs that assume
the old non-hyphenated paths.
| dependsOnEnvironmentSelector: components["schemas"]["Selector"]; | ||
| id: string; | ||
| /** | ||
| * Format: int32 | ||
| * @description Maximum age of dependency deployment before blocking progression (prevents stale promotions) | ||
| */ | ||
| maximumAgeHours?: number; | ||
| /** | ||
| * Format: int32 | ||
| * @description Minimum time to wait after the depends on environment is in a success state before the current environment can be deployed | ||
| * @default 0 | ||
| */ | ||
| minimumSockTimeMinutes: number; | ||
| /** | ||
| * Format: float | ||
| * @default 100 | ||
| */ | ||
| minimumSuccessPercentage: number; |
There was a problem hiding this comment.
Typo: minimumSockTimeMinutes
Likely intended “minimumSoakTimeMinutes”. If so, fix in the OpenAPI source and regenerate to avoid propagating a misspelled public field.
🤖 Prompt for AI Agents
In apps/web/app/api/openapi.ts around lines 349 to 366, the schema property is
misspelled as minimumSockTimeMinutes but should be minimumSoakTimeMinutes;
update the property name in the OpenAPI source (rename minimumSockTimeMinutes →
minimumSoakTimeMinutes), adjust any references/usages to the new name, keep the
same type, default and description, and then regenerate the OpenAPI
types/artifacts so the corrected field propagates through the generated code and
API surface.
| UpsertUserApprovalRecordRequest: { | ||
| environmentId: string; | ||
| reason?: string; | ||
| status: components["schemas"]["ApprovalStatus"]; | ||
| userId: string; | ||
| versionId: string; | ||
| }; |
There was a problem hiding this comment.
Do not accept userId/versionId in request; derive from auth/path
The request currently is:
{ environmentId, reason?, status, userId, versionId }
Remove userId and versionId; they must come from auth and the version path to prevent spoofing. If approvals can target multiple environments, decide on singular environmentId vs environmentIds[] and standardize with the server file.
Apply this diff:
- UpsertUserApprovalRecordRequest: {
- environmentId: string;
- reason?: string;
- status: components["schemas"]["ApprovalStatus"];
- userId: string;
- versionId: string;
- };
+ UpsertUserApprovalRecordRequest: {
+ environmentId: string;
+ reason?: string;
+ status: components["schemas"]["ApprovalStatus"];
+ };📝 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.
| UpsertUserApprovalRecordRequest: { | |
| environmentId: string; | |
| reason?: string; | |
| status: components["schemas"]["ApprovalStatus"]; | |
| userId: string; | |
| versionId: string; | |
| }; | |
| UpsertUserApprovalRecordRequest: { | |
| environmentId: string; | |
| reason?: string; | |
| status: components["schemas"]["ApprovalStatus"]; | |
| }; |
🤖 Prompt for AI Agents
In apps/web/app/api/openapi.ts around lines 548 to 554, the
UpsertUserApprovalRecordRequest schema incorrectly accepts userId and versionId
from the client; remove userId and versionId properties from this request shape
so those values are derived server-side from the authenticated user and the
request path, and reconcile the environment identifier shape with the server
implementation (either change environmentId to environmentIds[] or vice‑versa)
to match the server file—update the OpenAPI schema to reflect the final
decision, run any dependent typegen/validation tasks, and ensure the server
handler no longer reads userId/versionId from the body but uses auth and path
values instead.
| listDeployments: { | ||
| parameters: { | ||
| query?: { | ||
| /** @description Maximum number of items to return */ | ||
| limit?: number; | ||
| /** @description Number of items to skip */ | ||
| offset?: number; | ||
| }; | ||
| header?: never; | ||
| path: { | ||
| /** @description ID of the workspace */ | ||
| workspaceId: string; | ||
| }; | ||
| cookie?: never; | ||
| }; | ||
| requestBody?: never; | ||
| responses: { | ||
| /** @description Paginated list of items */ | ||
| 200: { | ||
| headers: { | ||
| [name: string]: unknown; | ||
| }; | ||
| content: { | ||
| "application/json": { | ||
| items: components["schemas"]["DeploymentAndSystem"][]; | ||
| /** @description Maximum number of items returned */ | ||
| limit: number; | ||
| /** @description Number of items skipped */ | ||
| offset: number; | ||
| /** @description Total number of items available */ | ||
| total: number; | ||
| }; | ||
| }; | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Ops wiring LGTM; one missing op
These operations look correct. However, getDeploymentVersion is missing in this file. Regenerate from the canonical spec to include it.
Run:
Also applies to: 980-1021, 1023-1078, 1079-1129, 1130-1165, 1166-1189, 1190-1232, 1233-1263, 1264-1305, 1306-1331, 1332-1369
🏁 Script executed:
#!/bin/bash
rg -nP '^\\s*getDeploymentVersion\\b' apps/web/app/api/openapi.ts || echo "missing"Length of output: 7
Regenerate openapi.ts from canonical spec to include missing getDeploymentVersion operation
Verification confirms getDeploymentVersion is missing from the file. Run regeneration from the canonical spec to add this operation.
🤖 Prompt for AI Agents
In apps/web/app/api/openapi.ts around lines 920 to 955 the generated OpenAPI
client is missing the getDeploymentVersion operation; regenerate the file from
the canonical OpenAPI specification so the operation is included. Run the
repository's OpenAPI generation command (e.g., the project’s openapi/gen script
or the specified generator CLI) against the canonical spec, replace the updated
apps/web/app/api/openapi.ts with the regenerated output, verify
getDeploymentVersion appears and typings compile, and commit the regenerated
file.
| upsertUserApprovalRecord: { | ||
| parameters: { | ||
| query?: never; | ||
| header?: never; | ||
| path?: never; | ||
| cookie?: never; | ||
| }; | ||
| requestBody: { | ||
| content: { | ||
| "application/json": components["schemas"]["UpsertUserApprovalRecordRequest"]; | ||
| }; | ||
| }; | ||
| responses: { | ||
| /** @description OK response */ | ||
| 200: { | ||
| headers: { | ||
| [name: string]: unknown; | ||
| }; | ||
| content: { | ||
| "application/json": components["schemas"]["UserApprovalRecord"]; | ||
| }; | ||
| }; | ||
| /** @description Resource created successfully */ | ||
| 201: { | ||
| headers: { | ||
| [name: string]: unknown; | ||
| }; | ||
| content: { | ||
| "application/json": components["schemas"]["UserApprovalRecord"]; | ||
| }; | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Incomplete responses; align with API’s 400/404 and shared ErrorResponse
Server defines 200/201/400/404 for upsertUserApprovalRecord. Add 400 and 404 (using ErrorResponse) for parity and predictable error typing.
Proposed diff:
responses: {
/** @description OK response */
/** @description Resource created successfully */
+ /** @description Invalid request */
+ 400: {
+ headers: { [name: string]: unknown; };
+ content: { "application/json": components["schemas"]["ErrorResponse"]; };
+ },
+ /** @description Resource not found */
+ 404: {
+ headers: { [name: string]: unknown; };
+ content: { "application/json": components["schemas"]["ErrorResponse"]; };
+ }
};📝 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.
| upsertUserApprovalRecord: { | |
| parameters: { | |
| query?: never; | |
| header?: never; | |
| path?: never; | |
| cookie?: never; | |
| }; | |
| requestBody: { | |
| content: { | |
| "application/json": components["schemas"]["UpsertUserApprovalRecordRequest"]; | |
| }; | |
| }; | |
| responses: { | |
| /** @description OK response */ | |
| 200: { | |
| headers: { | |
| [name: string]: unknown; | |
| }; | |
| content: { | |
| "application/json": components["schemas"]["UserApprovalRecord"]; | |
| }; | |
| }; | |
| /** @description Resource created successfully */ | |
| 201: { | |
| headers: { | |
| [name: string]: unknown; | |
| }; | |
| content: { | |
| "application/json": components["schemas"]["UserApprovalRecord"]; | |
| }; | |
| }; | |
| }; | |
| }; | |
| upsertUserApprovalRecord: { | |
| parameters: { | |
| query?: never; | |
| header?: never; | |
| path?: never; | |
| cookie?: never; | |
| }; | |
| requestBody: { | |
| content: { | |
| "application/json": components["schemas"]["UpsertUserApprovalRecordRequest"]; | |
| }; | |
| }; | |
| responses: { | |
| /** @description OK response */ | |
| 200: { | |
| headers: { | |
| [name: string]: unknown; | |
| }; | |
| content: { | |
| "application/json": components["schemas"]["UserApprovalRecord"]; | |
| }; | |
| }; | |
| /** @description Resource created successfully */ | |
| 201: { | |
| headers: { | |
| [name: string]: unknown; | |
| }; | |
| content: { | |
| "application/json": components["schemas"]["UserApprovalRecord"]; | |
| }; | |
| }; | |
| /** @description Invalid request */ | |
| 400: { | |
| headers: { [name: string]: unknown; }; | |
| content: { "application/json": components["schemas"]["ErrorResponse"]; }; | |
| }, | |
| /** @description Resource not found */ | |
| 404: { | |
| headers: { [name: string]: unknown; }; | |
| content: { "application/json": components["schemas"]["ErrorResponse"]; }; | |
| } | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In apps/web/app/api/openapi.ts around lines 1370 to 1402, the
upsertUserApprovalRecord operation currently declares only 200 and 201 responses
but the server also returns 400 and 404; add 400 and 404 response entries using
the shared ErrorResponse schema to match server behavior and ensure predictable
error typing; update the responses object to include 400 and 404 with
appropriate headers and content referencing
components["schemas"]["ErrorResponse"] so generated types include these error
cases.
| '/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}': { | ||
| get: { | ||
| summary: 'Get deployment version', | ||
| operationId: 'getDeploymentVersion', | ||
| description: 'Returns a deployment version by ID.', | ||
| parameters: [ | ||
| openapi.workspaceIdParam(), | ||
| openapi.deploymentVersionIdParam(), | ||
| ], | ||
| responses: openapi.okResponse(openapi.schemaRef('DeploymentVersion')) + openapi.notFoundResponse() + openapi.badRequestResponse(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Inconsistent path naming within the same file.
The new endpoint uses /deploymentversions/{deploymentVersionId} (no hyphen, line 4) while the existing endpoint at line 16 uses /deployment-versions/{versionId} (with hyphen). Additionally, the parameter names differ (deploymentVersionId vs versionId).
For consistency, the new endpoint should either:
- Use
/deployment-versions/{deploymentVersionId}to match the existing pattern, or - The existing endpoint should be updated to use
/deploymentversions/without the hyphen
This inconsistency is also present across the API surface (see comment on apps/api/src/routes/v1/workspaces/index.ts).
🤖 Prompt for AI Agents
In apps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet around lines
4 to 15, the new route path and parameter naming are inconsistent with the
existing endpoint (hyphenated path and different param name); update the new
endpoint to use the same hyphenated path and parameter naming convention: change
'/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}' to
'/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}' and
ensure the path parameter name matches any existing definitions (use
deploymentVersionId everywhere) so the OpenAPI paths and parameter references
remain consistent across the file and API surface.
Summary by CodeRabbit
Release Notes