feat: grad rollouts v2 #587
Conversation
WalkthroughThis change introduces a new "environment version rollout" policy rule for controlled, staged deployment rollouts across release targets. It adds database schema, API endpoints, OpenAPI documentation, backend logic, and end-to-end tests for configuring and retrieving rollout schedules. The policy system and rule engine are updated to support this rollout strategy, including approval-based rollout timing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant DB
participant RuleEngine
User->>API: GET /v1/deployment-versions/{deploymentVersionId}/environments/{environmentId}/rollout
API->>DB: Fetch deploymentVersion, environment, releaseTargets
API->>DB: Fetch applicable policies (with rollout config)
API->>RuleEngine: Compute rollout info for each releaseTarget
RuleEngine->>DB: (If needed) Fetch approvals, release target positions
RuleEngine-->>API: Rollout times and positions for each releaseTarget
API-->>User: JSON array of releaseTargets with rolloutTime, rolloutPosition
sequenceDiagram
participant Admin
participant API
participant DB
Admin->>API: PATCH /v1/policies/{policyId} (with environmentVersionRollout config)
API->>DB: Upsert environmentVersionRollout for policy
API-->>Admin: Updated policy with rollout config
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
packages/api/src/router/policy/evaluate.ts (1)
63-64: Extra join may inflate payloadAs in the rule-engine query, the rollout relation is now always selected but is not referenced in the rest of
evaluate.ts. If future logic will consume it, that’s fine; if not, you can save bandwidth by omitting it here.packages/rule-engine/src/manager/version-rule-engine.ts (1)
66-73: Brace-less multi-linereturnreduces safetyDropping the braces is legal, but the multi-line object literals make ASI pitfalls harder to spot and reduce diff clarity. Consider restoring braces for multi-line branches to keep the style consistent with the rest of the codebase:
-if (!result.passing) - return { +if (!result.passing) { chosenCandidate: null, rejectionReasons: new ConstantMap<string, string>( result.rejectionReason ?? "", ), - }; +}Same for the
allowedCandidates.length === 0block.Also applies to: 81-87, 90-94, 101-103
packages/rule-engine/src/rules/__tests__/environment-version-rollout-rule.test.ts (1)
112-114: UnnecessarytoBeCloseTo–differenceInMinutesalready floors
differenceInMinutesreturns an integer, sotoBeCloseTo(38, 1)is equivalent totoBe(38)and hides potential rounding issues.
Swap to a strict equality assertion to make the test intent clearer.packages/rule-engine/src/manager/version-manager-rules.ts (1)
60-63: Environment rollout rule not wired into the rule set
getRulesaggregates concurrency and approval rules but omits the newEnvironmentVersionRolloutRule.
If rollout gating is meant to be active at this level, the rule should be instantiated (probably via the helper inversion-manager-rules/environment-version-rollout.ts) and added to the returned array.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/api/src/router/policy/evaluate.ts(1 hunks)packages/db/src/schema/policy-relations.ts(2 hunks)packages/db/src/schema/rules/environment-version-rollout.ts(1 hunks)packages/db/src/schema/rules/index.ts(1 hunks)packages/db/src/schema/rules/rule-relations.ts(2 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(1 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(1 hunks)packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts(1 hunks)packages/rule-engine/src/manager/version-manager-rules/index.ts(1 hunks)packages/rule-engine/src/manager/version-manager-rules/version-approval.ts(1 hunks)packages/rule-engine/src/manager/version-rule-engine.ts(2 hunks)packages/rule-engine/src/rules/__tests__/environment-version-rollout-rule.test.ts(1 hunks)packages/rule-engine/src/rules/environment-version-rollout-rule.ts(1 hunks)packages/rule-engine/src/rules/version-approval-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 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.
packages/rule-engine/src/index.tspackages/rule-engine/src/types.tspackages/rule-engine/src/rules/version-approval-rule.tspackages/db/src/schema/rules/index.tspackages/rule-engine/src/db/get-applicable-policies.tspackages/api/src/router/policy/evaluate.tspackages/db/src/schema/rules/rule-relations.tspackages/rule-engine/src/manager/version-rule-engine.tspackages/rule-engine/src/manager/version-manager-rules/index.tspackages/db/src/schema/policy-relations.tspackages/rule-engine/src/rules/__tests__/environment-version-rollout-rule.test.tspackages/rule-engine/src/rules/environment-version-rollout-rule.tspackages/rule-engine/src/manager/version-manager-rules/version-approval.tspackages/db/src/schema/rules/environment-version-rollout.tspackages/rule-engine/src/manager/version-manager-rules.tspackages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts
🧬 Code Graph Analysis (4)
packages/db/src/schema/rules/rule-relations.ts (2)
packages/db/src/schema/rules/environment-version-rollout.ts (1)
policyRuleEnvironmentVersionRollout(12-34)packages/db/src/schema/policy.ts (1)
policy(42-62)
packages/db/src/schema/policy-relations.ts (1)
packages/db/src/schema/rules/environment-version-rollout.ts (1)
policyRuleEnvironmentVersionRollout(12-34)
packages/rule-engine/src/manager/version-manager-rules/version-approval.ts (3)
packages/rule-engine/src/types.ts (2)
Policy(66-74)FilterRule(31-36)packages/rule-engine/src/rules/version-approval-rule.ts (4)
VersionApprovalRule(28-66)getAnyApprovalRecords(68-81)getRoleApprovalRecords(83-96)getUserApprovalRecords(98-111)packages/rule-engine/src/manager/version-rule-engine.ts (1)
Version(11-16)
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts (8)
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(48-54)packages/rule-engine/src/rules/version-approval-rule.ts (3)
getAnyApprovalRecords(68-81)getUserApprovalRecords(98-111)getRoleApprovalRecords(83-96)packages/db/src/schema/release.ts (1)
releaseTarget(20-42)packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
takeFirst(9-13)packages/rule-engine/src/rules/environment-version-rollout-rule.ts (2)
RolloutTypeToOffsetFunction(31-37)EnvironmentVersionRolloutRule(46-89)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (12)
packages/rule-engine/src/index.ts (1)
2-2: Adjusted export path for version-manager-rules index
Exporting from./manager/version-manager-rules/index.jscorrectly aligns with the new consolidated exports.packages/rule-engine/src/rules/version-approval-rule.ts (1)
15-15: Verify DB schema support forapprovedAtfield
The newly addedapprovedAt: Date | nullin theRecordtype relies on a matching column in the approval record tables. Confirm that the database schema and ORM mappings include this field so thatrecord.approvedAtis populated as expected.packages/db/src/schema/rules/index.ts (1)
13-13: Expose environment version rollout rule in schema exports
Includingexport * from "./environment-version-rollout.js"correctly makes the new rollout rule schema available through the rules index.packages/rule-engine/src/types.ts (1)
73-73: AddenvironmentVersionRollouttoPolicytype
ThePolicytype now includes an optionalenvironmentVersionRolloutproperty, matching the new rollout rule schema. Ensure consumers handle bothnulland non-nullvalues.packages/rule-engine/src/manager/version-manager-rules/index.ts (1)
1-2: Consolidate version-manager rule exports
Re-exporting bothenvironment-version-rollout.jsandversion-approval.jscreates a unified entry point for version manager rules.packages/db/src/schema/rules/rule-relations.ts (1)
21-22: Environment-rollout relation wired correctlyThe import and the one-to-one relation mirror the existing pattern for other rule tables and look correct:
• correct path and naming
•fields/referencespair matches the rollout table’spolicyIdFK
No further action needed.Also applies to: 142-151
packages/rule-engine/src/db/get-applicable-policies.ts (1)
44-45: Relation eagerly loaded – double-check necessity
environmentVersionRolloutis now fetched for every policy. This is fine for correctness, but the property is not consumed anywhere in this module. If the rollout data is only required downstream, keep as is; otherwise, consider deferring the load to where it is actually used to avoid unnecessary hydration on large workspaces.packages/db/src/schema/policy-relations.ts (1)
14-15: Policy ↔︎ rollout relation added correctlyThe import and
one()mapping line up with the rollout table definition. Nothing else to flag.Also applies to: 33-36
packages/db/src/schema/rules/environment-version-rollout.ts (2)
22-29: Decimal scale = 2 may lose needed precision
positionGrowthFactorandtimeScaleIntervalfeed exponential / linear calculations.
Restricting them to two decimal places could be limiting for small factors (e.g., 0.005) or sub-minute intervals. Consider a higher scale or numeric type.
30-33: Enum value vs. string default
drizzle-ormexpects a raw enum value in.default().
PassingRolloutType.Linearworks only if the enum is emitted exactly as the DB value. Double-check generated SQL to ensure it doesn’t wrap the value in quotes twice.packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts (2)
1-18: Imports look good.All imports are properly structured and used within the file.
52-84: Well-implemented position calculation logic.The deterministic ordering using MD5 hash and window functions ensures consistent positioning across different versions. Good error handling for missing release targets.
packages/rule-engine/src/manager/version-manager-rules/version-approval.ts
Show resolved
Hide resolved
packages/rule-engine/src/manager/version-manager-rules/version-approval.ts
Show resolved
Hide resolved
packages/rule-engine/src/rules/environment-version-rollout-rule.ts
Outdated
Show resolved
Hide resolved
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts
Show resolved
Hide resolved
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
e2e/api/schema.ts (1)
2998-3021: 🛠️ Refactor suggestionCreation flow missing
environmentVersionRollout
upsertPolicyrequest body omits the new field, forcing callers to create a policy and then PATCH it.
Add the property for parity withupdatePolicy.concurrency?: components["schemas"]["PolicyConcurrency"]; + environmentVersionRollout?: components["schemas"]["EnvironmentVersionRollout"];
🧹 Nitpick comments (9)
apps/webservice/src/app/api/v1/policies/[policyId]/route.ts (1)
5-7: Redundant schema import
SCHEMAandschemaboth resolve to the same module; keeping one alias avoids cognitive overhead and tree-shaking noise.-import * as SCHEMA from "@ctrlplane/db/schema"; -import * as schema from "@ctrlplane/db/schema"; +import * as schema from "@ctrlplane/db/schema";(then rename
SCHEMAusages toschema).e2e/tests/api/policies/environment-version-rollout-policy.spec.ts (1)
26-26: Consider making the timeout more robust.The 1-second fixed timeout could be flaky in CI environments. Consider using a more reliable wait mechanism or increasing the timeout.
-await new Promise((resolve) => setTimeout(resolve, 1_000)); +// Wait for resources to be fully created +await new Promise((resolve) => setTimeout(resolve, 2_000));apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/environments/[environmentId]/rollout/route.ts (1)
89-117: Consider optimizing database queries.The code makes sequential database queries that could potentially be optimized:
getDeploymentVersion(line 89)getEnvironment(line 99)getReleaseTargets(line 106)getApplicablePoliciesWithoutResourceScope(line 112)Consider using Promise.all() for independent queries to improve performance.
-const deploymentVersion = await getDeploymentVersion( - db, - deploymentVersionId, -); -if (deploymentVersion == null) - return NextResponse.json( - { error: "Deployment version not found" }, - { status: NOT_FOUND }, - ); - -const environment = await getEnvironment(db, environmentId); -if (environment == null) - return NextResponse.json( - { error: "Environment not found" }, - { status: NOT_FOUND }, - ); +const [deploymentVersion, environment] = await Promise.all([ + getDeploymentVersion(db, deploymentVersionId), + getEnvironment(db, environmentId), +]); + +if (deploymentVersion == null) + return NextResponse.json( + { error: "Deployment version not found" }, + { status: NOT_FOUND }, + ); + +if (environment == null) + return NextResponse.json( + { error: "Environment not found" }, + { status: NOT_FOUND }, + );openapi.v1.json (4)
338-366: 200 response schema for rollout data
Returning an array ofReleaseTargetobjects extended withrolloutTimeandrolloutPositionis appropriate. To prevent excessively large payloads, consider adding amaxItemsconstraint on the array (CKV_OPENAPI_21).
2255-2260: Extend updatePolicy request with rollout config
IncludingenvironmentVersionRolloutin the PATCH request schema enables clients to configure rollout behavior. To support explicit removal of rollout settings, consider marking this property asnullable: true.
6197-6222: Refine EnvironmentVersionRollout schema description
TherolloutTypedescription mentions “normalized versions” but the enum only includes"linear"and"exponential". Either update the description to match the enum or add the missing normalized enum values.
6281-6286: Expose rollout config in Policy schema
AddingenvironmentVersionRollouttoPolicycompletes the schema. To indicate absence of a rollout policy, you may want to mark this property asnullable: truein the response definition.e2e/api/schema.ts (2)
399-401:GET /v1/policies/{policyId}added – remember rate-limit headersEndpoint wiring is fine; consider returning standard
Retry-After/X-RateLimit-*headers (as with other read-heavy endpoints) for consistency.
1652-1701: Inline response type may lead to duplication
getRolloutInfore-definesrolloutTime/rolloutPositioninline instead of extendingReleaseTargetvia a named schema.
Consider promoting this to e.g.ReleaseTargetWithRolloutto avoid copy-paste drift when fields evolve.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/environments/[environmentId]/rollout/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/environments/[environmentId]/rollout/route.ts(1 hunks)apps/webservice/src/app/api/v1/openapi.ts(0 hunks)apps/webservice/src/app/api/v1/policies/[policyId]/openapi.ts(2 hunks)apps/webservice/src/app/api/v1/policies/[policyId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/policies/openapi.ts(3 hunks)e2e/api/entity-fixtures.ts(1 hunks)e2e/api/schema.ts(8 hunks)e2e/package.json(1 hunks)e2e/tests/api/policies/environment-version-rollout-policy.spec.ts(1 hunks)e2e/tests/api/policies/environment-version-rollout-policy.spec.yaml(1 hunks)openapi.v1.json(6 hunks)packages/api/src/router/policy/evaluate.ts(1 hunks)packages/db/src/schema/policy.ts(3 hunks)packages/db/src/schema/rules/environment-version-rollout.ts(1 hunks)packages/rule-engine/src/db/create-policy.ts(2 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(3 hunks)packages/rule-engine/src/db/update-policy.ts(4 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(2 hunks)packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts(1 hunks)packages/rule-engine/src/manager/version-manager.ts(1 hunks)packages/rule-engine/src/rules/environment-version-rollout-rule.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/api/v1/openapi.ts
✅ Files skipped from review due to trivial changes (3)
- e2e/package.json
- apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/environments/[environmentId]/rollout/openapi.ts
- e2e/tests/api/policies/environment-version-rollout-policy.spec.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/api/src/router/policy/evaluate.ts
- packages/rule-engine/src/manager/version-manager-rules.ts
- packages/rule-engine/src/rules/environment-version-rollout-rule.ts
- packages/db/src/schema/rules/environment-version-rollout.ts
- packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts
🧰 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/webservice/src/app/api/v1/policies/[policyId]/route.tspackages/db/src/schema/policy.tsapps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/environments/[environmentId]/rollout/route.tse2e/api/entity-fixtures.tspackages/rule-engine/src/db/create-policy.tspackages/rule-engine/src/db/get-applicable-policies.tse2e/tests/api/policies/environment-version-rollout-policy.spec.tspackages/rule-engine/src/db/update-policy.tspackages/rule-engine/src/manager/version-manager.tsapps/webservice/src/app/api/v1/policies/openapi.tsapps/webservice/src/app/api/v1/policies/[policyId]/openapi.tse2e/api/schema.ts
🧬 Code Graph Analysis (4)
packages/db/src/schema/policy.ts (1)
packages/db/src/schema/rules/environment-version-rollout.ts (1)
createPolicyRuleEnvironmentVersionRollout(46-54)
packages/rule-engine/src/db/create-policy.ts (1)
packages/db/src/common.ts (1)
buildConflictUpdateColumns(30-46)
packages/rule-engine/src/db/get-applicable-policies.ts (2)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
Tx(22-22)
packages/rule-engine/src/manager/version-manager.ts (1)
packages/rule-engine/src/manager/version-manager-rules.ts (1)
getRules(58-76)
🪛 Checkov (3.2.334)
openapi.v1.json
[MEDIUM] 343-369: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (16)
packages/db/src/schema/policy.ts (1)
40-40: Environment-rollout rule wired in, but double-check null-delete semanticsThe new
environmentVersionRolloutfield is correctly imported and threaded through bothcreatePolicyandupdatePolicyschemas – nice.
One edge-case to verify: when a client wants to remove an existing rollout rule it will submit"environmentVersionRollout": null.
Please confirm thatupdatePolicyInTxinterpretsnullas “delete the existing row” rather than attempting an UPSERT withnullvalues (which would violate the NOT-NULL constraints in the rollout table).If the current implementation does not cascade-delete, add explicit handling, e.g.
if (data.environmentVersionRollout === null) { await tx.delete(schema.policyRuleEnvironmentVersionRollout) .where(eq(schema.policyRuleEnvironmentVersionRollout.policyId, policyId)); }Also applies to: 161-165, 200-204
apps/webservice/src/app/api/v1/policies/[policyId]/route.ts (1)
33-45: Verify relation names included in.withclause
concurrencyis listed as a relation. If the relation factory was not added inpolicy-relations.ts, Drizzle will throw at runtime. Same for the newenvironmentVersionRollout.Please run the route in a dev environment to ensure the query compiles.
packages/rule-engine/src/db/get-applicable-policies.ts (1)
62-63: Drizzle call name – ensure it isdb.selectDistinctDouble-check that the Drizzle version you target exports
selectDistinct, notselectDistinctOnorselectDistinct...variations; mismatching helper names will fail at runtime.packages/rule-engine/src/db/create-policy.ts (1)
193-211:✅ Verification successful
Verify the necessity of converting numeric values to strings.
The code converts
positionGrowthFactorandtimeScaleIntervalfrom numbers to strings before storing them in the database. This type conversion could lead to precision loss or parsing issues when retrieving the data.Consider storing these values as numeric types in the database if precision is important, or document why string storage is necessary.
🏁 Script executed:
#!/bin/bash # Description: Check the schema definition for policyRuleEnvironmentVersionRollout to verify the column types # Check the schema definition fd -e ts "environment-version-rollout" packages/db/src/schema --exec cat {} \; | grep -A 20 "policyRuleEnvironmentVersionRollout"Length of output: 1067
Keep numeric-to-string conversion for decimal fields
The
positionGrowthFactorandtimeScaleIntervalcolumns are defined asdecimal(…)in the schema and the insert builder expects string inputs for these fields. Converting the validatednumbertostringwith.toString()is therefore required and preserves full precision.• File: packages/rule-engine/src/db/create-policy.ts
• Lines: 193–211No changes needed.
apps/webservice/src/app/api/v1/policies/openapi.ts (1)
78-99: LGTM! Well-structured OpenAPI schema definition.The
EnvironmentVersionRolloutschema is properly defined with:
- Clear property descriptions explaining the purpose of each field
- Dynamic enum extraction from the database schema ensuring consistency
- All required fields properly marked
apps/webservice/src/app/api/v1/policies/[policyId]/openapi.ts (1)
12-60: Well-defined GET operation with proper error handling.The GET operation correctly:
- Uses UUID format for the policyId parameter
- Defines appropriate error responses (404 for not found, 500 for server errors)
- References the Policy schema for the response body
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/environments/[environmentId]/rollout/route.ts (1)
130-134: Rollout position sorting implementation looks correct.The sorting by rollout position ensures consistent ordering of release targets, which is essential for predictable rollout behavior.
packages/rule-engine/src/db/update-policy.ts (1)
207-208: Integration looks solidDestructuring, Promise.all orchestration, and relation fetch have been updated consistently.
No further concerns here.Also applies to: 233-233, 246-246
openapi.v1.json (4)
312-337: New rollout endpoint definition
The/v1/deployment-versions/{deploymentVersionId}/environments/{environmentId}/rolloutpath and its two UUID path parameters follow the established API conventions. TheoperationId, summary, and parameter schemas are correctly specified.
373-402: Error responses for rollout endpoint
The 404 (not found) and 500 (internal server error) responses use a consistent error-object schema matching other endpoints.
2096-2152: GET policy by ID operation
The new GET/v1/policies/{policyId}endpoint correctly returns the updatedPolicyschema or appropriate 404/500 errors. The path parameter andoperationIdalign with existing patterns.
2502-2503: Use updated Policy schema in upsertPolicy response
Switching the POST/v1/policies200 response to reference the newPolicyschema ensures consistency with the GET and PATCH operations.e2e/api/schema.ts (4)
61-77: New endpoint added – LGTM, but confirm RBAC & pagination needsThe path definition looks consistent with existing style.
Double-check that downstream route handler enforces workspace / environment access controls and, if the rollout list can grow large, whether pagination or filtering should be exposed up-front.
1377-1378: Optional rollout field onPolicy– looks goodThe new linkage is correctly typed and optional – ✅.
2802-2844:operations.getPolicymirrors schema accurately – okNo issues spotted.
2916-2917: Update body supports rollout config – nice catchUpdate flow now accepts
environmentVersionRollout; implementation aligns with schema.
| const ruleGetter = options?.rules ?? getRules; | ||
| const rules = await ruleGetter(policy, this.releaseTarget.id); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
rules callback typing is now stale
await ruleGetter(policy, this.releaseTarget.id) passes two args and expects a Promise, yet
VersionEvaluateOptions.rules is still typed as
(p: Policy | null) => Array<FilterRule<Version> | PreValidationRule>.
Updating the type eliminates implicit-any/unused-param noise and makes the async contract explicit:
- rules?: (p: Policy | null) => Array<FilterRule<Version> | PreValidationRule>;
+ rules?: (
+ p: Policy | null,
+ releaseTargetId: string,
+ ) => Promise<Array<FilterRule<Version> | PreValidationRule>> | Array<FilterRule<Version> | PreValidationRule>;🤖 Prompt for AI Agents
In packages/rule-engine/src/manager/version-manager.ts around lines 183 to 185,
the rules callback type in VersionEvaluateOptions is outdated because it
currently expects a synchronous function with one parameter, but the code calls
it with two parameters and expects a Promise. Update the
VersionEvaluateOptions.rules type signature to accept two parameters (policy and
releaseTargetId) and return a Promise of an array of FilterRule or
PreValidationRule to match the actual usage and make the async contract
explicit.
There was a problem hiding this comment.
((p: Policy | null) => Array<FilterRule | PreValidationRule>) | ((policy: Policy | null, releaseTargetId: string) => Promise<Array<FilterRule | PreValidationRule>>)
this is the inferred, type, it looks fine to me? lmk
There was a problem hiding this comment.
@adityachoudhari26 good observation—the inferred type of the expression (options?.rules ?? getRules) is indeed the union you pasted, so the call site itself type-checks.
The only place the mismatch can bite is when a caller explicitly provides a rules implementation:
const opts: VersionEvaluateOptions = {
// ⛔ Type '{ ... }' is not assignable ...
rules: async (policy, releaseTargetId) => {
/* custom logic */
return [];
},
};Here the assignment fails because VersionEvaluateOptions.rules is still declared as the sync, single-arg signature returning an array, and Promise<Array<…>> isn’t assignable to that.
If you don’t expect external consumers to pass an async implementation, the current typing is harmless. Otherwise, widening the type (or adding an overload) eliminates that friction and improves IntelliSense:
type VersionEvaluateOptions = {
rules?: (
p: Policy | null,
releaseTargetId: string
) =>
| Array<FilterRule<Version> | PreValidationRule>
| Promise<Array<FilterRule<Version> | PreValidationRule>>;
// …
};Totally your call—I just wanted to flag the potential DX hiccup.
| test("should rollout versions in a linear fashion", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const { id: workspaceId } = workspace; | ||
| const { prefix } = builder.refs; | ||
|
|
||
| const deployment = builder.refs.deployments.find( | ||
| (d) => d.name === `${prefix}-linear-rollout-no-approval`, | ||
| )!; | ||
|
|
||
| const environment = builder.refs.environments.find( | ||
| (e) => e.name === `${prefix}-linear-rollout-no-approval`, | ||
| )!; | ||
|
|
||
| const policyRef = builder.refs.policies.find( | ||
| (p) => p.name === `${prefix}-linear-rollout-no-approval`, | ||
| )!; | ||
| const policyResponse = await api.GET("/v1/policies/{policyId}", { | ||
| params: { | ||
| path: { | ||
| policyId: policyRef.id, | ||
| }, | ||
| }, | ||
| }); | ||
| expect(policyResponse.response.status).toBe(200); | ||
| expect(policyResponse.data).toBeDefined(); | ||
| const policy = policyResponse.data!; | ||
| expect(policy.environmentVersionRollout).toBeDefined(); | ||
| const environmentVersionRollout = policy.environmentVersionRollout!; | ||
| expect(environmentVersionRollout.rolloutType).toBe("linear"); | ||
|
|
||
| const deploymentVersionResponse = await api.POST( | ||
| "/v1/deployment-versions", | ||
| { | ||
| body: { | ||
| deploymentId: deployment.id, | ||
| tag: faker.string.alphanumeric(10), | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| expect(deploymentVersionResponse.response.status).toBe(201); | ||
| expect(deploymentVersionResponse.data).toBeDefined(); | ||
| const deploymentVersion = deploymentVersionResponse.data!; | ||
|
|
||
| const rolloutResponse = await api.GET( | ||
| `/v1/deployment-versions/{deploymentVersionId}/environments/{environmentId}/rollout`, | ||
| { | ||
| params: { | ||
| path: { | ||
| deploymentVersionId: deploymentVersion.id, | ||
| environmentId: environment.id, | ||
| }, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| expect(rolloutResponse.response.status).toBe(200); | ||
| const releaseTargetsWithRolloutInfo = rolloutResponse.data ?? []; | ||
| expect(releaseTargetsWithRolloutInfo).toHaveLength(4); | ||
|
|
||
| const expectedRolloutStart = startOfMinute( | ||
| new Date(deploymentVersion.createdAt), | ||
| ); | ||
|
|
||
| for (let i = 0; i < releaseTargetsWithRolloutInfo.length; i++) { | ||
| const releaseTarget = releaseTargetsWithRolloutInfo[i]; | ||
| const expectedRolloutTime = addMinutes( | ||
| expectedRolloutStart, | ||
| environmentVersionRollout.timeScaleInterval * i, | ||
| ); | ||
|
|
||
| expect(releaseTarget.rolloutTime).toBe(expectedRolloutTime); | ||
| expect(releaseTarget.rolloutPosition).toBe(i); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test coverage for edge cases and other rollout types.
The test only covers the happy path for linear rollout. Consider adding tests for:
- Exponential rollout type
- Normalized rollout types
- Error scenarios (invalid policy, missing environment, etc.)
- Edge cases (zero growth factor, negative intervals)
Would you like me to generate additional test cases to improve coverage?
🤖 Prompt for AI Agents
In e2e/tests/api/policies/environment-version-rollout-policy.spec.ts from lines
33 to 110, the current test only covers the linear rollout happy path. To
improve coverage, add new test cases for exponential and normalized rollout
types, include error handling tests for invalid policies and missing
environments, and cover edge cases such as zero growth factor and negative
intervals. Structure these tests similarly to the existing one, adjusting inputs
and assertions to reflect each scenario.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
e2e/api/schema.ts (1)
3003-3024:⚠️ Potential issue
upsertPolicyrequest body missingenvironmentVersionRollout
updatePolicyalready supports this field, but the create/upsert flow does not, forcing callers to perform a two-step create-then-update.- concurrency?: components["schemas"]["PolicyConcurrency"]; + concurrency?: components["schemas"]["PolicyConcurrency"]; + environmentVersionRollout?: components["schemas"]["EnvironmentVersionRollout"];Keeping the two endpoints symmetrical prevents avoidable 400s and simplifies SDKs.
♻️ Duplicate comments (1)
e2e/api/schema.ts (1)
1348-1362: Enum now aligns with docstring – resolvedThe
"*-normalized"variants are present, eliminating the earlier discrepancy.
🧹 Nitpick comments (10)
e2e/tests/api/policies/environment-version-rollout-policy.spec.yaml (1)
84-84: Add trailing newlineThe file is missing a trailing newline, tripping YAML lint and some Git diff viewers.
A one-byte fix keeps linters and tooling quiet.@@ - env: prod↵ + env: prod +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 84-84: no new line character at the end of file
(new-line-at-end-of-file)
apps/webservice/src/app/api/v1/policies/[policyId]/route.ts (3)
5-8: Duplicate wildcard import obscures intent
@ctrlplane/db/schemais imported twice under different aliases (SCHEMAandschema).
Besides the cognitive overhead, it becomes easy to call the wrong constant (e.g.schema.policyvsSCHEMA.policy) and TypeScript will not help you spot the mistake.-import * as SCHEMA from "@ctrlplane/db/schema"; -import * as schema from "@ctrlplane/db/schema"; +import * as dbSchema from "@ctrlplane/db/schema";Then adjust the remaining references (
SCHEMA → dbSchema).
This keeps a single, unambiguous alias.
65-69: Property shadowing may confuse clients
...policy, concurrencyoverwrites the originalconcurrencyrelation with a scalar value.
While deliberate, the silent overwrite is non-obvious and could break future refactors.Safer options:
- Rename the scalar (
concurrencyValue), or- Strip the relation before the spread:
const { concurrency: _, ...sanitizedPolicy } = policy; return NextResponse.json({ ...sanitizedPolicy, concurrency });
70-76: Include contextual info in error log
log.error("Failed to get policy", { error });omits thepolicyId, making incident tracing harder.-log.error("Failed to get policy", { error }); +log.error("Failed to get policy", { policyId, error });packages/db/drizzle/0112_curved_annihilus.sql (1)
8-10: Missing index for frequent look-upsAccess patterns will likely join on
policy_id.
Although the UNIQUE constraint creates an index, be explicit for clarity and future flexibility:CREATE INDEX IF NOT EXISTS idx_env_rollout_policy_id ON policy_rule_environment_version_rollout(policy_id);openapi.v1.json (4)
343-349: Bound the rollout response array
The response schema for the rollout endpoint defines an unbounded array. To prevent excessively large payloads, consider adding amaxItemsconstraint. For example:type: array items: allOf: [...] maxItems: 1000🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 343-369: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
2255-2262: Strictly defineenvironmentVersionRolloutin request body
The PATCH schema now allows anenvironmentVersionRolloutobject but doesn’t prevent unspecified properties. Add anadditionalProperties: falseat the same level to enforce strict validation:"environmentVersionRollout": { "$ref": "#/components/schemas/EnvironmentVersionRollout" }, + "additionalProperties": false
6197-6224: HardenEnvironmentVersionRolloutschema and provide example
The newEnvironmentVersionRolloutschema lacks anadditionalPropertiesconstraint and an example. To improve validation and documentation, extend it as follows:"EnvironmentVersionRollout": { "type": "object", + "additionalProperties": false, "properties": { "positionGrowthFactor": { ... }, ... }, "required": [ ... ], + "example": { + "positionGrowthFactor": 1.2, + "timeScaleInterval": 3600, + "rolloutType": "exponential-normalized" + } }
6284-6287: Enhance documentation forenvironmentVersionRolloutinPolicyschema
ThePolicyschema now includesenvironmentVersionRollout. Consider adding a description or example under this property to clarify its usage for API consumers.packages/db/src/schema/rules/environment-version-rollout.ts (1)
8-13: Name shadowing is confusing
rolloutTypeserves both as the enum factory and later as the column builder (rolloutType("rollout_type")).
Renaming the factory torolloutTypeEnum(or similar) avoids mental context switching and improves readability.Also applies to: 64-66
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/webservice/src/app/api/v1/policies/[policyId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/policies/openapi.ts(3 hunks)e2e/api/entity-fixtures.ts(1 hunks)e2e/api/schema.ts(8 hunks)e2e/tests/api/policies/environment-version-rollout-policy.spec.ts(1 hunks)e2e/tests/api/policies/environment-version-rollout-policy.spec.yaml(1 hunks)openapi.v1.json(6 hunks)packages/db/drizzle/0112_curved_annihilus.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/rules/environment-version-rollout.ts(1 hunks)packages/rule-engine/src/db/create-policy.ts(3 hunks)packages/rule-engine/src/db/update-policy.ts(4 hunks)packages/rule-engine/src/rules/environment-version-rollout-rule.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/api/entity-fixtures.ts
- apps/webservice/src/app/api/v1/policies/openapi.ts
- packages/rule-engine/src/db/create-policy.ts
- packages/rule-engine/src/rules/environment-version-rollout-rule.ts
- e2e/tests/api/policies/environment-version-rollout-policy.spec.ts
- packages/rule-engine/src/db/update-policy.ts
🧰 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/webservice/src/app/api/v1/policies/[policyId]/route.tspackages/db/src/schema/rules/environment-version-rollout.tse2e/api/schema.ts
🧬 Code Graph Analysis (1)
apps/webservice/src/app/api/v1/policies/[policyId]/route.ts (3)
apps/webservice/src/app/api/v1/middleware.ts (1)
request(24-53)apps/webservice/src/app/api/v1/auth.ts (2)
authn(16-22)authz(26-62)packages/db/src/schema/policy.ts (1)
policy(43-63)
🪛 Checkov (3.2.334)
openapi.v1.json
[MEDIUM] 343-369: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 YAMLlint (1.37.1)
e2e/tests/api/policies/environment-version-rollout-policy.spec.yaml
[error] 84-84: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
e2e/api/schema.ts (1)
62-77: New rollout-info endpoint looks consistentPath params, operation id (
getRolloutInfo), and response schema use the recently addedrolloutTime/rolloutPositionfields – matches backend contract.Also applies to: 1655-1704
| const concurrency = policy?.concurrency?.concurrency; | ||
| const environmentVersionRollout = | ||
| policy?.environmentVersionRollout != null | ||
| ? { | ||
| ...policy.environmentVersionRollout, | ||
| rolloutType: | ||
| SCHEMA.dbRolloutTypeToAPIRolloutType[ | ||
| policy.environmentVersionRollout.rolloutType | ||
| ], | ||
| } | ||
| : null; | ||
|
|
There was a problem hiding this comment.
Leaking internal DB columns over the public API
environmentVersionRollout is created by spreading the DB row:
{ ...policy.environmentVersionRollout, rolloutType: … }This forwards internal columns such as id, policyId, createdAt, etc.
Those fields are not part of the OpenAPI contract and shouldn’t be exposed.
-const environmentVersionRollout = /* … */ {
- ...policy.environmentVersionRollout,
- rolloutType: dbSchema.dbRolloutTypeToAPIRolloutType[…],
-};
+const { positionGrowthFactor, timeScaleInterval, rolloutType } =
+ policy.environmentVersionRollout;
+const environmentVersionRollout = {
+ positionGrowthFactor,
+ timeScaleInterval,
+ rolloutType: dbSchema.dbRolloutTypeToAPIRolloutType[rolloutType],
+};Return only the whitelisted, documented fields.
🤖 Prompt for AI Agents
In apps/webservice/src/app/api/v1/policies/[policyId]/route.ts around lines 47
to 58, the environmentVersionRollout object is created by spreading the entire
policy.environmentVersionRollout DB row, which exposes internal fields like id,
policyId, and createdAt not meant for the public API. To fix this, explicitly
construct environmentVersionRollout by selecting and returning only the
whitelisted, documented fields defined in the OpenAPI contract instead of
spreading the entire object.
There was a problem hiding this comment.
i'm fine with these other fields being returned
There was a problem hiding this comment.
@adityachoudhari26 Understood—if you’re comfortable exposing those extra fields, we’ll leave the implementation as-is. Let me know if you’d like to revisit or tighten the response payload in the future, otherwise I’ll consider this resolved.
| "position_growth_factor" numeric DEFAULT '1' NOT NULL, | ||
| "time_scale_interval" numeric NOT NULL, | ||
| "rollout_type" "rollout_type" DEFAULT 'linear' NOT NULL, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer interval over numeric for time units
time_scale_interval is declared as numeric, yet downstream code most likely treats it as a duration.
PostgreSQL’s native interval type provides richer semantics (units, arithmetic, formatting) and prevents unit confusion.
If a plain number is truly required, add a CHECK constraint to enforce positivity and document the unit (seconds? minutes?).
-- example
ALTER TABLE policy_rule_environment_version_rollout
ADD CONSTRAINT chk_time_scale_positive
CHECK (time_scale_interval > 0);🤖 Prompt for AI Agents
In packages/db/drizzle/0112_curved_annihilus.sql around lines 6 to 8, the column
time_scale_interval is defined as numeric but represents a time duration. Change
its type to PostgreSQL's native interval type to leverage proper time semantics
and avoid unit confusion. If numeric must be kept, add a CHECK constraint to
ensure the value is positive and document the expected unit clearly in comments.
There was a problem hiding this comment.
we actually don't treat it as a duration, we literlaly treat it as a numeric. its used as a scaling factor in an equation
There was a problem hiding this comment.
@adityachoudhari26 Got it—thanks for clarifying that time_scale_interval is a dimensionless scaling factor. In that case, sticking with numeric makes perfect sense.
(As a minor hardening step, you could still add a CHECK (time_scale_interval > 0) to guard against accidental negatives, but that’s entirely optional.)
Let me know if you’d like a quick patch for the check constraint; otherwise I’ll consider this resolved.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#587
File: packages/db/drizzle/0112_curved_annihilus.sql:6-8
Timestamp: 2025-06-13T00:14:44.033Z
Learning: In table `policy_rule_environment_version_rollout`, column `time_scale_interval` is intentionally stored as `numeric` because it represents a dimensionless scaling factor in rollout equations, not a time duration.
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.
| "/v1/deployment-versions/{deploymentVersionId}/environments/{environmentId}/rollout": { | ||
| "get": { | ||
| "summary": "Get the rollout information across all release targets for a given deployment version and environment", | ||
| "operationId": "getRolloutInfo", | ||
| "parameters": [ |
There was a problem hiding this comment.
Add security requirement to rollout endpoint
The new GET /v1/deployment-versions/{deploymentVersionId}/environments/{environmentId}/rollout is missing a security section. All protected API operations should declare:
security:
- bearerAuth: []to enforce authentication.
🤖 Prompt for AI Agents
In openapi.v1.json around lines 312 to 316, the GET endpoint for rollout is
missing a security section. Add a "security" field with the value
[{"bearerAuth": []}] to this endpoint to enforce authentication as required for
all protected API operations.
| "get": { | ||
| "summary": "Get a policy", | ||
| "operationId": "getPolicy", | ||
| "parameters": [ | ||
| { | ||
| "name": "policyId", | ||
| "in": "path", |
There was a problem hiding this comment.
Protect GET /v1/policies/{policyId} with security
This newly added GET operation for retrieving a policy doesn’t declare a security requirement. To align with other policy endpoints, add:
security:
- bearerAuth: []🤖 Prompt for AI Agents
In openapi.v1.json around lines 2096 to 2102, the GET operation for
/v1/policies/{policyId} is missing a security requirement. Add a security
section with bearerAuth to this operation to protect it, matching the security
declarations of other policy endpoints. This involves inserting "security":
[{"bearerAuth": []}] within the GET operation object.
| rolloutType: z.enum([ | ||
| "linear", | ||
| "exponential", | ||
| "linear-normalized", | ||
| "exponential-normalized", | ||
| ]), | ||
| }, |
There was a problem hiding this comment.
API ↔︎ DB enum mismatch will break inserts/updates
The table stores underscore values (linear_normalized, …) but the insert schema allows hyphenated values (linear-normalized, …).
Unless every write path converts via apiRolloutTypeToDBRolloutType before hitting the DB (and createInsertSchema is never used for direct inserts), this will raise a invalid input value for enum "rollout_type" error.
Either:
- Change the Zod enum to underscore variants, or
- Keep current enum but wrap DB writes in an explicit transform in
createInsertSchema:
rolloutType: z
- .enum(["linear", "exponential", "linear-normalized", "exponential-normalized"])
+ .enum(["linear", "exponential", "linear-normalized", "exponential-normalized"])
.transform((v) => apiRolloutTypeToDBRolloutType[v]),Pick one place to do the translation; having it in multiple layers invites drift.
Also applies to: 64-66, 8-13
🤖 Prompt for AI Agents
In packages/db/src/schema/rules/environment-version-rollout.ts around lines 79
to 85, the Zod enum uses hyphenated strings while the database expects
underscore-separated values, causing enum mismatch errors on inserts/updates.
Fix this by either changing the Zod enum values to use underscores to match the
DB enum or by adding a transformation in createInsertSchema that converts
hyphenated values to underscore variants before writing to the DB. Ensure this
translation is done in a single consistent place to avoid drift. Also apply this
fix to lines 64-66 and 8-13 where similar enums are defined.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
e2e/api/schema.ts (1)
3004-3027:⚠️ Potential issue
upsertPolicymisses the newenvironmentVersionRolloutfield
Policy,updatePolicy, andgetPolicyexpose the optionalenvironmentVersionRollout, but theupsertPolicyrequest body doesn’t allow it.
New policies therefore cannot be created with rollout settings in a single call – clients have to upsert and then patch, which is error-prone.- concurrency?: components["schemas"]["PolicyConcurrency"]; + concurrency?: components["schemas"]["PolicyConcurrency"]; + environmentVersionRollout?: components["schemas"]["EnvironmentVersionRollout"];Add the field to keep the create and update semantics aligned.
♻️ Duplicate comments (3)
e2e/tests/api/policies/environment-version-rollout-policy.spec.ts (1)
136-137: Same flakiness comment as above – each occurrence should be updated.Also applies to: 226-228, 318-320, 415-416, 496-497
openapi.v1.json (2)
317-325: Add security requirement to rollout endpoint
The new GET/v1/deployment-versions/{deploymentVersionId}/environments/{environmentId}/rolloutis missing asecuritysection. All protected operations should declare:"security": [{"bearerAuth": []}],to enforce authentication.
2101-2107: Add security requirement to getPolicy
The GET/v1/policies/{policyId}operation lacks asecuritysection. To align with other policy endpoints, insert:"security": [{"bearerAuth": []}],
🧹 Nitpick comments (6)
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/route.ts (1)
46-48: Guard against “Invalid Date” and future timestamps
new Date(ctx.body.approvedAt)can still yieldInvalid Dateif the runtime’s parser fails spectacularly or if someone bypasses validation.
Consider an explicit check and reject obviously invalid/future dates to keep data consistent:-const approvedAt = - ctx.body.approvedAt != null ? new Date(ctx.body.approvedAt) : new Date(); +const approvedAtRaw = ctx.body.approvedAt ?? new Date(); +const approvedAt = + approvedAtRaw instanceof Date ? approvedAtRaw : new Date(approvedAtRaw); + +if (Number.isNaN(approvedAt.getTime()) || approvedAt.getTime() > Date.now()) + return NextResponse.json({ error: "Invalid approvedAt value" }, { status: 400 });Also applies to: 55-56
e2e/tests/api/policies/environment-version-rollout-policy.spec.ts (2)
292-301:toBeCloseToon an integer loses intent
rolloutPositionshould be an integer index.
Prefer strict equality to catch accidental floats:-expect(Number(releaseTarget.rolloutPosition)).toBeCloseTo(i, 4); +expect(Number(releaseTarget.rolloutPosition)).toBe(i);
32-120: Consider extracting helpers to reduce 400-line duplicationEvery test repeats:
- Policy creation payload assembly
- Deployment-version creation
- Rollout retrieval
- Expectation loop
Factor these into utility functions (e.g.,
createPolicy,createDeploymentVersion,getRolloutInfo) to cut boilerplate and clarify what changes between tests.Also applies to: 122-210, 212-302, 304-399, 401-480, 482-583
openapi.v1.json (1)
349-353: Limit response array size
The200response schema defines an unbounded array. Consider adding amaxItemsconstraint to prevent excessively large payloads:"schema": { "type": "array", + "maxItems": 1000, "items": { ... } }e2e/api/schema.ts (2)
1348-1362: Specify units & add validation hints for rollout parameters
positionGrowthFactorandtimeScaleIntervalare plainnumbertypes without any indication of valid range or unit (seconds? minutes?).
Clients cannot reason about the expected magnitude, and nothing prevents negative or zero values which would break the maths on the rule-engine side.positionGrowthFactor: number; // e.g. >= 0.1 - timeScaleInterval: number; + /** + * Format: int32 + * @minimum 1 + * @description Time-scale interval in seconds. + */ + timeScaleInterval: number;Consider adding
@minimum,@maximum, or at least clarifying the unit in the doc-string so generated SDKs carry that information.
1657-1707: Introduce a named schema for the rollout response for better reuseThe response body is defined as an inline anonymous array of
ReleaseTarget & { rolloutTime; rolloutPosition }.
This pattern is already used by e2e tests and the node-SDK – moving it to a named component schema (e.g.RolloutInfo) will:
- reduce duplication across specs,
- yield cleaner generated typings,
- make future evolution (adding fields) easier.
No functional impact, but improves maintainability of the OpenAPI spec.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/route.ts(2 hunks)e2e/api/schema.ts(9 hunks)e2e/tests/api/policies/environment-version-rollout-policy.spec.ts(1 hunks)e2e/tests/api/policies/environment-version-rollout-policy.spec.yaml(1 hunks)openapi.v1.json(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/openapi.ts
🧰 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/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/route.tse2e/tests/api/policies/environment-version-rollout-policy.spec.tse2e/api/schema.ts
🧬 Code Graph Analysis (1)
e2e/api/schema.ts (1)
packages/node-sdk/src/schema.ts (2)
operations(1422-4459)components(892-1420)
🪛 Checkov (3.2.334)
openapi.v1.json
[MEDIUM] 348-374: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (5)
e2e/tests/api/policies/environment-version-rollout-policy.spec.yaml (1)
1-105: Looks good – declarative fixture reads cleanlyNo schema or structural issues spotted.
e2e/tests/api/policies/environment-version-rollout-policy.spec.ts (1)
388-394: Formula may normalise twice – double-check denominator
(1 - exp(-numReleaseTargets / positionGrowthFactor))mixesnumReleaseTargetswithpositionGrowthFactor.
If normalisation logic isf(i)/f(N), the denominator should mirror the numerator (exp(-N / positionGrowthFactor)is fine), but it must not also divide bynumReleaseTargetsearlier (that happens in the numerator).Confirm this reflects production logic; otherwise the expected times will drift.
openapi.v1.json (2)
2260-2264: Support environment version rollout in updatePolicy
Good addition of theenvironmentVersionRolloutproperty to the updatePolicy request schema. This aligns the API with the new rollout feature.
6202-6229: Define EnvironmentVersionRollout schema
The newEnvironmentVersionRolloutcomponent correctly captures rollout parameters for policies.e2e/api/schema.ts (1)
1605-1610: Potential audit-bypass via client-suppliedapprovedAtAllowing callers to set
approvedAtin theapproveDeploymentVersionrequest can let them falsify the approval timestamp (e.g. back-dating to skirt deny-windows).
Unless this is an intentional privileged capability, remove it from the request schema and let the service populate it server-side.Do the backend handlers ignore / overwrite
approvedAt? If not, please hard-enforce it on the server and drop the field from the request contract.
| const bodySchema = z.object({ | ||
| reason: z.string().optional(), | ||
| approvedAt: z.string().datetime().optional(), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate & coerce approvedAt into a Date upfront
z.string().datetime() merely validates the ISO-8601 string but you still need to turn it into a Date.
Doing the coercion in the schema avoids repeated new Date() logic and guarantees you never store an Invalid Date.
-const bodySchema = z.object({
- reason: z.string().optional(),
- approvedAt: z.string().datetime().optional(),
-});
+const bodySchema = z.object({
+ reason: z.string().optional(),
+ approvedAt: z
+ .string()
+ .datetime({ offset: true }) // enforce timezone to avoid local-time ambiguities
+ .optional()
+ .transform((v) => (v ? new Date(v) : undefined)),
+});The rest of the handler can then use ctx.body.approvedAt ?? new Date() without additional parsing.
📝 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.
| const bodySchema = z.object({ | |
| reason: z.string().optional(), | |
| approvedAt: z.string().datetime().optional(), | |
| }); | |
| const bodySchema = z.object({ | |
| reason: z.string().optional(), | |
| approvedAt: z | |
| .string() | |
| .datetime({ offset: true }) // enforce timezone to avoid local-time ambiguities | |
| .optional() | |
| .transform((v) => (v ? new Date(v) : undefined)), | |
| }); |
🤖 Prompt for AI Agents
In
apps/webservice/src/app/api/v1/deployment-versions/[deploymentVersionId]/approve/route.ts
around lines 14 to 17, the schema currently validates approvedAt as an ISO-8601
string but does not convert it to a Date object. Update the zod schema to coerce
approvedAt into a Date using z.coerce.date().optional() instead of
z.string().datetime().optional(). This ensures approvedAt is always a valid Date
object, allowing the rest of the handler to use ctx.body.approvedAt ?? new
Date() without extra parsing or risk of invalid dates.
| const timeScaleInterval = faker.number.float({ min: 1, max: 100 }); | ||
| const policyResponse = await api.POST("/v1/policies", { | ||
| body: { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Random floats make the suite non-deterministic & flaky
faker.number.float({ min: 1, max: 100 }) produces different (and fractional) minutes on every run, which:
- Hampers reproducibility when a test fails.
- Introduces sub-minute offsets even though rollout code most likely deals in whole minutes.
Replace with a deterministic integer (or seed faker) to stabilise the suite:
-const timeScaleInterval = faker.number.float({ min: 1, max: 100 });
+const timeScaleInterval = 10; // or faker.number.int({ min: 1, max: 100 })📝 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.
| const timeScaleInterval = faker.number.float({ min: 1, max: 100 }); | |
| const policyResponse = await api.POST("/v1/policies", { | |
| body: { | |
| const timeScaleInterval = 10; // or faker.number.int({ min: 1, max: 100 }) | |
| const policyResponse = await api.POST("/v1/policies", { | |
| body: { |
🤖 Prompt for AI Agents
In e2e/tests/api/policies/environment-version-rollout-policy.spec.ts around
lines 46 to 48, the use of faker.number.float({ min: 1, max: 100 }) generates
random fractional minute values causing test flakiness and non-determinism.
Replace this with a deterministic integer value, either by using a fixed integer
or by seeding faker to produce consistent results, ensuring the
timeScaleInterval is a whole number to match expected rollout code behavior and
improve test reproducibility.
There was a problem hiding this comment.
on the contrary I prefer using trandom numbers because the tests should pass regardless of what floats are passed in here
There was a problem hiding this comment.
@adityachoudhari26 Got it —keeping randomness is intentional so the tests prove robustness across the full numeric range. Thanks for clarifying!
A couple of quick thoughts to keep the suite solid while still using random floats:
• Seeded randomness per test run
faker.seed(Date.now());gives you fresh values every CI execution but still lets you reproduce a failure locally by re-using the seed printed in the logs.
• Wider tolerance window
If rollout times are rounded to the nearest minute on the server side (startOfMinute ‑> addMinutes), fractional inputs can introduce up-to-30 s skew. Bumping the assertion from diff < 50 ms to something like diff < 1_000 ms (or using differenceInMinutes === 0) prevents rare flake without sacrificing the random-input guarantee.
Let me know if you’d like me to adjust the tolerance or add seeding; otherwise I’m happy to leave the current approach as is.
| const tenMinutesFromNow = addMinutes(new Date(), 10); | ||
| const approvalResponse = await api.POST( | ||
| "/v1/deployment-versions/{deploymentVersionId}/approve", | ||
| { | ||
| params: { | ||
| path: { | ||
| deploymentVersionId: deploymentVersion.id, | ||
| }, | ||
| }, | ||
| body: { approvedAt: tenMinutesFromNow.toISOString() }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Possible race: rule engine may not evaluate before assertion
Right after sending the approval you immediately fetch rollout info.
If rollout evaluation is asynchronous, the rollout times could still be null, causing intermittent failures.
Add a poll/wait helper until rollout information is populated:
-const rolloutResponse = await api.GET(
+const rolloutResponse = await api.waitFor(() =>
+ api.GET(
`/v1/deployment-versions/{deploymentVersionId}/environments/{environmentId}/rollout`,
/* params */
-);
+ ),
+ { timeout: 30_000, interval: 1_000 },
+);(or use Playwright’s expect.poll)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/tests/api/policies/environment-version-rollout-policy.spec.ts around
lines 541 to 552, the test immediately fetches rollout info after sending
approval, which may cause intermittent failures if the rollout evaluation is
asynchronous and not completed yet. To fix this, implement a polling or waiting
mechanism that repeatedly checks the rollout information until it is populated
before proceeding with assertions. You can use a custom poll/wait helper or
Playwright’s expect.poll to wait for the rollout times to be non-null.
| "type": "string" | ||
| }, | ||
| "approvedAt": { | ||
| "type": "string", | ||
| "format": "date-time", | ||
| "nullable": true | ||
| } |
There was a problem hiding this comment.
Missing security requirement for approveDeploymentVersion
The POST /v1/deployment-versions/{deploymentVersionId}/approve operation must include a security section to enforce bearer authentication.
Apply:
"post": {
+ "security": [{"bearerAuth": []}],
"summary": "Approve a deployment version",
...Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In openapi.v1.json around lines 246 to 252, the POST operation for
/v1/deployment-versions/{deploymentVersionId}/approve is missing a security
section. Add a security field specifying bearer authentication to enforce
security on this endpoint. This involves adding a "security" array with the
appropriate bearerAuth scheme to the operation object.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts (1)
46-54: Still possible runtime crash when no approvedAt found
Same issue flagged previously –latestApprovalRecordcan beundefinedif every record’sapprovedAtis null, causing.approvedAtaccess to explode.- const latestApprovalRecord = _.chain(allApprovalRecords) - .filter((record) => record.approvedAt != null) - .maxBy((record) => record.approvedAt) - .value(); - - return latestApprovalRecord.approvedAt ?? version.createdAt; + const latestApprovalRecord = _.chain(allApprovalRecords) + .filter((r) => r.approvedAt != null) + .maxBy((r) => r.approvedAt) + .value(); + + return latestApprovalRecord?.approvedAt ?? version.createdAt;
🧹 Nitpick comments (1)
packages/db/drizzle/0112_steady_proteus.sql (1)
6-9: Add DB-level safeguards for invalid rollout parameters
position_growth_factorandtime_scale_intervalcan be zero / negative, which later throws in application code. Capture this earlier with check constraints:ALTER TABLE "policy_rule_environment_version_rollout" ADD CONSTRAINT position_growth_factor_positive CHECK (position_growth_factor > 0), ADD CONSTRAINT time_scale_interval_positive CHECK (time_scale_interval > 0);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx(2 hunks)e2e/tests/api/policies/environment-version-rollout-policy.spec.ts(1 hunks)packages/api/src/router/policy/router.ts(1 hunks)packages/db/drizzle/0112_steady_proteus.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/rules/environment-version-rollout.ts(1 hunks)packages/rule-engine/src/db/create-policy.ts(3 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(3 hunks)packages/rule-engine/src/db/update-policy.ts(4 hunks)packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts(1 hunks)packages/rule-engine/src/rules/environment-version-rollout-rule.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/api/src/router/policy/router.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rule-engine/src/db/get-applicable-policies.ts
- packages/db/drizzle/meta/_journal.json
- packages/rule-engine/src/db/create-policy.ts
- packages/rule-engine/src/rules/environment-version-rollout-rule.ts
- packages/rule-engine/src/db/update-policy.ts
- packages/db/src/schema/rules/environment-version-rollout.ts
- e2e/tests/api/policies/environment-version-rollout-policy.spec.ts
🧰 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/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsxpackages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts
🧬 Code Graph Analysis (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx (1)
packages/db/src/schema/rules/environment-version-rollout.ts (1)
PolicyRuleEnvironmentVersionRollout(71-72)
⏰ 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: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx (1)
27-28: Addition looks good but check UpdatePolicy schema
Policynow carriesenvironmentVersionRollout, 👍. EnsureSCHEMA.updatePolicyincludes the same field; otherwise RHF type inference will complain at runtime.
...ice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx
Outdated
Show resolved
Hide resolved
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts
Outdated
Show resolved
Hide resolved
| const getNumReleaseTargets = async (db: Tx, releaseTargetId: string) => { | ||
| const releaseTarget = await db | ||
| .select() | ||
| .from(schema.releaseTarget) | ||
| .where(eq(schema.releaseTarget.id, releaseTargetId)) | ||
| .then(takeFirst); | ||
|
|
||
| return db | ||
| .select({ count: count() }) | ||
| .from(schema.releaseTarget) | ||
| .where( | ||
| and( | ||
| eq(schema.releaseTarget.deploymentId, releaseTarget.deploymentId), | ||
| eq(schema.releaseTarget.environmentId, releaseTarget.environmentId), | ||
| ), | ||
| ) | ||
| .then(takeFirst) | ||
| .then((r) => r.count); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-check releaseTarget before dereference
releaseTarget may be null, in which case releaseTarget.deploymentId will throw. Align with the explicit error handling used elsewhere.
- const releaseTarget = await db
- .select()
- .from(schema.releaseTarget)
- .where(eq(schema.releaseTarget.id, releaseTargetId))
- .then(takeFirst);
-
- return db
+ const releaseTarget = await db
+ .select()
+ .from(schema.releaseTarget)
+ .where(eq(schema.releaseTarget.id, releaseTargetId))
+ .then(takeFirst);
+
+ if (releaseTarget == null)
+ throw new Error(`Release target ${releaseTargetId} not found`);
+
+ return db📝 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.
| const getNumReleaseTargets = async (db: Tx, releaseTargetId: string) => { | |
| const releaseTarget = await db | |
| .select() | |
| .from(schema.releaseTarget) | |
| .where(eq(schema.releaseTarget.id, releaseTargetId)) | |
| .then(takeFirst); | |
| return db | |
| .select({ count: count() }) | |
| .from(schema.releaseTarget) | |
| .where( | |
| and( | |
| eq(schema.releaseTarget.deploymentId, releaseTarget.deploymentId), | |
| eq(schema.releaseTarget.environmentId, releaseTarget.environmentId), | |
| ), | |
| ) | |
| .then(takeFirst) | |
| .then((r) => r.count); | |
| }; | |
| const getNumReleaseTargets = async (db: Tx, releaseTargetId: string) => { | |
| const releaseTarget = await db | |
| .select() | |
| .from(schema.releaseTarget) | |
| .where(eq(schema.releaseTarget.id, releaseTargetId)) | |
| .then(takeFirst); | |
| if (releaseTarget == null) | |
| throw new Error(`Release target ${releaseTargetId} not found`); | |
| return db | |
| .select({ count: count() }) | |
| .from(schema.releaseTarget) | |
| .where( | |
| and( | |
| eq(schema.releaseTarget.deploymentId, releaseTarget.deploymentId), | |
| eq(schema.releaseTarget.environmentId, releaseTarget.environmentId), | |
| ), | |
| ) | |
| .then(takeFirst) | |
| .then((r) => r.count); | |
| }; |
🤖 Prompt for AI Agents
In
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts
around lines 57 to 75, the code dereferences releaseTarget without checking if
it is null, which can cause a runtime error. Add a null check after fetching
releaseTarget; if it is null, throw an explicit error similar to other parts of
the codebase. This prevents dereferencing null and aligns with existing error
handling practices.
There was a problem hiding this comment.
takeFirst utility function throws error if null
There was a problem hiding this comment.
@adityachoudhari26
You're absolutely right—takeFirst already guarantees a non-null result (or throws), so the extra null-check I suggested is redundant. Thanks for pointing that out, and sorry for the noise! I’ll retract the suggestion.
(I had this behavior stored in my notes from an earlier PR; seems I overlooked it here.)
(\_/)
(•.•) ?
/ >🍃 – Appreciative bunny pondering the reminder
🧠 Learnings used
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#395
File: packages/api/src/router/environment-page/resources/router.ts:40-45
Timestamp: 2025-03-24T18:46:38.894Z
Learning: The `takeFirst` utility function in the codebase (from `@ctrlplane/db`) throws an Error with message "Found non unique or inexistent value" if the result array doesn't contain exactly one element, making additional null/undefined checks unnecessary after its use.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts (1)
46-54:⚠️ Potential issuePossible runtime crash when no
approvedAtvalues – optional-chaining missing
latestApprovalRecordcan beundefined(e.g. all records haveapprovedAt = null).
Dereferencing it will throw before the nullish-coalescing fallback kicks in.- return latestApprovalRecord.approvedAt ?? version.createdAt; + return latestApprovalRecord?.approvedAt ?? version.createdAt;
🧹 Nitpick comments (3)
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts (3)
40-44: Fetch approval records in parallel for ~3× faster latencyThe three DB hits run sequentially; with
Promise.allwe can issue them concurrently and keep the code equally clear.- const allApprovalRecords = [ - ...(await getAnyApprovalRecords([version.id])), - ...(await getUserApprovalRecords([version.id])), - ...(await getRoleApprovalRecords([version.id])), - ]; + const [any, user, role] = await Promise.all([ + getAnyApprovalRecords([version.id]), + getUserApprovalRecords([version.id]), + getRoleApprovalRecords([version.id]), + ]); + const allApprovalRecords = [...any, ...user, ...role];
28-35: Prefer an explicitfor…ofloop over async-reducefor clarity
Array.reducewith async callbacks is hard to read and accidentally easy to misuse.
The intent (“break on first failure”) is clearer with a simple loop:let allApprovalsPassed = true; for (const rule of versionApprovalRules) { const { allowedCandidates } = await rule.filter([version]); if (allowedCandidates.length === 0) { allApprovalsPassed = false; break; } }
182-186:numReleaseTargetsis recalculated per release-target – cache once
getNumReleaseTargetsis already executed earlier when building the rule.
Calling it again here adds an extra query per target and can dominate rollout listing time in large deployments.
Consider passing the earlier value down or memoising the helper.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx(2 hunks)packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyFormContext.tsx
🧰 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.
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts
🧠 Learnings (1)
packages/rule-engine/src/manager/version-manager-rules/environment-version-rollout.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#395
File: packages/api/src/router/environment-page/resources/router.ts:40-45
Timestamp: 2025-03-24T18:46:38.894Z
Learning: The `takeFirst` utility function in the codebase (from `@ctrlplane/db`) throws an Error with message "Found non unique or inexistent value" if the result array doesn't contain exactly one element, making additional null/undefined checks unnecessary after its use.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests