add approval rule #454
add approval rule #454
Conversation
WalkthroughThis pull request expands the policy rule system by updating exports and schema definitions across multiple modules. It adds new approval rule files and redefines relationships within the policy schema. Several files have been modified to remove the deprecated deny window logic and replace it with new approval mechanisms. Query functions and evaluation logic in the rule engine are updated to incorporate these additional approval rules. Database schemas and SQL migrations have been introduced to support the new functionality, while logging and channel configurations receive minor adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Deployment Request
participant Eval as evaluateRepository
participant Rule as VersionApprovalRule
participant DB as ApprovalRecords Database
Client->>Eval: Initiate policy evaluation
Eval->>Rule: Call denyWindows & new approval rule functions
Rule->>DB: Query approval records (Any/User/Role)
DB-->>Rule: Return approval records
Rule->>Eval: Return filtered release decision (allowed/rejected)
Eval->>Client: Return chosen release and rejection reasons
Possibly related PRs
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 (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (5)
packages/db/src/schema/rules/deny-window.ts (3)
19-19: Consider usingdefault(null)for consistencyThe
dtendfield usesdefault(sql\NULL`)while other nullable fields in this file use.default(null)`. Consider using the latter for consistency unless there's a specific reason for using the SQL expression.- dtend: timestamp("dtend", { withTimezone: false }).default(sql`NULL`), + dtend: timestamp("dtend", { withTimezone: false }).nullable().default(null),
25-114: RRule schema definition is comprehensive but largeThe
rruleSchemadefinition is thorough and well-documented with descriptions for each field. However, it's quite large and might be clearer if extracted to a separate file for better maintainability, especially if it might be reused elsewhere.
88-93: Consider more specific typing for byweekday fieldThe
byweekdayfield accepts both an array of numbers and an array of any type. If possible, consider defining a more specific type instead ofz.any()to improve type safety.byweekday: z .array(z.number()) - .or(z.array(z.any())) + .or(z.array(z.object({ /* specific weekday object structure */ }).passthrough())) .optional() .describe("The days of the week (0=MO to 6=SU, or weekday objects)"),packages/db/src/schema/rules/approval.ts (2)
56-96:policyRuleApprovalRecordtable.Appropriate references to user/team/role for flexible assignment, plus a clear approval status and timestamps. Consider creating an index on
approvalRuleIdif you anticipate large queries.
122-146: Thoroughly test these approval schema exports.Exporting these schemas and types is logical. Consider adding or extending unit tests to confirm expected validation behavior (e.g., boundary checks for
approverId,status).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/db/src/schema/index.ts(1 hunks)packages/db/src/schema/policy-relations.ts(2 hunks)packages/db/src/schema/policy.ts(1 hunks)packages/db/src/schema/rules/approval.ts(1 hunks)packages/db/src/schema/rules/base.ts(1 hunks)packages/db/src/schema/rules/deny-window.ts(1 hunks)packages/db/src/schema/rules/index.ts(1 hunks)packages/db/src/schema/rules/rule-relations.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/db/src/schema/index.tspackages/db/src/schema/rules/base.tspackages/db/src/schema/rules/index.tspackages/db/src/schema/policy.tspackages/db/src/schema/policy-relations.tspackages/db/src/schema/rules/rule-relations.tspackages/db/src/schema/rules/deny-window.tspackages/db/src/schema/rules/approval.ts
🧬 Code Definitions (5)
packages/db/src/schema/rules/base.ts (1)
packages/db/src/schema/policy.ts (1)
policy(23-39)
packages/db/src/schema/policy-relations.ts (3)
packages/db/src/schema/policy.ts (2)
policy(23-39)policyTarget(41-52)packages/db/src/schema/rules/deny-window.ts (1)
policyRuleDenyWindow(13-21)packages/db/src/schema/rules/approval.ts (1)
policyRuleApproval(31-47)
packages/db/src/schema/rules/rule-relations.ts (3)
packages/db/src/schema/rules/deny-window.ts (1)
policyRuleDenyWindow(13-21)packages/db/src/schema/policy.ts (1)
policy(23-39)packages/db/src/schema/rules/approval.ts (2)
policyRuleApproval(31-47)policyRuleApprovalRecord(57-96)
packages/db/src/schema/rules/deny-window.ts (1)
packages/db/src/schema/rules/base.ts (2)
basePolicyRuleFields(7-17)basePolicyRuleValidationFields(20-22)
packages/db/src/schema/rules/approval.ts (1)
packages/db/src/schema/rules/base.ts (2)
basePolicyRuleFields(7-17)basePolicyRuleValidationFields(20-22)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (23)
packages/db/src/schema/rules/deny-window.ts (4)
13-21: Database schema for deny window looks well-definedThe table schema is properly structured using the base policy rule fields and includes specialized fields for deny windows. The RRule JSON storage approach with proper typing is a good choice for storing complex recurrence rules.
116-118: Good use of comment to explain type castingThe comment explaining the type casting approach for the
typedRruleSchemais helpful for future developers. This is a good practice when using complex TypeScript cast patterns.
120-128: Schema creation and field omission looks goodThe insert schema is correctly created with appropriate validation fields, and the omission of auto-generated fields (
idandcreatedAt) from the insert schema is the right approach.
129-147: Type exports and schema creation follow best practicesThe exports for create, update, and query schemas follow consistent patterns and provide good type safety with appropriate use of partial schemas for updates.
packages/db/src/schema/index.ts (1)
27-27: Export of rule schemas is properly integratedThe new export line correctly integrates the rules module with the main schema exports, making all rule-related entities accessible through the main schema module.
packages/db/src/schema/rules/index.ts (1)
1-5: Well-structured central export for rule schemasThe index file properly centralizes all rule-related exports, following the pattern used elsewhere in the codebase. This organization makes imports cleaner for consumers of these modules.
packages/db/src/schema/rules/base.ts (2)
7-17: Base policy rule fields are well-definedThe base fields provide a good foundation for all policy rule types with proper primary key, foreign key reference with cascade deletion, and timestamp handling.
20-22: Validation schema follows best practicesThe validation schema correctly uses Zod to ensure the policy ID is a valid UUID string, providing consistent validation across all rule types.
packages/db/src/schema/policy.ts (1)
103-109: No issues identified in the new export comment.These lines simply introduce or clarify the export section, and there appears to be no functional addition or removal here. The code is consistent with the rest of the file.
packages/db/src/schema/policy-relations.ts (4)
8-11: Confirm continued usage ofpolicyRuleDenyWindow.We see that
policyRuleDenyWindowis being imported and referenced again despite the broader effort to remove or deprecate deny window logic. Verify that this import remains necessary and does not introduce dead or confusing code.
14-19: Re-exportingpolicyRuleDenyWindowRelations.You are re-exporting the deny window relations while removing or deprecating the deny window functionality in other files. Double-check whether this re-export is fully intentional or should be removed to avoid inconsistencies.
28-28: Newapprovalsrelationship.This relationship is a clear addition for handling the new approval rules. No issues spotted regarding correctness or style.
40-40: No functional change in the closing bracket.No substantive modification beyond the closing bracket.
packages/db/src/schema/rules/rule-relations.ts (4)
1-4: Import statements look consistent.All imported modules appear to be correctly used below.
5-13: RevisitpolicyRuleDenyWindowRelationsif deny window logic is deprecated.While the relationship definition is valid, ensure you still need it if deny window functionality is indeed removed from other parts of the schema.
15-24:policyRuleApprovalRelationssetup looks good.This relation properly references the
policyone-to-one and thepolicyRuleApprovalRecordone-to-many. No issues found.
26-34:policyRuleApprovalRecordRelationscorrectly referencespolicyRuleApproval.Everything seems well-defined for maintaining the link between approval records and their corresponding approval rules.
packages/db/src/schema/rules/approval.ts (6)
1-4: Initial imports.Imports from Drizzle ORM and related modules are straightforward and appear correct.
22-28: Definition of theapprovalTypeenum.The enumerations ("individual", "team", "role", "any") clearly cover distinct approval pathways.
30-47:policyRuleApprovaltable structure.Fields correctly extend
basePolicyRuleFields, with a required approval type and optionalapproverId. No concurrency or logical concerns noted.
49-54:approvalStatusenum is simple and clear.Values "pending", "approved", and "rejected" adequately cover typical approval states.
98-107:policyRuleApprovalInsertSchema.Combines base fields with typed constraints (e.g.,
approvalTypeenum). Implementation aligns with the table definition.
109-120:policyRuleApprovalRecordInsertSchema.Well-formed Zod schema reflecting the table fields for references and optional fields (teamId, roleId).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/rule-engine/src/rules/version-any-approval-rule.ts (2)
9-14: Rename type to avoid confusion with built-in Record.Using a type alias named
Recordcould lead to confusion with TypeScript’s built-inRecord<K, V>utility type. Consider adopting a more specific name likeApprovalRecord.-type Record = { +type ApprovalRecord = { status: "approved" | "rejected"; userId: string; reason: string; approvedAt: Date; };
30-64: Handle partial errors gracefully.If
getApprovalRecordsthrows an error for one release, the entire filter process may fail. Consider wrapping individual calls in a try/catch if partial approval checks are desirable. Otherwise, this is a robust approach for strictly enforcing required approvals.packages/db/src/schema/rules/approval-any.ts (1)
26-35: Unique index name doesn't match indexed fields.The unique index is correctly implemented on
deploymentVersionIdanduserId, but the nameunique_rule_id_user_iddoesn't accurately reflect the fields it's indexing, which could be confusing for future maintenance.Consider renaming the index to better reflect the fields it's indexing:
- uniqueRuleIdUserId: uniqueIndex("unique_rule_id_user_id").on( + uniqueDeploymentVersionIdUserId: uniqueIndex("unique_deployment_version_id_user_id").on( t.deploymentVersionId, t.userId, ),packages/db/src/schema/rules/approval-user.ts (1)
31-46: Align omitted fields in insert schemas.
policyRuleUserApprovalInsertSchemaomits{ id, createdAt }, whilepolicyRuleUserApprovalRecordInsertSchemaomits{ id, createdAt, updatedAt }. This inconsistency could lead to confusion or alignment issues. Consider making the omitted fields uniform to avoid unexpected behavior.packages/db/src/schema/rules/rule-relations.ts (1)
1-180: Potential duplication withapproval-relations.ts.
Multiple approval relations appear in both files. Consider merging or abstracting shared logic to reduce redundancy and simplify maintenance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/db/src/schema/index.ts(1 hunks)packages/db/src/schema/policy-relations.ts(1 hunks)packages/db/src/schema/rules/approval-any.ts(1 hunks)packages/db/src/schema/rules/approval-base.ts(1 hunks)packages/db/src/schema/rules/approval-relations.ts(1 hunks)packages/db/src/schema/rules/approval-role.ts(1 hunks)packages/db/src/schema/rules/approval-team.ts(1 hunks)packages/db/src/schema/rules/approval-user.ts(1 hunks)packages/db/src/schema/rules/index.ts(1 hunks)packages/db/src/schema/rules/rule-relations.ts(1 hunks)packages/rule-engine/src/rules/version-any-approval-rule.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/index.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.
packages/rule-engine/src/rules/version-any-approval-rule.tspackages/db/src/schema/rules/approval-base.tspackages/db/src/schema/policy-relations.tspackages/db/src/schema/rules/approval-role.tspackages/db/src/schema/rules/approval-any.tspackages/db/src/schema/rules/approval-team.tspackages/db/src/schema/rules/approval-relations.tspackages/db/src/schema/rules/rule-relations.tspackages/db/src/schema/rules/approval-user.tspackages/db/src/schema/rules/index.ts
🧬 Code Definitions (7)
packages/db/src/schema/policy-relations.ts (5)
packages/db/src/schema/rules/deny-window.ts (1)
policyRuleDenyWindow(13-21)packages/db/src/schema/rules/approval-user.ts (1)
policyRuleUserApproval(11-18)packages/db/src/schema/rules/approval-team.ts (1)
policyRuleTeamApproval(17-29)packages/db/src/schema/rules/approval-role.ts (1)
policyRuleRoleApproval(11-23)packages/db/src/schema/rules/approval-any.ts (1)
policyRuleAnyApproval(16-23)
packages/db/src/schema/rules/approval-role.ts (1)
packages/db/src/schema/rules/approval-base.ts (2)
baseApprovalRecordFields(13-37)baseApprovalRecordValidationFields(40-45)
packages/db/src/schema/rules/approval-any.ts (1)
packages/db/src/schema/rules/approval-base.ts (2)
baseApprovalRecordFields(13-37)baseApprovalRecordValidationFields(40-45)
packages/db/src/schema/rules/approval-team.ts (1)
packages/db/src/schema/rules/approval-base.ts (2)
baseApprovalRecordFields(13-37)baseApprovalRecordValidationFields(40-45)
packages/db/src/schema/rules/approval-relations.ts (4)
packages/db/src/schema/rules/approval-user.ts (2)
policyRuleUserApproval(11-18)policyRuleUserApprovalRecord(21-28)packages/db/src/schema/rules/approval-team.ts (2)
policyRuleTeamApproval(17-29)policyRuleTeamApprovalRecord(32-42)packages/db/src/schema/rules/approval-role.ts (2)
policyRuleRoleApproval(11-23)policyRuleRoleApprovalRecord(26-33)packages/db/src/schema/rules/approval-any.ts (2)
policyRuleAnyApproval(16-23)policyRuleAnyApprovalRecord(26-35)
packages/db/src/schema/rules/rule-relations.ts (6)
packages/db/src/schema/rules/deny-window.ts (1)
policyRuleDenyWindow(13-21)packages/db/src/schema/policy.ts (1)
policy(23-39)packages/db/src/schema/rules/approval-user.ts (2)
policyRuleUserApproval(11-18)policyRuleUserApprovalRecord(21-28)packages/db/src/schema/rules/approval-team.ts (2)
policyRuleTeamApproval(17-29)policyRuleTeamApprovalRecord(32-42)packages/db/src/schema/rules/approval-role.ts (2)
policyRuleRoleApproval(11-23)policyRuleRoleApprovalRecord(26-33)packages/db/src/schema/rules/approval-any.ts (2)
policyRuleAnyApproval(16-23)policyRuleAnyApprovalRecord(26-35)
packages/db/src/schema/rules/approval-user.ts (1)
packages/db/src/schema/rules/approval-base.ts (2)
baseApprovalRecordFields(13-37)baseApprovalRecordValidationFields(40-45)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (22)
packages/db/src/schema/rules/index.ts (1)
1-10: Centralized exports look good.These star exports provide a convenient, single entry point for all rule-related modules. No issues found.
packages/db/src/schema/rules/approval-base.ts (3)
6-10: Consider additional approval statuses.If your use case requires more states (e.g., "pending"), expand the enum to accommodate them. Otherwise, this looks fine.
12-37: Base approval record fields appear consistent.The references to user IDs, default random IDs, and timestamps align with typical database schema patterns. No issues discovered here.
39-45: Validation fields align with database schema.Using Zod to ensure UUID format and enumerated statuses is a sound approach, preventing invalid records from being persisted. No issues found.
packages/db/src/schema/policy-relations.ts (3)
8-14: Well-structured imports for approval rules.The code now imports all approval rule types from a centralized
./rules/index.jsfile rather than directly from individual files. This approach improves code organization and maintainability.
17-18: Good practice: Re-exporting rule relations.Re-exporting entities from
./rules/index.jscentralizes imports/exports and makes it easier for consumers to access the necessary types and schemas from a single location.
27-30: Enhanced policy relations with multiple approval types.The addition of relations for various approval types (user, team, role, any) provides a more flexible and comprehensive policy system. This structure allows for granular control over approval workflows.
packages/db/src/schema/rules/approval-role.ts (4)
11-23: Well-designed role approval rule schema.The table definition for
policyRuleRoleApprovalappropriately extends the base policy rule fields and adds role-specific fields. TherequiredApprovalsCountwith a default of 1 is a sensible choice for most use cases.
26-33: Properly established record relationship.The
policyRuleRoleApprovalRecordtable correctly establishes the relationship with the approval rule viaruleIdwith cascade deletion, ensuring that records are automatically removed when the parent rule is deleted.
36-51: Comprehensive validation schemas.The validation schemas properly integrate base validation fields and add role-specific validations. The minimum count validation for
requiredApprovalsCountprevents invalid configurations.
54-69: Clear type exports and schema organization.The export pattern for create and update schemas is consistent and well-organized, making it easy for consumers to understand and use the correct types.
packages/db/src/schema/rules/approval-any.ts (3)
16-23: Well-structured any approval rule schema.The table definition for
policyRuleAnyApprovalappropriately extends the base policy rule fields with a well-documentedrequiredApprovalsCountfield that defaults to 1.
38-49: Comprehensive validation schemas.The validation schemas properly integrate base validation fields and add the necessary validation for
requiredApprovalsCountwith an appropriate minimum value.
52-82: Consistent type exports across approval rules.The export pattern for types and schemas follows the same structure used in other approval rule files, maintaining consistency throughout the codebase.
packages/db/src/schema/rules/approval-team.ts (4)
17-29: Well-designed team approval rule schema.The table definition for
policyRuleTeamApprovalappropriately extends the base policy rule fields and adds team-specific fields with proper references to the team table.
45-60: Comprehensive validation schemas.The validation schemas properly integrate base validation fields with team-specific validations, ensuring data integrity for team approval rules.
63-93: Consistent type exports and naming conventions.The export pattern for create and update schemas follows the same structure used in other approval rule files, maintaining consistency across the codebase which improves maintainability.
32-42:❓ Verification inconclusive
Consider adding a unique index for team approval records.
Unlike the
policyRuleAnyApprovalRecordtable, there's no unique index ondeploymentVersionIdanduserIdfor team approval records. This could potentially allow the same user to approve the same deployment version multiple times for a team.If this is an intentional design decision, it's fine to leave as is. Otherwise, consider adding a unique index similar to the one in the
policyRuleAnyApprovalRecordtable:
🏁 Script executed:
#!/bin/bash # Check if other approval record tables have unique indices rg -A 3 "uniqueIndex" packages/db/src/schema/rules/approval-*.tsLength of output: 927
Team Approval Record: Unique Composite Index Recommendation
It appears that the team approval record in
packages/db/src/schema/rules/approval-team.tsdoes not enforce a unique composite index on(deploymentVersionId, userId), unlike what is implemented in thepolicyRuleAnyApprovalRecordtable (see the unique index inpackages/db/src/schema/rules/approval-any.ts). This discrepancy could allow the same user to approve the same deployment version multiple times for a team, which may not be the intended behavior.
- Location:
packages/db/src/schema/rules/approval-team.ts(Lines 32–42)- Suggestion: If the design should prevent duplicate approvals, add a unique index similar to the following:
uniqueTeamApproval: uniqueIndex("unique_team_approval").on(t.deploymentVersionId, t.userId),- Note: If the absence of this index is an intentional design decision, please ensure that this behavior is well documented.
packages/db/src/schema/rules/approval-user.ts (2)
15-17: Consider specifyingonDeleteoronUpdateconstraints for user references.
It is often beneficial to define explicit actions for foreign keys to handle deletions or updates of the referenced user entity. Confirm whether you prefercascade,restrict, orset nullto maintain data consistency.
55-60: Update schemas are well-structured.
Using partial insert schemas to define patch objects ensures flexible updates without re-validating unchanged properties. This is a sound approach for incremental updates.packages/db/src/schema/rules/rule-relations.ts (2)
17-26: Add foreign key action for Deny Window references.
ThepolicyRuleDenyWindowrelation omitsonDeleteoronUpdatehandling. Unless it’s intentional, clarify whether you need to cascade or restrict changes to prevent orphaned records.
128-153: Well-structured “On behalf of team” relations.
Consistently naming relation fields and referencing the correct records helps keep the schema clear and traceable. This is a neat approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (23)
packages/db/src/schema/rules/approval-relations.ts (11)
25-35: User approval rule relations look solid.
Consider specifyingonDeleteoronUpdatefor clarity on referential actions.
37-52: User approval record relations are consistent.
Everything matches the user approval logic. You may also defineonDeletefor foreign key constraints.
54-64: Team approval rule relations appear correct.
Optional suggestion: defineonDeletefor a clearer referential policy.
66-81: Team approval record relations are well-defined.
Naming like"teamTeamApprovals"can be confusing—consider simplifying to strengthen code readability.
83-93: Role approval rule relations follow the same pattern.
All references look consistent. Consider adding explicitonDelete.
95-110: Role approval record relations are correct.
The structure aligns with the existing convention; you might want to specifyonDeleteon the foreign key.
112-122: Any approval rule relations are properly set.
No immediate concerns. Optionally specifyonDelete.
124-139: Any approval record relations are consistent.
No issues. As with other relations, consider explicit referential actions.
141-166: On-behalf-of-team relations are well-structured.
All references match the expected foreign keys.
168-193: On-behalf-of-role relations are consistent.
No major issues. Same optional note ononDeleteusage.
1-193: Overall file observation: Consider reducing duplication.
These relations share a similar pattern across user, team, role, and any. Extracting common logic might simplify maintenance.packages/db/src/schema/rules/rule-relations.ts (12)
18-27: Deny window rule relations look good.
Optionally specifyonDeleteoronUpdatefor the foreign key.
29-39: User approval rule relations are consistent.
No major concerns. Again, consider specifyingonDelete.
41-56: User approval record relations match existing patterns.
Everything is coherent. AddonDeleteif you prefer explicit referential behavior.
58-68: Team approval rule relations are correct.
Optional: define the foreign key constraint withonDelete.
70-85: Team approval record relations handle references properly.
Naming like"teamTeamApprovals"might be simplified for clarity if desired.
87-97: Role approval rule relations are well-defined.
No immediate issues—consider onDelete usage.
99-114: Role approval record relations mirror the user/team approach.
Everything is aligned. AddonDeleteif needed.
116-126: Any approval rule relations look fine.
Matches the same structure as others. OptionalonDeletecustomization.
128-143: Any approval record relations are consistent.
Maintains a parallel structure to other approval records.
145-170: On-behalf-of-team relations are coherent.
The references align well; no concerns.
172-197: On-behalf-of-role relations follow the same pattern.
Everything looks correct.
1-197: Overall file observation: DRY opportunity.
This file reiterates similar relationship patterns already defined for other entities. Consider factoring out common logic to reduce repetition.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/db/src/schema/policy-relations.ts(1 hunks)packages/db/src/schema/rules/approval-relations.ts(1 hunks)packages/db/src/schema/rules/approval-role.ts(1 hunks)packages/db/src/schema/rules/approval-user.ts(1 hunks)packages/db/src/schema/rules/rule-relations.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/db/src/schema/policy-relations.tspackages/db/src/schema/rules/approval-role.tspackages/db/src/schema/rules/approval-user.tspackages/db/src/schema/rules/approval-relations.tspackages/db/src/schema/rules/rule-relations.ts
🧬 Code Definitions (5)
packages/db/src/schema/policy-relations.ts (5)
packages/db/src/schema/rules/deny-window.ts (1)
policyRuleDenyWindow(13-21)packages/db/src/schema/rules/approval-user.ts (1)
policyRuleUserApproval(17-24)packages/db/src/schema/rules/approval-team.ts (1)
policyRuleTeamApproval(17-29)packages/db/src/schema/rules/approval-role.ts (1)
policyRuleRoleApproval(17-29)packages/db/src/schema/rules/approval-any.ts (1)
policyRuleAnyApproval(16-23)
packages/db/src/schema/rules/approval-role.ts (1)
packages/db/src/schema/rules/approval-base.ts (2)
baseApprovalRecordFields(13-37)baseApprovalRecordValidationFields(40-45)
packages/db/src/schema/rules/approval-user.ts (1)
packages/db/src/schema/rules/approval-base.ts (2)
baseApprovalRecordFields(13-37)baseApprovalRecordValidationFields(40-45)
packages/db/src/schema/rules/approval-relations.ts (4)
packages/db/src/schema/rules/approval-user.ts (2)
policyRuleUserApproval(17-24)policyRuleUserApprovalRecord(27-37)packages/db/src/schema/rules/approval-team.ts (2)
policyRuleTeamApproval(17-29)policyRuleTeamApprovalRecord(32-42)packages/db/src/schema/rules/approval-role.ts (2)
policyRuleRoleApproval(17-29)policyRuleRoleApprovalRecord(32-42)packages/db/src/schema/rules/approval-any.ts (2)
policyRuleAnyApproval(16-23)policyRuleAnyApprovalRecord(26-35)
packages/db/src/schema/rules/rule-relations.ts (6)
packages/db/src/schema/rules/deny-window.ts (1)
policyRuleDenyWindow(13-21)packages/db/src/schema/policy.ts (1)
policy(23-39)packages/db/src/schema/rules/approval-user.ts (2)
policyRuleUserApproval(17-24)policyRuleUserApprovalRecord(27-37)packages/db/src/schema/rules/approval-team.ts (2)
policyRuleTeamApproval(17-29)policyRuleTeamApprovalRecord(32-42)packages/db/src/schema/rules/approval-role.ts (2)
policyRuleRoleApproval(17-29)policyRuleRoleApprovalRecord(32-42)packages/db/src/schema/rules/approval-any.ts (2)
policyRuleAnyApproval(16-23)policyRuleAnyApprovalRecord(26-35)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (23)
packages/db/src/schema/policy-relations.ts (3)
8-14: Ensure proper usage and consistency of the new rule imports.The newly added imports from
./rules/index.jslook appropriate and align with the re-export strategy. Just confirm that none of these imports are redundant or unused.
17-18: Re-exporting rules centralizes module access.Re-exporting everything from
./rules/index.jsis useful for discoverability. Ensure there's no name collision or confusion if multiple items share similar naming conventions inindex.js.
27-30:❓ Verification inconclusive
Verify each approval relation references a valid policy ID.
All new relations (
userApprovals,teamApprovals,roleApprovals,anyApprovals) appear correct. Confirm that each respective table includes a foreign key referencing the correct policy to avoid broken relations.
🏁 Script executed:
#!/bin/bash # Description: Verify that each approval rule table contains a policyId foreign key. rg -A 10 'pgTable\("policy_rule_(user|team|role|any)_approval".*\{' packages/db/src/schema/rules | grep -i "policyId"Length of output: 117
Action Required: Verify Foreign Key Constraints on Approval Tables
Please ensure that each approval relation (userApprovals,teamApprovals,roleApprovals,anyApprovals) correctly references a valid policy identifier. Although our initial automated search did not return explicit matches for a foreign key such aspolicyIdorpolicy_id, this might be due to the context or formatting of the schema definitions. Manually verify in the files underpackages/db/src/schema/rulesthat each table (e.g.policy_rule_user_approval,policy_rule_team_approval,policy_rule_role_approval,policy_rule_any_approval) includes the appropriate foreign key constraint to the policy entity.
🏁 Script executed:
#!/bin/bash # Re-run search with increased context for foreign key definitions in approval tables. rg -A30 'pgTable\("policy_rule_(user|team|role|any)_approval"' packages/db/src/schema/rules | grep -Ei "policy(id|_id)"Length of output: 121
Action Required: Manual Verification of Foreign Key References in Approval Tables
The automated queries did not return explicit evidence that the approval tables (i.e.,policy_rule_user_approval,policy_rule_team_approval,policy_rule_role_approval,policy_rule_any_approval) include a foreign key referencing the valid policy ID. Please manually verify in the schema files underpackages/db/src/schema/rulesthat each table definition contains the appropriate foreign key constraint (commonly defined with a column likepolicyIdorpolicy_idreferencing the policy entity). This check is essential to prevent broken relations.packages/db/src/schema/rules/approval-role.ts (8)
1-4: No issues with the import statements.Imports for types and utilities look correct and consistent with the project's setup.
31-42: Cascading delete ensures data integrity for approval record dependencies.Linking
ruleIdwith{ onDelete: "cascade" }is beneficial for maintaining data consistency. This approach is good for automatically cleaning up dependent records.
44-53: Validation schema for role approval rule looks comprehensive.Using
int().min(1).default(1)prevents invalid zero or negative approval counts. OmittingidandcreatedAtfrom insertion is standard. No issues identified.
54-61: Role approval record validation matches the base record fields.The schema includes
ruleIdas a required UUID and omits autogen fields. Looks consistent with the overall approach.
62-72: Create schemas exported correctly.These create schemas fully align with
policyRuleRoleApprovalandpolicyRuleRoleApprovalRecord. No concerns found.
74-85: Partial update schemas facilitate flexible updates.Adopting partial versions of the create schemas is a straightforward approach to updating only select fields. Implementation is good.
87-93: Exported model types follow the typical Drizzle pattern.The inferred model types are correct and help keep code strongly typed. No issues found here.
6-29:✅ Verification successful
Confirm that the base fields include the policy reference.
policyRuleRoleApprovalreferencesroleIdbut notpolicyIdexplicitly. Verify thatbasePolicyRuleFieldscarries the necessary foreign key to link the rule to a policy, if required by design.
🏁 Script executed:
#!/bin/bash # Checking the base fields for a policyId reference rg -A 5 'export const basePolicyRuleFields' packages/db/src/schema/rulesLength of output: 486
Base Field Verification: Policy Reference Confirmed
The
basePolicyRuleFieldsobject (defined inpackages/db/src/schema/rules/base.ts) includes thepolicyIdfield with the proper foreign key reference topolicy.id. This confirms that the design requirement is met, and no explicit addition of apolicyIdinpolicyRuleRoleApprovalis needed.packages/db/src/schema/rules/approval-user.ts (8)
1-5: Imports align with the new user approval functionality.All imported modules are relevant to user-based approval definitions, no extraneous imports observed.
26-37: Cascading rule deletion is handled properly.The approval record references the parent rule with
{ onDelete: "cascade" }to ensure dependent records are cleaned up. This is an effective approach for data integrity.
39-46: Insert schema for user approval rule looks good.Fields from
basePolicyRuleValidationFieldsplususerIdform a well-defined schema. OmittingidandcreatedAtis correct for insertion.
48-55: User approval record insert schema is consistent with the base.Requires
ruleIdas a UUID and omits auto-generated fields. No concerns here.
56-66: Create schemas for user approvals are straightforward.They reference the correct validation schemas and produce typed objects. No issues found.
68-79: Partial schemas for updates maintain logical consistency.Using partial variants of insert schemas matches typical update patterns and ensures flexible field updates.
81-87: Inferred model types finalize the comprehensive approach.Exporting types via
InferSelectModelis a clean way to maintain a strongly typed schema across the codebase.
6-24:✅ Verification successful
Validate that the base policy fields link to the correct policy.
policyRuleUserApprovalincludes auserId, but confirmbasePolicyRuleFieldshas a reference to connect this rule to a policy if necessary.
🏁 Script executed:
#!/bin/bash # Checking the base fields for a policyId reference rg -A 5 'export const basePolicyRuleFields' packages/db/src/schema/rulesLength of output: 486
Policy Rule Fields Verification Complete
The
basePolicyRuleFieldsalready include apolicyIdthat correctly referencespolicy.id(with cascading deletion enabled). This confirms that the necessary linkage to a policy is in place. No changes are required.packages/db/src/schema/rules/approval-relations.ts (1)
1-23: All imports look good.
No issues detected with these import statements.packages/db/src/schema/rules/rule-relations.ts (3)
1-2: Imports from "drizzle-orm" are straightforward.
3-3: Policy import statement is correct.
4-16: Additional imports from index.js appear valid.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/rule-engine/src/evaluate.ts (2)
28-39: Consider input validation forrequiredApprovalsCount.In
versionAnyApprovalRule, theapproval.requiredApprovalsCountis passed directly intominApprovalswithout any safeguards. If a policy has invalid (e.g., negative) values, the logic may produce unexpected results. Consider adding a sanity check or default to a minimum of 1.const versionAnyApprovalRule = ( approvalRules?: Policy["versionAnyApprovals"] | null, ) => { if (approvalRules == null) return []; return approvalRules.map( (approval) => new VersionApprovalRule({ - minApprovals: approval.requiredApprovalsCount, + minApprovals: + approval.requiredApprovalsCount && approval.requiredApprovalsCount > 0 + ? approval.requiredApprovalsCount + : 1, getApprovalRecords: getAnyApprovalRecords, }), ); };
41-52: Replicate validation strategy for role-based approvals.Like the previous function, consider verifying
approval.requiredApprovalsCountto avoid the edge case where it might be zero or negative.const versionRoleApprovalRule = ( approvalRules?: Policy["versionRoleApprovals"] | null, ) => { if (approvalRules == null) return []; return approvalRules.map( (approval) => new VersionApprovalRule({ - minApprovals: approval.requiredApprovalsCount, + minApprovals: + approval.requiredApprovalsCount && approval.requiredApprovalsCount > 0 + ? approval.requiredApprovalsCount + : 1, getApprovalRecords: getRoleApprovalRecords, }), ); };packages/rule-engine/src/rules/version-approval-rule.ts (2)
14-19:Recordtype clarity.Defining a clear, minimal structure for each record fosters readability. If additional metadata (e.g., timestamps) is likely needed in future, consider extension or a separate type.
26-30: Configure defaults inVersionApprovalRuleOptions.While
minApprovalscan be a parameter, you might consider defaulting it to1or the minimum valid integer if the input is missing or invalid.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/db/src/schema/policy-relations.ts(2 hunks)packages/db/src/schema/rules/approval-base.ts(1 hunks)packages/db/src/schema/rules/approval-team.ts(1 hunks)packages/db/src/schema/rules/index.ts(1 hunks)packages/db/src/schema/rules/rule-relations.ts(1 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(1 hunks)packages/rule-engine/src/evaluate.ts(3 hunks)packages/rule-engine/src/rules/version-approval-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/rules/approval-base.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.
packages/rule-engine/src/db/get-applicable-policies.tspackages/rule-engine/src/types.tspackages/db/src/schema/rules/index.tspackages/rule-engine/src/evaluate.tspackages/db/src/schema/policy-relations.tspackages/rule-engine/src/rules/version-approval-rule.tspackages/db/src/schema/rules/rule-relations.tspackages/db/src/schema/rules/approval-team.ts
🧬 Code Definitions (4)
packages/rule-engine/src/evaluate.ts (2)
packages/rule-engine/src/types.ts (1)
Policy(66-72)packages/rule-engine/src/rules/version-approval-rule.ts (4)
VersionApprovalRule(32-72)getAnyApprovalRecords(74-88)getRoleApprovalRecords(90-104)getUserApprovalRecords(106-120)
packages/db/src/schema/policy-relations.ts (3)
packages/db/src/schema/rules/approval-user.ts (1)
policyRuleUserApproval(17-24)packages/db/src/schema/rules/approval-role.ts (1)
policyRuleRoleApproval(17-29)packages/db/src/schema/rules/approval-any.ts (1)
policyRuleAnyApproval(16-23)
packages/rule-engine/src/rules/version-approval-rule.ts (1)
packages/rule-engine/src/types.ts (3)
DeploymentResourceContext(37-42)DeploymentResourceRule(58-64)DeploymentResourceRuleResult(44-47)
packages/db/src/schema/rules/rule-relations.ts (3)
packages/db/src/schema/rules/approval-user.ts (2)
policyRuleUserApprovalRecord(27-37)policyRuleUserApproval(17-24)packages/db/src/schema/rules/approval-role.ts (2)
policyRuleRoleApprovalRecord(32-42)policyRuleRoleApproval(17-29)packages/db/src/schema/rules/approval-any.ts (2)
policyRuleAnyApprovalRecord(26-35)policyRuleAnyApproval(16-23)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (23)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
118-125: Approval rules integration looks good!The query has been properly enhanced to include the necessary approval-related data:
versionAnyApprovals,versionRoleApprovals, andversionUserApprovals. This aligns with the schema changes and enables the rule engine to evaluate deployment policies using the new approval records.packages/rule-engine/src/types.ts (1)
69-71: Policy type appropriately updated with approval relationsThe Policy type has been correctly extended with the new approval-related properties, ensuring type safety when working with the approval rules data returned by the enhanced query in
getApplicablePolicies.packages/db/src/schema/rules/index.ts (1)
1-10: Well-structured centralized exports for rulesThis new index file provides a single point of access for all rule-related schemas and components, making imports cleaner throughout the codebase. The organization of exports is logical - grouping base rules, deny windows, and the various approval types together.
packages/db/src/schema/policy-relations.ts (3)
8-13: Clean import organization for approval rulesThe import structure is well-organized, bringing in all the necessary approval rule tables from the new centralized exports.
16-17: Helpful re-export of rule relationsThis re-export makes the rule relations accessible to consumers of the policy-relations module, maintaining a clean and consistent API.
30-33: Complete policy relations for approval rulesThe relations are correctly defined as many-to-one relationships between policies and their approval rules, covering all approval types (user, role, and any). This completes the database schema required for the approval functionality.
packages/rule-engine/src/evaluate.ts (2)
9-14: Imports for version approval rule look good.These named imports from
"./rules/version-approval-rule.js"(getAnyApprovalRecords,getRoleApprovalRecords,getUserApprovalRecords,VersionApprovalRule) confirm that the respective functions and class are correctly pulled in for use in the local file. No immediate issues stand out.
131-136:❓ Verification inconclusive
Order of rule composition may impact short-circuit behavior.
You’re combining deny windows with approval rules in an array here. Confirm that the
RuleEngineevaluates them in the desired order. If denies must override approvals (or vice versa), verify that the evaluation logic aligns with your intended precedence.
🏁 Script executed:
#!/bin/bash # Description: Show usage of the new rules composition within the RuleEngine class to confirm short-circuit logic if any. rg -A 15 "class RuleEngine"Length of output: 1124
Verify that rule evaluation order ensures intended overriding behavior
The
rulesarray inpackages/rule-engine/src/evaluate.tsis constructed with the deny rules (fromdenyWindows(policy)) listed before the approval rules. This ordering suggests that deny conditions would be checked first if the engine processes the array sequentially. However, the current implementation of theRuleEngine(as seen in its constructor inpackages/rule-engine/src/rule-engine.ts) does not explicitly document or enforce any short-circuit behavior.
- Confirm that the evaluation in
RuleEngineindeed respects the order of therulesarray.- Ensure that this ordering matches your intended precedence (e.g., denies overriding approvals) during rule evaluation.
- Consider adding or updating tests and documentation to clarify and verify how short-circuit logic is handled.
packages/rule-engine/src/rules/version-approval-rule.ts (6)
1-4: Imports are appropriate for DB-related rule logic.You’re correctly importing
inArrayand database references for fetching approval records. Ensure that the Drizzle-ORM-based query methods are reliably handling potential connection or query errors, though.
21-24:GetApprovalRecordsFuncis well-defined.This function type is straightforward and enforces a consistent contract for retrieving records.
32-72:VersionApprovalRulelogic is solid.
- Collects version IDs.
- Fetches approval records.
- Filters out releases with any rejections.
- Ensures approvals meet
minApprovals.It's a well-organized approach. Just watch for potential concurrency impacts if multiple users update or reject a deployment simultaneously. Optionally, you could add caching or concurrent checks if performance is a concern at scale.
74-88:getAnyApprovalRecordsquery.The usage of
db.query.policyRuleAnyApprovalRecord.findManywithinArrayis straightforward. No apparent security concerns as parameter interpolation is handled by Drizzle-ORM.
90-104:getRoleApprovalRecordsapproach is consistent.Mirrors the logic from
getAnyApprovalRecords. Good consistency.
106-120:getUserApprovalRecordsapproach is consistent.Identical pattern for user approvals. The code is coherent and symmetrical with the other two retrieval functions.
packages/db/src/schema/rules/rule-relations.ts (9)
1-16: File structure and imports are consistent.Sensible grouping of relevant modules (
user,deploymentVersion,approvalrules). This maintains clarity for new relations.
18-23:userApprovalRelationsdefinitions align with naming.Associating
userwith multiple approval record tables here is correct. The usage ofmanysuits a standard one-to-many relationship.
25-33:deploymentVersionApprovalRelationsdefinitions are accurate.Setting up each of these one-to-many relationships (user, role, any) properly ties a
deploymentVersionto multiple approval records.
35-41:policyRuleUserApprovalRelationsis straightforward.Mapping the user approval rule to its records fosters clarity. No concerns here.
61-67:policyRuleRoleApprovalRelationsparallels the user approach.This consistently connects the role approval rule to its records.
87-92:policyRuleAnyApprovalRelationsis a proper 1-to-many.For “any” approval, the relationship with records is consistent with the rest.
95-107:policyRuleAnyApprovalRecordRelationsis consistent with pattern.Similar to user/role approval records, referencing user and deployment version. The code consistently implements one-to-one for the record’s needed data.
43-59:✅ Verification successful
policyRuleUserApprovalRecordRelationsensures correct references.Correlating user, deployment version, and the rule helps keep track of who approved what. Make sure onDelete cascades are set as needed to maintain referential integrity.
🏁 Script executed:
#!/bin/bash # Description: Check that policyRuleUserApprovalRecord config has the correct cascade policy. rg "policyRuleUserApprovalRecord" -A 10Length of output: 8575
Ensure Cascade Integrity in Approval Record Relations
The relation configuration in
packages/db/src/schema/rules/rule-relations.tsforpolicyRuleUserApprovalRecordRelationscorrectly establishes the connections to the user, deployment version, and the approval rule. I've verified that the onDelete cascade is properly enforced in the table definition (see theruleIdfield configuration inpackages/db/src/schema/rules/approval-user.ts), which maintains referential integrity.No changes are needed here.
69-85:✅ Verification successful
policyRuleRoleApprovalRecordRelationsreferences look fine.As with user approval records, referencing user, deployment version, and the rule is correct. Verify whether role references also have needed cascade or set-null constraints.
🏁 Script executed:
#!/bin/bash # Searching how role references are defined and whether they cascade properly rg "policyRuleRoleApprovalRecord" -A 10Length of output: 9198
Verified Role Reference Cascade Constraints Are Configured Correctly
- The foreign key for the rule in
policyRuleRoleApprovalRecordis defined inpackages/db/src/schema/rules/approval-role.tswith a cascade deletion policy:
- This matches the behavior of user approval records and follows the expected cascading setup.
- No changes are required in the
policyRuleRoleApprovalRecordRelationsas the cascade constraints have been correctly applied.
| const versionUserApprovalRule = ( | ||
| approvalRules?: Policy["versionUserApprovals"] | null, | ||
| ) => { | ||
| if (approvalRules == null) return []; | ||
| return approvalRules.map( | ||
| () => | ||
| new VersionApprovalRule({ | ||
| minApprovals: 1, | ||
| getApprovalRecords: getUserApprovalRecords, | ||
| }), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm ignoring approval data for user approvals is intentional.
Here, versionUserApprovalRule always sets minApprovals: 1 and ignores any fields in the approval object. If your use case requires varying counts, consider reading the approval argument as in the other functions.
const versionUserApprovalRule = (
approvalRules?: Policy["versionUserApprovals"] | null,
) => {
if (approvalRules == null) return [];
return approvalRules.map(
- () =>
+ (approval) =>
new VersionApprovalRule({
- minApprovals: 1,
+ minApprovals: approval.requiredApprovalsCount ?? 1,
getApprovalRecords: getUserApprovalRecords,
}),
);
};📝 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 versionUserApprovalRule = ( | |
| approvalRules?: Policy["versionUserApprovals"] | null, | |
| ) => { | |
| if (approvalRules == null) return []; | |
| return approvalRules.map( | |
| () => | |
| new VersionApprovalRule({ | |
| minApprovals: 1, | |
| getApprovalRecords: getUserApprovalRecords, | |
| }), | |
| ); | |
| }; | |
| const versionUserApprovalRule = ( | |
| approvalRules?: Policy["versionUserApprovals"] | null, | |
| ) => { | |
| if (approvalRules == null) return []; | |
| return approvalRules.map( | |
| (approval) => | |
| new VersionApprovalRule({ | |
| minApprovals: approval.requiredApprovalsCount ?? 1, | |
| getApprovalRecords: getUserApprovalRecords, | |
| }), | |
| ); | |
| }; |
| // import type { InferSelectModel } from "drizzle-orm"; | ||
| // import { integer, pgTable, uuid } from "drizzle-orm/pg-core"; | ||
| // import { createInsertSchema } from "drizzle-zod"; | ||
| // import { z } from "zod"; | ||
|
|
||
| // import { team } from "../team.js"; | ||
| // import { | ||
| // baseApprovalRecordFields, | ||
| // baseApprovalRecordValidationFields, | ||
| // } from "./approval-base.js"; | ||
| // import { | ||
| // basePolicyRuleFields, | ||
| // basePolicyRuleValidationFields, | ||
| // } from "./base.js"; | ||
|
|
||
| // // Team approval rule - requires approval from members of a team | ||
| // export const policyRuleTeamApproval = pgTable("policy_rule_team_approval", { | ||
| // ...basePolicyRuleFields, | ||
|
|
||
| // // Team whose members can approve | ||
| // teamId: uuid("team_id") | ||
| // .notNull() | ||
| // .references(() => team.id), | ||
|
|
||
| // // Minimum number of approvals required from team members | ||
| // requiredApprovalsCount: integer("required_approvals_count") | ||
| // .notNull() | ||
| // .default(1), | ||
| // }); | ||
|
|
||
| // // Approval records specific to team approval rules | ||
| // export const policyRuleTeamApprovalRecord = pgTable( | ||
| // "policy_rule_team_approval_record", | ||
| // { | ||
| // ...baseApprovalRecordFields, | ||
|
|
||
| // // Link to the team approval rule | ||
| // ruleId: uuid("rule_id") | ||
| // .notNull() | ||
| // .references(() => policyRuleTeamApproval.id, { onDelete: "cascade" }), | ||
| // }, | ||
| // ); | ||
|
|
||
| // // Validation schemas | ||
| // export const policyRuleTeamApprovalInsertSchema = createInsertSchema( | ||
| // policyRuleTeamApproval, | ||
| // { | ||
| // ...basePolicyRuleValidationFields, | ||
| // teamId: z.string().uuid(), | ||
| // requiredApprovalsCount: z.number().int().min(1).default(1), | ||
| // }, | ||
| // ).omit({ id: true, createdAt: true }); | ||
|
|
||
| // export const policyRuleTeamApprovalRecordInsertSchema = createInsertSchema( | ||
| // policyRuleTeamApprovalRecord, | ||
| // { | ||
| // ...baseApprovalRecordValidationFields, | ||
| // ruleId: z.string().uuid(), | ||
| // }, | ||
| // ).omit({ id: true, createdAt: true, updatedAt: true }); | ||
|
|
||
| // // Export create schemas | ||
| // export const createPolicyRuleTeamApproval = policyRuleTeamApprovalInsertSchema; | ||
| // export type CreatePolicyRuleTeamApproval = z.infer< | ||
| // typeof createPolicyRuleTeamApproval | ||
| // >; | ||
|
|
||
| // export const createPolicyRuleTeamApprovalRecord = | ||
| // policyRuleTeamApprovalRecordInsertSchema; | ||
| // export type CreatePolicyRuleTeamApprovalRecord = z.infer< | ||
| // typeof createPolicyRuleTeamApprovalRecord | ||
| // >; | ||
|
|
||
| // // Export update schemas | ||
| // export const updatePolicyRuleTeamApproval = | ||
| // policyRuleTeamApprovalInsertSchema.partial(); | ||
| // export type UpdatePolicyRuleTeamApproval = z.infer< | ||
| // typeof updatePolicyRuleTeamApproval | ||
| // >; | ||
|
|
||
| // export const updatePolicyRuleTeamApprovalRecord = | ||
| // policyRuleTeamApprovalRecordInsertSchema.partial(); | ||
| // export type UpdatePolicyRuleTeamApprovalRecord = z.infer< | ||
| // typeof updatePolicyRuleTeamApprovalRecord | ||
| // >; | ||
|
|
||
| // // Export model types | ||
| // export type PolicyRuleTeamApproval = InferSelectModel< | ||
| // typeof policyRuleTeamApproval | ||
| // >; | ||
| // export type PolicyRuleTeamApprovalRecord = InferSelectModel< | ||
| // typeof policyRuleTeamApprovalRecord | ||
| // >; |
There was a problem hiding this comment.
Entire file is commented out and needs to be uncommented.
The entire file appears to be commented out with // at the beginning of each line. This will prevent any of the code from being functional. The code itself looks well-structured and follows best practices, but it needs to be uncommented to work properly.
Uncomment all lines in the file to make the schema definitions, validation schemas, and exports functional. Without uncommenting, the approval rule system for teams won't work as intended.
-// import type { InferSelectModel } from "drizzle-orm";
-// import { integer, pgTable, uuid } from "drizzle-orm/pg-core";
-// import { createInsertSchema } from "drizzle-zod";
-// import { z } from "zod";
+import type { InferSelectModel } from "drizzle-orm";
+import { integer, pgTable, uuid } from "drizzle-orm/pg-core";
+import { createInsertSchema } from "drizzle-zod";
+import { z } from "zod";
-// import { team } from "../team.js";
-// import {
-// baseApprovalRecordFields,
-// baseApprovalRecordValidationFields,
-// } from "./approval-base.js";
-// import {
-// basePolicyRuleFields,
-// basePolicyRuleValidationFields,
-// } from "./base.js";
+import { team } from "../team.js";
+import {
+ baseApprovalRecordFields,
+ baseApprovalRecordValidationFields,
+} from "./approval-base.js";
+import {
+ basePolicyRuleFields,
+ basePolicyRuleValidationFields,
+} from "./base.js";
-// // Team approval rule - requires approval from members of a team
-// export const policyRuleTeamApproval = pgTable("policy_rule_team_approval", {
-// ...basePolicyRuleFields,
+// Team approval rule - requires approval from members of a team
+export const policyRuleTeamApproval = pgTable("policy_rule_team_approval", {
+ ...basePolicyRuleFields,(and so on for the rest of the file)
📝 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.
| // import type { InferSelectModel } from "drizzle-orm"; | |
| // import { integer, pgTable, uuid } from "drizzle-orm/pg-core"; | |
| // import { createInsertSchema } from "drizzle-zod"; | |
| // import { z } from "zod"; | |
| // import { team } from "../team.js"; | |
| // import { | |
| // baseApprovalRecordFields, | |
| // baseApprovalRecordValidationFields, | |
| // } from "./approval-base.js"; | |
| // import { | |
| // basePolicyRuleFields, | |
| // basePolicyRuleValidationFields, | |
| // } from "./base.js"; | |
| // // Team approval rule - requires approval from members of a team | |
| // export const policyRuleTeamApproval = pgTable("policy_rule_team_approval", { | |
| // ...basePolicyRuleFields, | |
| // // Team whose members can approve | |
| // teamId: uuid("team_id") | |
| // .notNull() | |
| // .references(() => team.id), | |
| // // Minimum number of approvals required from team members | |
| // requiredApprovalsCount: integer("required_approvals_count") | |
| // .notNull() | |
| // .default(1), | |
| // }); | |
| // // Approval records specific to team approval rules | |
| // export const policyRuleTeamApprovalRecord = pgTable( | |
| // "policy_rule_team_approval_record", | |
| // { | |
| // ...baseApprovalRecordFields, | |
| // // Link to the team approval rule | |
| // ruleId: uuid("rule_id") | |
| // .notNull() | |
| // .references(() => policyRuleTeamApproval.id, { onDelete: "cascade" }), | |
| // }, | |
| // ); | |
| // // Validation schemas | |
| // export const policyRuleTeamApprovalInsertSchema = createInsertSchema( | |
| // policyRuleTeamApproval, | |
| // { | |
| // ...basePolicyRuleValidationFields, | |
| // teamId: z.string().uuid(), | |
| // requiredApprovalsCount: z.number().int().min(1).default(1), | |
| // }, | |
| // ).omit({ id: true, createdAt: true }); | |
| // export const policyRuleTeamApprovalRecordInsertSchema = createInsertSchema( | |
| // policyRuleTeamApprovalRecord, | |
| // { | |
| // ...baseApprovalRecordValidationFields, | |
| // ruleId: z.string().uuid(), | |
| // }, | |
| // ).omit({ id: true, createdAt: true, updatedAt: true }); | |
| // // Export create schemas | |
| // export const createPolicyRuleTeamApproval = policyRuleTeamApprovalInsertSchema; | |
| // export type CreatePolicyRuleTeamApproval = z.infer< | |
| // typeof createPolicyRuleTeamApproval | |
| // >; | |
| // export const createPolicyRuleTeamApprovalRecord = | |
| // policyRuleTeamApprovalRecordInsertSchema; | |
| // export type CreatePolicyRuleTeamApprovalRecord = z.infer< | |
| // typeof createPolicyRuleTeamApprovalRecord | |
| // >; | |
| // // Export update schemas | |
| // export const updatePolicyRuleTeamApproval = | |
| // policyRuleTeamApprovalInsertSchema.partial(); | |
| // export type UpdatePolicyRuleTeamApproval = z.infer< | |
| // typeof updatePolicyRuleTeamApproval | |
| // >; | |
| // export const updatePolicyRuleTeamApprovalRecord = | |
| // policyRuleTeamApprovalRecordInsertSchema.partial(); | |
| // export type UpdatePolicyRuleTeamApprovalRecord = z.infer< | |
| // typeof updatePolicyRuleTeamApprovalRecord | |
| // >; | |
| // // Export model types | |
| // export type PolicyRuleTeamApproval = InferSelectModel< | |
| // typeof policyRuleTeamApproval | |
| // >; | |
| // export type PolicyRuleTeamApprovalRecord = InferSelectModel< | |
| // typeof policyRuleTeamApprovalRecord | |
| // >; | |
| import type { InferSelectModel } from "drizzle-orm"; | |
| import { integer, pgTable, uuid } from "drizzle-orm/pg-core"; | |
| import { createInsertSchema } from "drizzle-zod"; | |
| import { z } from "zod"; | |
| import { team } from "../team.js"; | |
| import { | |
| baseApprovalRecordFields, | |
| baseApprovalRecordValidationFields, | |
| } from "./approval-base.js"; | |
| import { | |
| basePolicyRuleFields, | |
| basePolicyRuleValidationFields, | |
| } from "./base.js"; | |
| // Team approval rule - requires approval from members of a team | |
| export const policyRuleTeamApproval = pgTable("policy_rule_team_approval", { | |
| ...basePolicyRuleFields, | |
| // Team whose members can approve | |
| teamId: uuid("team_id") | |
| .notNull() | |
| .references(() => team.id), | |
| // Minimum number of approvals required from team members | |
| requiredApprovalsCount: integer("required_approvals_count") | |
| .notNull() | |
| .default(1), | |
| }); | |
| // Approval records specific to team approval rules | |
| export const policyRuleTeamApprovalRecord = pgTable( | |
| "policy_rule_team_approval_record", | |
| { | |
| ...baseApprovalRecordFields, | |
| // Link to the team approval rule | |
| ruleId: uuid("rule_id") | |
| .notNull() | |
| .references(() => policyRuleTeamApproval.id, { onDelete: "cascade" }), | |
| }, | |
| ); | |
| // Validation schemas | |
| export const policyRuleTeamApprovalInsertSchema = createInsertSchema( | |
| policyRuleTeamApproval, | |
| { | |
| ...basePolicyRuleValidationFields, | |
| teamId: z.string().uuid(), | |
| requiredApprovalsCount: z.number().int().min(1).default(1), | |
| }, | |
| ).omit({ id: true, createdAt: true }); | |
| export const policyRuleTeamApprovalRecordInsertSchema = createInsertSchema( | |
| policyRuleTeamApprovalRecord, | |
| { | |
| ...baseApprovalRecordValidationFields, | |
| ruleId: z.string().uuid(), | |
| }, | |
| ).omit({ id: true, createdAt: true, updatedAt: true }); | |
| // Export create schemas | |
| export const createPolicyRuleTeamApproval = policyRuleTeamApprovalInsertSchema; | |
| export type CreatePolicyRuleTeamApproval = z.infer< | |
| typeof createPolicyRuleTeamApproval | |
| >; | |
| export const createPolicyRuleTeamApprovalRecord = | |
| policyRuleTeamApprovalRecordInsertSchema; | |
| export type CreatePolicyRuleTeamApprovalRecord = z.infer< | |
| typeof createPolicyRuleTeamApprovalRecord | |
| >; | |
| // Export update schemas | |
| export const updatePolicyRuleTeamApproval = | |
| policyRuleTeamApprovalInsertSchema.partial(); | |
| export type UpdatePolicyRuleTeamApproval = z.infer< | |
| typeof updatePolicyRuleTeamApproval | |
| >; | |
| export const updatePolicyRuleTeamApprovalRecord = | |
| policyRuleTeamApprovalRecordInsertSchema.partial(); | |
| export type UpdatePolicyRuleTeamApprovalRecord = z.infer< | |
| typeof updatePolicyRuleTeamApprovalRecord | |
| >; | |
| // Export model types | |
| export type PolicyRuleTeamApproval = InferSelectModel< | |
| typeof policyRuleTeamApproval | |
| >; | |
| export type PolicyRuleTeamApprovalRecord = InferSelectModel< | |
| typeof policyRuleTeamApprovalRecord | |
| >; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
apps/event-worker/src/workers/policy-evaluate.ts (2)
68-68: Use a dedicated logger instead ofconsole.log.While
console.logaids debugging, switching to a structured logger (likepino,winston, etc.) offers better log level management and consistency in production environments.
89-91: Verify the safety of logged data.
rejectionReasonscould include contextual or user-provided information. Ensure that no PII or confidential data is leaked in logs.packages/rule-engine/src/evaluate.ts (3)
28-39: Provide a default forminApprovals.If
approval.requiredApprovalsCountis not set,VersionApprovalRulemight receiveundefined. Consider a safe fallback likeapproval.requiredApprovalsCount ?? 1.- minApprovals: approval.requiredApprovalsCount, + minApprovals: approval.requiredApprovalsCount ?? 1,
40-52: Provide a default forminApprovals.Same concern as with
versionAnyApprovalRule. Ensure it’s always an integer to avoid issues ifrequiredApprovalsCountis missing.- minApprovals: approval.requiredApprovalsCount, + minApprovals: approval.requiredApprovalsCount ?? 1,
131-137: Be mindful when logging entire policy.If the policy object contains sensitive fields, consider redacting or removing them from logs before printing.
packages/db/drizzle/0084_abnormal_lady_ursula.sql (5)
6-6: Statement Breakpoint MarkerThe line
--> statement-breakpointappears to be used as a visual separator between logical sections. Ensure that these markers are either properly formatted as SQL comments (for example, using--) or confirmed to be acceptable by the migration tool to avoid any accidental execution issues.
7-12: Creation ofpolicy_rule_user_approvalTableThe table definition for
policy_rule_user_approvalis clear and defines all necessary columns with appropriate types and defaults (e.g., usinggen_random_uuid()for the primary key andnow()for timestamp defaults).
Consider whether adding explicit indexes on the foreign key columns (such as"policy_id"and"user_id") might benefit query performance during frequent joins.
14-24: Creation ofpolicy_rule_user_approval_recordTableThis table includes detailed fields for tracking approval records, such as
status(using the new enum), timestamps, and references via"rule_id".
A couple of suggestions:
- Confirm that the default of
NULLfor"approved_at"is intentional and handled correctly by your application logic.- Evaluate if additional indexing on columns like
"deployment_version_id"or"rule_id"might be beneficial for read performance based on expected query patterns.
26-32: Creation ofpolicy_rule_role_approvalTableThe definition of this table is concise and includes a sensible default of
1for"required_approvals_count".
Ensure that the foreign key"policy_id"(which will be linked later via a constraint) is indexed if it will be heavily used in joins.
53-62: Creation ofpolicy_rule_any_approval_recordTableThis table is defined to record generic approval actions. The column definitions (including the use of the
approval_statusenum) are consistent with other record tables.
Consider if the"updated_at"column needs automatic management (e.g., via a trigger) if the application requires tracking modification times.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/workers/policy-evaluate.ts(2 hunks)packages/db/drizzle/0084_abnormal_lady_ursula.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/rules/rule-relations.ts(1 hunks)packages/rule-engine/src/evaluate.ts(3 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/policy-evaluate.tspackages/rule-engine/src/evaluate.tspackages/db/src/schema/rules/rule-relations.ts
🧬 Code Definitions (2)
apps/event-worker/src/workers/policy-evaluate.ts (1)
packages/rule-engine/src/evaluate.ts (1)
evaluateRepository(102-141)
packages/db/src/schema/rules/rule-relations.ts (5)
packages/db/src/schema/rules/approval-user.ts (2)
policyRuleUserApprovalRecord(27-37)policyRuleUserApproval(17-24)packages/db/src/schema/rules/approval-role.ts (2)
policyRuleRoleApprovalRecord(32-42)policyRuleRoleApproval(17-29)packages/db/src/schema/rules/approval-any.ts (2)
policyRuleAnyApprovalRecord(26-35)policyRuleAnyApproval(16-23)packages/db/src/schema/rules/deny-window.ts (1)
policyRuleDenyWindow(13-21)packages/db/src/schema/policy.ts (1)
policy(23-39)
⏰ 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 (22)
packages/db/drizzle/meta/_journal.json (1)
593-598: Entry addition looks good.No conflicts found with the existing set of indices (
idx). The new entry is consistent with the JSON structure and naming convention.packages/rule-engine/src/evaluate.ts (2)
9-14: Imports look appropriate.The new imports for approval records and
VersionApprovalRuleappear valid and are used properly below.
54-65: Confirm ignoringapprovalparameter is intentional.
versionUserApprovalRulealways setsminApprovals: 1and ignores properties from each rule object. If this is unintended, consider passingapproval.requiredApprovalsCount.packages/db/src/schema/rules/rule-relations.ts (4)
1-19: Imports and initialization look consistent.All new imports match usage, and the file sets a clear foundation for rule relations.
20-25: User relations appear correct.Mapping users to various approval record sets is coherent and properly declared.
35-51: User approval record relations are logical.Linking each record to a specific user, deployment version, and approval rule ensures consistency in referencing.
101-109: Deny window relation is cleanly defined.Associating each
policyRuleDenyWindowwith itspolicyfosters clarity in the existing schema structure.packages/db/drizzle/0084_abnormal_lady_ursula.sql (15)
1-5: Enum Type Creation with Exception HandlingThe creation of the
approval_statusenum is well-handled using a DO block along with exception handling for duplicate objects. This pattern ensures that the type is only created once even if the script is executed multiple times.
34-44: Creation ofpolicy_rule_role_approval_recordTableThis table is structurally similar to the user approval record table, providing consistency across approval record types.
Verify that application logic handles fields like"updated_at"correctly, as no default is provided for automatic update.
46-51: Creation ofpolicy_rule_any_approvalTableThe table definition correctly captures the generic approval scenario with required columns and a default required approval count of 1.
64-68: Foreign Key Constraint forpolicy_rule_user_approvalTable (policy_id)The DO block adds a foreign key constraint on
"policy_id"referencing thepolicytable withON DELETE cascade. This is appropriate if you want deletions in the policy table to automatically remove dependent approval records.
Ensure that this cascade behavior aligns with your domain requirements.
70-74: Foreign Key Constraint forpolicy_rule_user_approvalTable (user_id)This DO block adds the constraint on
"user_id"withON DELETE no action. The implementation is clear; just verify that the intended behavior (i.e., preventing deletion of referenced users or handling it via application logic) is correct according to your requirements.
76-80: Foreign Key Constraint forpolicy_rule_user_approval_recordTable (user_id)The foreign key on
"user_id"in the user approval record table is correctly added using a DO block with exception handling.
82-86: Foreign Key Constraint forpolicy_rule_user_approval_recordTable (rule_id)The constraint here links
"rule_id"to the primary key ofpolicy_rule_user_approval, withON DELETE cascade, ensuring referential integrity between records and their associated approval rules.
88-92: Foreign Key Constraint forpolicy_rule_role_approvalTable (policy_id)The addition of the foreign key constraint linking
"policy_id"to thepolicytable withON DELETE cascadeis appropriate.
94-98: Foreign Key Constraint forpolicy_rule_role_approvalTable (role_id)This DO block links
"role_id"to theroletable withON DELETE no action, which ensures that a role cannot be deleted if it is referenced. Confirm that the absence of cascade deletion matches your business logic.
100-104: Foreign Key Constraint forpolicy_rule_role_approval_recordTable (user_id)The foreign key on
"user_id"in the role approval record table is defined withON DELETE no action. The implementation is consistent with the other user-related constraints.
106-110: Foreign Key Constraint forpolicy_rule_role_approval_recordTable (rule_id)The constraint linking
"rule_id"topolicy_rule_role_approvalwithON DELETE cascadehelps ensure that when a role approval rule is removed, the associated records are automatically cleaned up.
112-116: Foreign Key Constraint forpolicy_rule_any_approvalTable (policy_id)Adding the constraint on
"policy_id"withON DELETE cascadefor the generic approval table is consistent with the handling of other approval types.
118-122: Foreign Key Constraint forpolicy_rule_any_approval_recordTable (user_id)This DO block correctly adds the foreign key constraint on
"user_id"in the generic approval record table.
124-124: Creation of Unique Index onpolicy_rule_any_approval_recordThe unique index on the combination of
"deployment_version_id"and"user_id"enforces the intended uniqueness for approval records.
Double-check that this uniqueness rule meets all business requirements for preventing duplicate approvals for a given deployment version by the same user.
125-126: Dropping Columns frompolicy_rule_deny_windowTableThe ALTER TABLE commands drop the
"name"and"description"columns frompolicy_rule_deny_windowif they exist. This cleanup appears intentional based on the new approval rule structure.
Please ensure that no legacy code depends on these columns or that appropriate data migrations have been performed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rule-engine/src/evaluate.ts (2)
128-128: Remove debugging console.log statementThis appears to be debug code that should be removed before merging to production.
- console.log({ resolvedReleases });
132-132: Remove debugging console.log statementThis appears to be debug code that should be removed before merging to production.
- console.log({ policy });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/db/src/schema/rules/rule-relations.ts(1 hunks)packages/rule-engine/src/evaluate.ts(3 hunks)packages/rule-engine/src/repositories/db-release-repository.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rule-engine/src/repositories/db-release-repository.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.
packages/rule-engine/src/evaluate.tspackages/db/src/schema/rules/rule-relations.ts
🧬 Code Definitions (2)
packages/rule-engine/src/evaluate.ts (2)
packages/rule-engine/src/types.ts (1)
Policy(66-72)packages/rule-engine/src/rules/version-approval-rule.ts (4)
VersionApprovalRule(32-72)getAnyApprovalRecords(74-88)getRoleApprovalRecords(90-104)getUserApprovalRecords(106-120)
packages/db/src/schema/rules/rule-relations.ts (5)
packages/db/src/schema/rules/approval-user.ts (2)
policyRuleUserApprovalRecord(27-37)policyRuleUserApproval(17-24)packages/db/src/schema/rules/approval-role.ts (2)
policyRuleRoleApprovalRecord(32-42)policyRuleRoleApproval(17-29)packages/db/src/schema/rules/approval-any.ts (2)
policyRuleAnyApprovalRecord(26-35)policyRuleAnyApproval(16-23)packages/db/src/schema/policy.ts (1)
policy(23-39)packages/db/src/schema/rules/deny-window.ts (1)
policyRuleDenyWindow(13-21)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (13)
packages/rule-engine/src/evaluate.ts (5)
54-65: Consider usingapproval.requiredApprovalsCountfor user approvalsUnlike the other approval rule functions,
versionUserApprovalRulehardcodesminApprovals: 1and doesn't use theapprovalparameter when mapping over the rules. For consistency, consider updating this to match the pattern inversionAnyApprovalRuleandversionRoleApprovalRule.const versionUserApprovalRule = ( approvalRules?: Policy["versionUserApprovals"] | null, ) => { if (approvalRules == null) return []; return approvalRules.map( - () => + (approval) => new VersionApprovalRule({ - minApprovals: 1, + minApprovals: approval.requiredApprovalsCount ?? 1, getApprovalRecords: getUserApprovalRecords, }), ); };
133-138: Nice implementation of the approval rules!The approach to combine the different types of approval rules into the rules array is well-structured. This provides a clean way to evaluate various policy constraints for deployments.
9-14: Well-organized import structureGood job organizing the imports for the approval-related functions. The destructured import style keeps the code clean and readable.
28-39: Clean implementation of version any approval ruleThe implementation correctly handles null checks and properly maps the approval rules to create
VersionApprovalRuleinstances with the appropriate parameters.
41-52: Clean implementation of version role approval ruleThe implementation follows the same pattern as the other approval rules, making the code consistent and maintainable.
packages/db/src/schema/rules/rule-relations.ts (8)
1-18: Imports are well-organized and comprehensive.The file properly imports all necessary components from Drizzle ORM and related schema files. The imports are grouped logically, with the ORM utility first, followed by entity imports, and finally the specific rule types needed for relation definitions.
20-25: User relations are correctly defined.The
userApprovalRelationsprovides a clear one-to-many relationship between users and the various approval record types. This allows for efficiently querying all approval records associated with a user.
27-37: Policy rule user approval relations are well structured.The relationship between
policyRuleUserApprovaland both its approval records and parent policy is correctly established. This provides a clear path to navigate from a user approval rule to its policy context.
39-55: User approval record relations properly establish all connections.The relationships define appropriate connections to the user, deployment version, and the originating rule. This enables efficient querying of all contextual information needed when processing user approval records.
57-85: Role approval relations follow consistent patterns.The role approval relations mirror the structure used for user approvals, maintaining consistency in the codebase. Both the rule-to-policy and record-to-entities relationships are properly defined.
87-111: Any approval relations are correctly implemented.The "any approval" structure follows the same pattern as the other approval types. Note that unlike the user and role approval records, the any approval record doesn't have a direct rule relationship in its relations definition, which correctly matches its table structure from the code snippets.
113-121: Deny window relations provide necessary policy context.The relationship between deny windows and policies is appropriately defined, enabling queries to fetch associated policy information when working with deny windows.
1-122: Overall schema relations are well-designed and consistent.The file creates a comprehensive relational structure for the approval system. The relations follow consistent patterns, properly establish bidirectional relationships where needed, and align with the table definitions provided in the related files. This approach will facilitate efficient querying and data navigation throughout the approval workflow.
Summary by CodeRabbit
New Features
Chores