fix: approvals should be scoped to an environment #593
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThis change introduces environment-scoped approval and rejection for deployment versions across the API, database schema, rule engine, and frontend. New endpoints allow approvals and rejections per environment. Database tables and rule logic are updated to store and evaluate approvals with environment context. The frontend is refactored to support multi-environment approval workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant DB
participant RuleEngine
User->>Frontend: Selects environments and submits approval
Frontend->>API: POST /approve/environment/{envId}
API->>DB: Insert approval record (deploymentVersionId, environmentId, userId)
DB-->>API: Confirmation
API->>RuleEngine: Evaluate release targets for deployment/environment
RuleEngine->>DB: Fetch approval records for environment
RuleEngine-->>API: Evaluation result
API-->>Frontend: Return approval record
Frontend-->>User: Show approval status
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{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...
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
packages/db/src/schema/rules/approval-base.ts (1)
59-64: Add environmentId to validation schema.The
baseApprovalRecordValidationFieldsshould include validation for the newenvironmentIdfield to maintain consistency with the database schema.export const baseApprovalRecordValidationFields = { deploymentVersionId: z.string().uuid(), + environmentId: z.string().uuid(), userId: z.string().uuid(), status: z.enum(approvalStatus.enumValues), reason: z.string().optional(), };
🧹 Nitpick comments (6)
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/route.ts (1)
38-43: Verify nested query performanceThe nested query with deep relations (
deployment -> system -> environments) could impact performance for systems with many environments. Consider adding database indexes on the foreign key relationships if not already present.packages/rule-engine/src/rules/version-approval-rule.ts (1)
41-42: Remove debug logging before production.Console.log statements should be removed or replaced with proper logging infrastructure before merging to production.
- console.log("num approval records", approvalRecords.length); - console.log("min required", this.options.minApprovals);apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx (1)
119-121: Consider validation for empty environment selection.The component initializes with the passed
environmentId, but users can remove all environments. Consider adding validation to prevent submitting with no environments selected.const ApprovalDialogControl: React.FC<{ versionId: string; environments: schema.Environment[]; environmentId: string; onSubmit: () => void; onCancel: () => void; }> = ({ versionId, environments, environmentId, onSubmit, onCancel }) => { const [environmentIds, setEnvironmentIds] = useState<string[]>([ environmentId, ]); + + const hasSelectedEnvironments = environmentIds.length > 0;Then update the approve/reject buttons:
<Button variant="outline" onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Rejected)} - disabled={addRecord.isPending} + disabled={addRecord.isPending || !hasSelectedEnvironments} > Reject </Button> <Button onClick={() => handleSubmit(SCHEMA.ApprovalStatus.Approved)} - disabled={addRecord.isPending} + disabled={addRecord.isPending || !hasSelectedEnvironments} > Approve </Button>openapi.v1.json (3)
232-244: Add description forenvironmentIdparameter
TheenvironmentIdpath parameter lacks adescriptionfield. Please add a descriptive text to aid client code generation and API documentation.{ "name": "environmentId", "in": "path", "required": true, "schema": { "type": "string", "format": "uuid" }, + "description": "UUID of the target environment" }
639-648: Add description forenvironmentIdparameter
Include adescriptionfor theenvironmentIdpath parameter in the rejection endpoint to improve the OpenAPI docs.{ "name": "environmentId", "in": "path", "required": true, "schema": { "type": "string", "format": "uuid" }, + "description": "UUID of the target environment" }
5467-5471: Add description forenvironmentIdinApprovalRecord
The newly addedenvironmentIdproperty in theApprovalRecordschema lacks adescription. Please document its purpose."environmentId": { "type": "string", "format": "uuid", + "description": "UUID of the environment associated with this approval" },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/event-worker/src/workers/compute-systems-release-targets.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ApprovalRequiredCell.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx(2 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/environment/[environmentId]/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/environment/[environmentId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/openapi.ts(2 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/route.ts(2 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/environment/[environmentId]/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/environment/[environmentId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/route.ts(2 hunks)e2e/api/schema.ts(5 hunks)e2e/tests/api/policies/approval-policy.spec.ts(1 hunks)e2e/tests/api/policies/approval-policy.spec.yaml(2 hunks)openapi.v1.json(4 hunks)packages/api/src/router/deployment-version.ts(3 hunks)packages/api/src/router/policy/evaluate.ts(1 hunks)packages/db/drizzle/0117_little_the_santerians.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/rules/approval-any.ts(1 hunks)packages/db/src/schema/rules/approval-base.ts(2 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(1 hunks)packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts(4 hunks)packages/rule-engine/src/manager/version-manager-rules/version-approval.ts(1 hunks)packages/rule-engine/src/rules/version-approval-rule.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{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.
apps/event-worker/src/workers/compute-systems-release-targets.tsapps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/route.tsapps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/openapi.tsapps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/route.tspackages/rule-engine/src/manager/version-manager-rules.tspackages/api/src/router/policy/evaluate.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsxpackages/db/src/schema/rules/approval-any.tsapps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/environment/[environmentId]/route.tsapps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/environment/[environmentId]/openapi.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ApprovalRequiredCell.tsxe2e/tests/api/policies/approval-policy.spec.tsapps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/environment/[environmentId]/openapi.tspackages/rule-engine/src/rules/version-approval-rule.tsapps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/environment/[environmentId]/route.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsxpackages/db/src/schema/rules/approval-base.tspackages/api/src/router/deployment-version.tspackages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.tspackages/rule-engine/src/manager/version-manager-rules/version-approval.tse2e/api/schema.ts
🧬 Code Graph Analysis (7)
apps/event-worker/src/workers/compute-systems-release-targets.ts (1)
packages/logger/src/index.ts (1)
logger(48-48)
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/route.ts (2)
packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(110-137)packages/db/src/schema/environment.ts (1)
environment(59-84)
packages/api/src/router/policy/evaluate.ts (1)
packages/rule-engine/src/manager/version-manager-rules/version-approval.ts (3)
versionUserApprovalRule(41-53)versionRoleApprovalRule(27-39)versionAnyApprovalRule(14-25)
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/environment/[environmentId]/openapi.ts (2)
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/environment/[environmentId]/openapi.ts (1)
openapi(3-86)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/openapi.ts (1)
openapi(3-112)
packages/rule-engine/src/rules/version-approval-rule.ts (1)
packages/db/src/client.ts (1)
db(15-15)
packages/api/src/router/deployment-version.ts (1)
packages/events/src/dispatch-jobs.ts (1)
dispatchQueueJob(154-158)
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts (5)
packages/rule-engine/src/types.ts (1)
Policy(66-74)packages/rule-engine/src/manager/version-rule-engine.ts (1)
Version(11-16)packages/rule-engine/src/manager/version-manager-rules/version-approval.ts (1)
getVersionApprovalRules(55-70)packages/db/src/common.ts (1)
takeFirst(9-13)packages/db/src/schema/release.ts (1)
releaseTarget(21-43)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (54)
apps/event-worker/src/workers/compute-systems-release-targets.ts (1)
14-14: Stylistic simplification is safe and clear.Inlining the
logger.childobject literal doesn’t alter behavior and improves conciseness. No functional impact detected.packages/db/drizzle/meta/_journal.json (1)
824-830: LGTM! Migration journal entry is correctly formatted.The new journal entry follows the standard format and properly tracks the schema changes for environment-scoped approvals.
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/openapi.ts (1)
13-13: LGTM! Environment ID correctly added to schema.The
environmentIdfield is properly defined with UUID format and marked as required, which aligns with the environment-scoped approval functionality.Also applies to: 24-24
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/route.ts (2)
59-63: LGTM! Bulk insert with conflict handling is well implemented.The bulk insert operation with
onConflictDoNothing()properly handles potential duplicate rejection attempts and returns the inserted records.
47-57: ```shell
#!/bin/bashLocate the approve route for deployment-version approvals and print its implementation
approve_route=$(find apps/webservice/src/app/api/v1/deployment-versions -type f -name route.ts | grep approve)
echo "Approve route file: $approve_route"
echo "----- Snippet (lines 1-200) -----"
sed -n '1,200p' "$approve_route"</details> <details> <summary>apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (2)</summary> `10-10`: **LGTM! Import path improvement.** Using absolute import path improves maintainability and reduces issues with relative path changes. --- `27-27`: **LGTM! System prop type expansion aligns with feature requirements.** Adding `id` to the system prop type provides the necessary context for environment-scoped operations while maintaining backward compatibility with the existing `slug` property. </details> <details> <summary>apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ApprovalRequiredCell.tsx (2)</summary> `77-77`: **LGTM! System prop type expansion supports environment-scoped functionality.** Adding `id` to the system prop type provides the necessary context for the ApprovalDialog to fetch environments by system ID. --- `146-146`: **LGTM! System ID correctly passed to ApprovalDialog.** Passing `system.id` enables the ApprovalDialog to fetch environments for multi-environment approval workflows. </details> <details> <summary>packages/rule-engine/src/manager/version-manager-rules.ts (2)</summary> `73-76`: **LGTM - Good async handling pattern** The change to call `getVersionApprovalRules` asynchronously and store the result in a variable before spreading is a good practice for async operations. This aligns with the environment-scoped approval changes that require database queries to fetch environment context. --- `78-81`: **Minor formatting improvement** The condensed formatting of `ReleaseTargetLockRule` and the spread of `versionApprovalRules` maintains consistency with the async pattern changes. </details> <details> <summary>apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx (1)</summary> `117-117`: ```shell #!/bin/bash set -e # Inspect system prop assignment in DeploymentPageContent.tsx rg -C3 -F -n 'system={{' 'apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/DeploymentPageContent.tsx' # Inspect system data fetch usage in DeploymentPageContent.tsx rg -C3 -n 'api.system' 'apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/DeploymentPageContent.tsx' # Inspect the props type for DeploymentEnvironmentCell rg -C3 -n 'type DeploymentEnvironmentCellProps' 'apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx' # Inspect 'system:' declaration inside DeploymentEnvironmentCellProps rg -C3 -n 'system:' 'apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx'apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/route.ts (2)
68-70: Verify conflict handling with environment-scoped recordsThe
onConflictDoNothing()behavior should now work correctly with the updated unique index that includesenvironmentId. Ensure this prevents duplicate approvals per environment while allowing the same user to approve for different environments.
57-64: Potential scaling concern with bulk environment approvalsCreating approval records for all environments in a system could lead to excessive records for systems with many environments. Consider whether this bulk approval approach aligns with the intended user workflow.
#!/bin/bash # Description: Check if there are limits on number of environments per system # Expected: Find any validation or limits on environment count rg -A 5 -B 5 "environments.*length|environments.*count|environment.*limit" --type ts --type tsxe2e/tests/api/policies/approval-policy.spec.yaml (2)
15-22: Good test setup for multi-environment scenariosAdding a second environment with the same configuration enables testing environment-scoped approval logic. The naming pattern supports the policy selector changes.
42-43: Policy selector change appropriately broadens scopeChanging from "equals" to "contains" allows the policy to match both environments (
{{ prefix }}and{{ prefix }}-prod), which is necessary for testing multi-environment approval workflows.packages/db/src/schema/rules/approval-any.ts (1)
33-39: Correct implementation of environment-scoped unique constraintThe updated unique index properly enforces the constraint that a user can only approve a deployment version once per environment. The index name
"unique_deployment_version_id_environment_id_user_id"is descriptive and follows good naming conventions.packages/api/src/router/policy/evaluate.ts (1)
125-126: LGTM! Environment context properly integrated into approval rules.The changes correctly pass the
environmentIdparameter to the approval rule functions, enabling environment-scoped policy evaluation. The parameter order and types align with the updated function signatures in the rule engine.Also applies to: 133-134, 141-142
packages/db/src/schema/rules/approval-base.ts (2)
6-7: LGTM! Proper imports added for foreign key references.The imports for
deploymentVersionandenvironmentare correctly added to support the new foreign key relationships.
20-26: LGTM! Well-designed foreign key relationships with proper cascade behavior.The schema changes properly establish:
- Explicit foreign key reference for
deploymentVersionIdwith cascade delete- New required
environmentIdfield with cascade delete behaviorThis ensures referential integrity and automatic cleanup when parent records are deleted.
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/environment/[environmentId]/openapi.ts (1)
1-86: LGTM! Well-structured OpenAPI specification for environment-scoped approval.The specification properly defines:
- Required UUID path parameters for both deployment version and environment
- Appropriate request body schema with mandatory
reasonfield- Comprehensive response definitions covering success and error scenarios
- Correct reference to the shared
ApprovalRecordschema componentThe API design follows RESTful patterns and maintains consistency with existing endpoints.
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/environment/[environmentId]/route.ts (1)
35-98: LGTM! Well-implemented environment-scoped rejection endpoint.The implementation demonstrates excellent practices:
- Proper middleware stack with authentication, authorization, and body parsing
- Comprehensive validation of deployment version and environment existence
- Sound business logic requiring existing approval before rejection
- Appropriate use of
onConflictDoNothing()for idempotent operations- Clear error responses with meaningful messages
The authorization check using
DeploymentVersionUpdatepermission is appropriate for this operation.apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/environment/[environmentId]/openapi.ts (1)
1-86: LGTM! OpenAPI specification follows consistent patterns.The specification properly defines the environment-scoped rejection endpoint with:
- Appropriate path parameters and validation
- Correct HTTP status codes and error responses
- Proper reference to the shared
ApprovalRecordschemaThe structure maintains consistency with the corresponding approval endpoint.
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/environment/[environmentId]/route.ts (6)
15-18: LGTM! Well-structured request validation.The body schema correctly defines optional
reasonandapprovedAtfields with proper validation using Zod datetime parsing.
20-35: Good separation of concerns with the helper function.The
getExistingRecordfunction properly encapsulates the duplicate approval check logic and correctly queries by all three required fields: deploymentVersionId, environmentId, and userId.
54-72: LGTM! Proper entity validation with clear error responses.Both deployment version and environment existence checks follow consistent patterns with appropriate 404 responses.
90-102: Good use of conflict handling in database insert.The
.onConflictDoNothing()provides additional safety against race conditions, complementing the explicit duplicate check above.
103-113: LGTM! Proper job dispatch for rule evaluation.The code correctly finds all affected release targets for the specific deployment and environment combination, then triggers rule evaluation.
41-46: Verify authorization covers environment access.The current authorization only checks
DeploymentVersionUpdatepermission on the deployment version. Consider whether users should also have specific permissions on the target environment before approving.#!/bin/bash # Check if environment-specific permissions are enforced elsewhere ast-grep --pattern $'can.perform($_).on({ type: "environment", $$$ })'packages/rule-engine/src/rules/version-approval-rule.ts (3)
71-87: LGTM! Well-structured environment-scoped getter function.The
getAnyApprovalRecordsGetterproperly combines database conditions usingand()and correctly maps the result to includeversionId.
104-120: LGTM! Consistent pattern with role approval records.The
getRoleApprovalRecordsGetterfollows the same pattern as the any approval getter, maintaining consistency across approval types.
137-153: LGTM! Complete coverage for user approval records.The
getUserApprovalRecordsGettercompletes the set of environment-scoped getters, following the established pattern.e2e/tests/api/policies/approval-policy.spec.ts (2)
283-377: Excellent test coverage for environment-scoped approvals.This test properly verifies that:
- Approving a version for environment1 creates a release for that environment
- The same approval does NOT create a release for environment2
- The API correctly handles environment-specific approval endpoints
The test structure is clear and assertions are appropriate.
379-511: LGTM! Thorough test for duplicate approval prevention.This test effectively validates:
- First approval succeeds (200 status)
- Duplicate approval is rejected (409 status)
- No release is created when minimum approvals aren't met (policy requires 2 approvals)
The test setup with a new policy and environment is well-structured.
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts (4)
22-28: Good refactor to support environment-scoped approval rules.The function now properly accepts
environmentIdand awaits the asyncgetVersionApprovalRulescall, making the approval rules environment-aware.
129-135: LGTM! Clean helper function for environment ID retrieval.The
getEnvironmentIdfunction follows the established pattern usingtakeFirstto ensure a unique result and properly extracts the environment ID from the release target.
147-148: LGTM! Proper integration of environment context.The code correctly fetches the environment ID and passes it to the rollout start time getter, maintaining the environment-scoped behavior.
186-190: LGTM! Consistent use of environment context in rollout info.The function properly extracts
environmentIdfrom the release target and passes it to the rollout start time getter, maintaining consistency with the other changes.packages/db/drizzle/0117_little_the_santerians.sql (4)
5-7: LGTM! Consistent schema changes across all approval tables.All three approval record tables (
policy_rule_user_approval_record,policy_rule_role_approval_record,policy_rule_any_approval_record) consistently receive theenvironment_idcolumn with NOT NULL constraint.
8-13: LGTM! Proper foreign key constraints with cascade deletion.The foreign key constraints properly reference both
deployment_versionandenvironmenttables withON DELETE cascadeto maintain referential integrity.
14-14: LGTM! Updated unique constraint for environment scoping.The new unique index correctly includes
deployment_version_id,environment_id, anduser_id, ensuring users can only approve a deployment version once per environment while allowing multiple approvals across different environments.
1-3: Breaking change: Verify data loss is acceptable.The migration deletes all existing approval records. Ensure this data loss is intentional and stakeholders are aware of this breaking change.
What are the best practices for handling breaking database migrations that delete existing data?packages/api/src/router/deployment-version.ts (3)
345-345: LGTM! Input schema correctly updated for multi-environment support.The change from single
environmentIdtoenvironmentIdsarray properly supports the new batch approval functionality.
358-368: Well-implemented batch record creation logic.The mapping approach to create multiple approval records is clean and handles the multi-environment scenario correctly. Each record gets the same approval metadata but different environment IDs.
389-391: ```shell
#!/bin/bashDisplay full context around the conditional to understand its usage
sed -n '340,420p' packages/api/src/router/deployment-version.ts
</details> <details> <summary>apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx (2)</summary> `31-110`: **Well-structured multi-environment selection component.** The `EnvironmentCombobox` provides good UX with badges for selected environments and a searchable popover. The "All environments" option is a nice touch. --- `204-204`: Here’s a deeper sweep to locate the `bySystemId` query and confirm the existence of an “environment” router. Please run this from the repo root: ```shell #!/usr/bin/env bash echo "⮕ Searching for any 'bySystemId' references:" rg -n "bySystemId" || echo "No occurrences found." echo echo "⮕ Finding all tRPC routers and filtering for 'environment':" rg -n "createTRPCRouter" | rg "environment" || echo "No environment router definitions found." echo echo "⮕ Listing all .ts files with 'environment' in their filename:" fd -e ts environment || echo "No files matching '*environment*.ts'." echo echo "⮕ Inspecting those files for '.query(' definitions:" for file in $(fd -e ts environment); do echo "---- $file ----" rg -n "\.query\(" "$file" || echo " (no queries in this file)" donepackages/rule-engine/src/manager/version-manager-rules/version-approval.ts (2)
14-25: LGTM! Approval rule properly parameterized with environment context.The
versionAnyApprovalRulefunction correctly acceptsenvironmentIdand passes it to the environment-specific getter function.
55-58: ```shell
#!/bin/bashSearching for all call sites of getVersionApprovalRules across TS/JS files
echo "Searching for getVersionApprovalRules usage in .ts/.tsx/.js/.jsx files..."
rg -n "getVersionApprovalRules" -g ".ts" -g ".tsx" -g ".js" -g ".jsx" -C 3 || true</details> <details> <summary>e2e/api/schema.ts (5)</summary> `44-60`: **LGTM: Well-structured environment-scoped approval endpoint** The new endpoint for environment-scoped deployment version approval follows proper RESTful patterns and maintains consistency with existing API structure. --- `112-128`: **LGTM: Consistent environment-scoped rejection endpoint** The rejection endpoint mirrors the approval endpoint structure, maintaining API consistency and providing symmetric functionality for environment-scoped operations. --- `1039-1039`: **LGTM: Appropriate schema extension for environment scoping** Adding the `environmentId` field to the `ApprovalRecord` schema with proper UUID formatting enables tracking approval records per environment, which is essential for the environment-scoped approval feature. --- `1679-1742`: **LGTM: Comprehensive operation definition with proper error handling** The `approveDeploymentVersionForEnvironment` operation is well-defined with: - Correct path parameters for both deployment version and environment - Appropriate request body structure with required `reason` field - Comprehensive error response coverage (403, 404, 500) - Consistent return type using `ApprovalRecord` schema --- `1925-1988`: **LGTM: Symmetric reject operation maintains API consistency** The rejection operation properly mirrors the approval operation structure, ensuring consistent behavior across the environment-scoped approval workflow. The request/response patterns align with existing API conventions. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
32
to
34
| reason: { type: "string" }, | ||
| approvedAt: { type: "string", format: "date-time" }, | ||
| }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove inappropriate approvedAt field from rejection request body.
The approvedAt field doesn't make sense in a rejection request body since rejections should have approvedAt: null. This field should be removed from the rejection endpoint's request schema.
schema: {
type: "object",
properties: {
reason: { type: "string" },
- approvedAt: { type: "string", format: "date-time" },
},
required: ["reason"],
},📝 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.
Suggested change
| reason: { type: "string" }, | |
| approvedAt: { type: "string", format: "date-time" }, | |
| }, | |
| schema: { | |
| type: "object", | |
| properties: { | |
| reason: { type: "string" }, | |
| }, | |
| required: ["reason"], | |
| }, |
🤖 Prompt for AI Agents
In
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/reject/environment/[environmentId]/openapi.ts
around lines 32 to 34, remove the `approvedAt` field from the rejection request
body schema because it is inappropriate for rejection requests. Update the
schema to only include relevant fields like `reason` and exclude `approvedAt`
entirely.
Comment on lines
+138
to
+142
| const setEnvironmentSelected = (environmentIds: string[]) => | ||
| setEnvironmentIds((prev) => [...prev, ...environmentIds]); | ||
|
|
||
| const setEnvironmentUnselected = (environmentId: string) => | ||
| setEnvironmentIds((prev) => prev.filter((id) => id !== environmentId)); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with environment selection logic.
The setEnvironmentSelected function appends new IDs without checking for duplicates, while setEnvironmentUnselected properly filters. This could lead to duplicate environment IDs if the same environment is selected multiple times.
- const setEnvironmentSelected = (environmentIds: string[]) =>
- setEnvironmentIds((prev) => [...prev, ...environmentIds]);
+ const setEnvironmentSelected = (environmentIds: string[]) =>
+ setEnvironmentIds((prev) => {
+ const newIds = environmentIds.filter((id) => !prev.includes(id));
+ return [...prev, ...newIds];
+ });📝 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.
Suggested change
| const setEnvironmentSelected = (environmentIds: string[]) => | |
| setEnvironmentIds((prev) => [...prev, ...environmentIds]); | |
| const setEnvironmentUnselected = (environmentId: string) => | |
| setEnvironmentIds((prev) => prev.filter((id) => id !== environmentId)); | |
| const setEnvironmentSelected = (environmentIds: string[]) => | |
| setEnvironmentIds((prev) => { | |
| const newIds = environmentIds.filter((id) => !prev.includes(id)); | |
| return [...prev, ...newIds]; | |
| }); | |
| const setEnvironmentUnselected = (environmentId: string) => | |
| setEnvironmentIds((prev) => prev.filter((id) => id !== environmentId)); |
🤖 Prompt for AI Agents
In
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx
around lines 138 to 142, the setEnvironmentSelected function appends new
environment IDs without checking for duplicates, which can cause duplicate IDs
in the state. Modify setEnvironmentSelected to merge the new IDs with the
existing ones while removing duplicates, for example by using a Set or filtering
duplicates before updating the state.
Contributor
Author
There was a problem hiding this comment.
this probably won't happen and even if it does its a noop since its used in an inArray check in the db
Contributor
There was a problem hiding this comment.
@adityachoudhari26 You're absolutely right! I see the logic now:
- The
unselectedEnvironmentsfilter ensures users can only select from environments not already selected, making natural duplicate selection unlikely - Even in edge cases with duplicates, the database
inArrayoperations would treat them as identical values, making it effectively a no-op
Thanks for the clarification - your approach is sound and the additional complexity of duplicate filtering isn't necessary here.
Comment on lines
+55
to
+70
| export const getVersionApprovalRules = async ( | ||
| policy: Policy | null, | ||
| ): FilterRule<Version>[] => [ | ||
| ...versionUserApprovalRule(policy?.versionUserApprovals), | ||
| ...versionAnyApprovalRule(policy?.versionAnyApprovals), | ||
| ...versionRoleApprovalRule(policy?.versionRoleApprovals), | ||
| ]; | ||
| releaseTargetId: string, | ||
| ): Promise<FilterRule<Version>[]> => { | ||
| const releaseTarget = await db | ||
| .select() | ||
| .from(schema.releaseTarget) | ||
| .where(eq(schema.releaseTarget.id, releaseTargetId)) | ||
| .then(takeFirst); | ||
| const { environmentId } = releaseTarget; | ||
| return [ | ||
| ...versionUserApprovalRule(environmentId, policy?.versionUserApprovals), | ||
| ...versionAnyApprovalRule(environmentId, policy?.versionAnyApprovals), | ||
| ...versionRoleApprovalRule(environmentId, policy?.versionRoleApprovals), | ||
| ]; | ||
| }; |
Contributor
There was a problem hiding this comment.
Verify error handling for missing release target.
The function queries the release target by ID but doesn't handle the case where the release target might not exist. The takeFirst will return undefined if no record is found, which would cause a runtime error when accessing releaseTarget.environmentId.
export const getVersionApprovalRules = async (
policy: Policy | null,
releaseTargetId: string,
): Promise<FilterRule<Version>[]> => {
const releaseTarget = await db
.select()
.from(schema.releaseTarget)
.where(eq(schema.releaseTarget.id, releaseTargetId))
.then(takeFirst);
+
+ if (!releaseTarget) {
+ throw new Error(`Release target not found: ${releaseTargetId}`);
+ }
+
const { environmentId } = releaseTarget;
return [
...versionUserApprovalRule(environmentId, policy?.versionUserApprovals),
...versionAnyApprovalRule(environmentId, policy?.versionAnyApprovals),
...versionRoleApprovalRule(environmentId, policy?.versionRoleApprovals),
];
};📝 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.
Suggested change
| export const getVersionApprovalRules = async ( | |
| policy: Policy | null, | |
| ): FilterRule<Version>[] => [ | |
| ...versionUserApprovalRule(policy?.versionUserApprovals), | |
| ...versionAnyApprovalRule(policy?.versionAnyApprovals), | |
| ...versionRoleApprovalRule(policy?.versionRoleApprovals), | |
| ]; | |
| releaseTargetId: string, | |
| ): Promise<FilterRule<Version>[]> => { | |
| const releaseTarget = await db | |
| .select() | |
| .from(schema.releaseTarget) | |
| .where(eq(schema.releaseTarget.id, releaseTargetId)) | |
| .then(takeFirst); | |
| const { environmentId } = releaseTarget; | |
| return [ | |
| ...versionUserApprovalRule(environmentId, policy?.versionUserApprovals), | |
| ...versionAnyApprovalRule(environmentId, policy?.versionAnyApprovals), | |
| ...versionRoleApprovalRule(environmentId, policy?.versionRoleApprovals), | |
| ]; | |
| }; | |
| export const getVersionApprovalRules = async ( | |
| policy: Policy | null, | |
| releaseTargetId: string, | |
| ): Promise<FilterRule<Version>[]> => { | |
| const releaseTarget = await db | |
| .select() | |
| .from(schema.releaseTarget) | |
| .where(eq(schema.releaseTarget.id, releaseTargetId)) | |
| .then(takeFirst); | |
| if (!releaseTarget) { | |
| throw new Error(`Release target not found: ${releaseTargetId}`); | |
| } | |
| const { environmentId } = releaseTarget; | |
| return [ | |
| ...versionUserApprovalRule(environmentId, policy?.versionUserApprovals), | |
| ...versionAnyApprovalRule(environmentId, policy?.versionAnyApprovals), | |
| ...versionRoleApprovalRule(environmentId, policy?.versionRoleApprovals), | |
| ]; | |
| }; |
🤖 Prompt for AI Agents
In packages/rule-engine/src/manager/version-manager-rules/version-approval.ts
around lines 55 to 70, the function does not handle the case when the release
target is not found and takeFirst returns undefined, leading to a runtime error
when accessing environmentId. Add a check after fetching the release target to
verify it is not undefined; if it is, throw an appropriate error or handle the
missing case gracefully before accessing environmentId.
Contributor
Author
There was a problem hiding this comment.
takeFirst throws an error if release target is not found
Contributor
There was a problem hiding this comment.
@adityachoudhari26 Thank you for the clarification! You're absolutely right - if takeFirst throws an error when no record is found, then the current implementation already has proper error handling built-in. My suggested null check would indeed be unnecessary. I appreciate you correcting my understanding of how takeFirst behaves in this codebase.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#593
File: packages/rule-engine/src/manager/version-manager-rules/version-approval.ts:55-70
Timestamp: 2025-06-18T21:46:51.448Z
Learning: The `takeFirst` utility function from the database layer throws an error if no record is found, rather than returning `undefined`. This means functions using `takeFirst` already have error handling built-in and don't require additional null checks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Comment on lines
5499
to
5503
| "required": [ | ||
| "id", | ||
| "deploymentVersionId", | ||
| "environmentId", | ||
| "userId", |
Contributor
There was a problem hiding this comment.
Review backwards compatibility of new required environmentId
Marking environmentId as required in ApprovalRecord will break existing singular approve/reject endpoints since they don’t supply this field. Consider marking it nullable or creating a separate schema for environment-scoped records to avoid invalid responses.
🤖 Prompt for AI Agents
In openapi.v1.json around lines 5499 to 5503, the field "environmentId" was
added as required in the ApprovalRecord schema, which breaks backward
compatibility with existing approve/reject endpoints that do not provide this
field. To fix this, either make "environmentId" nullable in the schema or create
a separate schema variant for environment-scoped approval records, ensuring the
original endpoints remain valid and do not produce invalid responses.
Comment on lines
625
to
728
| "/v1/deployment-versions/{deploymentVersionId}/reject/environment/{environmentId}": { | ||
| "post": { | ||
| "summary": "Reject a deployment version for an environment", | ||
| "operationId": "rejectDeploymentVersionForEnvironment", | ||
| "parameters": [ | ||
| { | ||
| "name": "deploymentVersionId", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| } | ||
| }, | ||
| { | ||
| "name": "environmentId", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| } | ||
| } | ||
| ], | ||
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "reason": { | ||
| "type": "string" | ||
| }, | ||
| "approvedAt": { | ||
| "type": "string", | ||
| "format": "date-time" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "reason" | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "responses": { | ||
| "200": { | ||
| "description": "Rejection record created", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ApprovalRecord" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "403": { | ||
| "description": "Forbidden", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "404": { | ||
| "description": "Deployment version or environment not found", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal server error", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
Ensure requestBody is marked required
Same issue in the rejection endpoint: the requestBody is missing "required": true. Add it for consistency and to prevent invalid calls.
"/v1/deployment-versions/{deploymentVersionId}/reject/environment/{environmentId}": {
"post": {
"parameters": [ ... ],
- "requestBody": {
+ "requestBody": {
+ "required": true,
"content": {
"application/json": { ... }
}📝 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.
Suggested change
| "/v1/deployment-versions/{deploymentVersionId}/reject/environment/{environmentId}": { | |
| "post": { | |
| "summary": "Reject a deployment version for an environment", | |
| "operationId": "rejectDeploymentVersionForEnvironment", | |
| "parameters": [ | |
| { | |
| "name": "deploymentVersionId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| }, | |
| { | |
| "name": "environmentId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| } | |
| ], | |
| "requestBody": { | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "reason": { | |
| "type": "string" | |
| }, | |
| "approvedAt": { | |
| "type": "string", | |
| "format": "date-time" | |
| } | |
| }, | |
| "required": [ | |
| "reason" | |
| ] | |
| } | |
| } | |
| } | |
| }, | |
| "responses": { | |
| "200": { | |
| "description": "Rejection record created", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ApprovalRecord" | |
| } | |
| } | |
| } | |
| }, | |
| "403": { | |
| "description": "Forbidden", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "404": { | |
| "description": "Deployment version or environment not found", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "Internal server error", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "/v1/deployment-versions/{deploymentVersionId}/reject/environment/{environmentId}": { | |
| "post": { | |
| "summary": "Reject a deployment version for an environment", | |
| "operationId": "rejectDeploymentVersionForEnvironment", | |
| "parameters": [ | |
| { | |
| "name": "deploymentVersionId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| }, | |
| { | |
| "name": "environmentId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| } | |
| ], | |
| "requestBody": { | |
| "required": true, | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "reason": { | |
| "type": "string" | |
| }, | |
| "approvedAt": { | |
| "type": "string", | |
| "format": "date-time" | |
| } | |
| }, | |
| "required": [ | |
| "reason" | |
| ] | |
| } | |
| } | |
| } | |
| }, | |
| "responses": { | |
| "200": { | |
| "description": "Rejection record created", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ApprovalRecord" | |
| } | |
| } | |
| } | |
| }, | |
| "403": { | |
| "description": "Forbidden", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "404": { | |
| "description": "Deployment version or environment not found", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "Internal server error", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
In openapi.v1.json between lines 625 and 728, the POST endpoint for rejecting a
deployment version lacks the "required": true attribute on the requestBody
object. To fix this, add "required": true to the requestBody field to ensure the
request body is mandatory, maintaining consistency and preventing invalid calls.
Comment on lines
222
to
325
| "/v1/deployment-versions/{deploymentVersionId}/approve/environment/{environmentId}": { | ||
| "post": { | ||
| "summary": "Approve a deployment version for an environment", | ||
| "operationId": "approveDeploymentVersionForEnvironment", | ||
| "parameters": [ | ||
| { | ||
| "name": "deploymentVersionId", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| } | ||
| }, | ||
| { | ||
| "name": "environmentId", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| } | ||
| } | ||
| ], | ||
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "reason": { | ||
| "type": "string" | ||
| }, | ||
| "approvedAt": { | ||
| "type": "string", | ||
| "format": "date-time" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "reason" | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "responses": { | ||
| "200": { | ||
| "description": "Approval record created", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ApprovalRecord" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "403": { | ||
| "description": "Forbidden", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "404": { | ||
| "description": "Deployment version or environment not found", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal server error", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
Ensure requestBody is marked required
The new environment-scoped approval endpoint’s requestBody object is missing a "required": true flag. Without this, clients may omit the request body altogether, leading to unexpected errors. Add "required": true to align with other endpoints.
"/v1/deployment-versions/{deploymentVersionId}/approve/environment/{environmentId}": {
"post": {
"parameters": [ ... ],
- "requestBody": {
+ "requestBody": {
+ "required": true,
"content": {
"application/json": { ... }
}📝 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.
Suggested change
| "/v1/deployment-versions/{deploymentVersionId}/approve/environment/{environmentId}": { | |
| "post": { | |
| "summary": "Approve a deployment version for an environment", | |
| "operationId": "approveDeploymentVersionForEnvironment", | |
| "parameters": [ | |
| { | |
| "name": "deploymentVersionId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| }, | |
| { | |
| "name": "environmentId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| } | |
| ], | |
| "requestBody": { | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "reason": { | |
| "type": "string" | |
| }, | |
| "approvedAt": { | |
| "type": "string", | |
| "format": "date-time" | |
| } | |
| }, | |
| "required": [ | |
| "reason" | |
| ] | |
| } | |
| } | |
| } | |
| }, | |
| "responses": { | |
| "200": { | |
| "description": "Approval record created", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ApprovalRecord" | |
| } | |
| } | |
| } | |
| }, | |
| "403": { | |
| "description": "Forbidden", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "404": { | |
| "description": "Deployment version or environment not found", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "Internal server error", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "/v1/deployment-versions/{deploymentVersionId}/approve/environment/{environmentId}": { | |
| "post": { | |
| "summary": "Approve a deployment version for an environment", | |
| "operationId": "approveDeploymentVersionForEnvironment", | |
| "parameters": [ | |
| { | |
| "name": "deploymentVersionId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| }, | |
| { | |
| "name": "environmentId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| } | |
| ], | |
| "requestBody": { | |
| "required": true, | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "reason": { | |
| "type": "string" | |
| }, | |
| "approvedAt": { | |
| "type": "string", | |
| "format": "date-time" | |
| } | |
| }, | |
| "required": [ | |
| "reason" | |
| ] | |
| } | |
| } | |
| } | |
| }, | |
| "responses": { | |
| "200": { | |
| "description": "Approval record created", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ApprovalRecord" | |
| } | |
| } | |
| } | |
| }, | |
| "403": { | |
| "description": "Forbidden", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "404": { | |
| "description": "Deployment version or environment not found", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "Internal server error", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
In openapi.v1.json between lines 222 and 325, the POST endpoint for approving a
deployment version for an environment has a requestBody defined but it is
missing the "required": true flag. To fix this, add "required": true to the
requestBody object to ensure clients must include the request body when calling
this endpoint, preventing omission and potential errors.
This was referenced Jul 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
API Changes
Tests
Database