clean up rule engine library #463
Conversation
WalkthroughThis pull request removes the legacy deployment evaluation logic by deleting the old evaluation file and refactoring many components. The public API now exports a new versioned rule engine. Updates span type renaming (switching from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant VRM as VersionReleaseManager
participant DB as Database
participant VRE as VersionRuleEngine
Client->>VRM: evaluate()
VRM->>DB: get context & latest release
DB-->>VRM: return context, candidate list
VRM->>VRM: getPolicy(forceRefresh)
VRM->>VRE: evaluate(context, candidates)
VRE->>VRE: apply filtering rules sequentially
VRE-->>VRM: return evaluation result (chosenCandidate, rejection reasons)
VRM-->>Client: evaluation result
sequenceDiagram
participant Client
participant VRM as VersionReleaseManager
participant DB as Database
Client->>VRM: upsertRelease(versionId)
VRM->>DB: findLatestRelease()
alt Version ID matches existing record
DB-->>VRM: existing release
VRM-->>Client: { created: false, release: existing release }
else New version detected
VRM->>DB: insert new release record
DB-->>VRM: new release record
VRM-->>Client: { created: true, release: new release }
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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 (4)
packages/rule-engine/src/version-rule-engine.ts (4)
1-2: Consider partial lodash imports.
If you are only using a few lodash functions (such asminByandmaxBy), you could import them individually to reduce bundle size, e.g.,import minBy from "lodash/minBy";.
10-16: Consider narrowing the types forconfigandmetadata.
UsingRecord<string, any>andRecord<string, string>works but can over-generalize. You might use more specific types or generics to better capture the data shape, which improves maintainability and type safety.
38-39: Consider simplifying the rule definition union.
Having both directRuleEngineFilter<Version>objects and functions returning filters can be flexible but increases complexity. You might unify this approach if the codebase consistently relies on one pattern.
161-165: Consider storingrequiresSequentialUpgradeas a boolean.
Filtering onv.metadata.requiresSequentialUpgrade === "true"suggests it’s stored as a string. A boolean field could be more direct and less error-prone.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/rule-engine/src/evaluate.ts(3 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/releases.ts(3 hunks)packages/rule-engine/src/repositories/db-release-repository.ts(5 hunks)packages/rule-engine/src/repositories/types.ts(2 hunks)packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts(1 hunks)packages/rule-engine/src/rules/__tests__/version-approval-rule.test.ts(1 hunks)packages/rule-engine/src/rules/deployment-deny-rule.ts(4 hunks)packages/rule-engine/src/rules/version-approval-rule.ts(6 hunks)packages/rule-engine/src/types.ts(2 hunks)packages/rule-engine/src/version-rule-engine.ts(7 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/rules/__tests__/version-approval-rule.test.tspackages/rule-engine/src/evaluate.tspackages/rule-engine/src/repositories/types.tspackages/rule-engine/src/releases.tspackages/rule-engine/src/repositories/db-release-repository.tspackages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.tspackages/rule-engine/src/rules/deployment-deny-rule.tspackages/rule-engine/src/types.tspackages/rule-engine/src/version-rule-engine.tspackages/rule-engine/src/rules/version-approval-rule.ts
🧬 Code Definitions (7)
packages/rule-engine/src/rules/__tests__/version-approval-rule.test.ts (2)
packages/rule-engine/src/releases.ts (1)
Releases(18-304)packages/rule-engine/src/types.ts (1)
RuleEngineContext(35-40)
packages/rule-engine/src/repositories/types.ts (1)
packages/rule-engine/src/types.ts (1)
RuleEngineContext(35-40)
packages/rule-engine/src/releases.ts (1)
packages/rule-engine/src/types.ts (2)
RuleEngineContext(35-40)ResolvedRelease(5-15)
packages/rule-engine/src/repositories/db-release-repository.ts (2)
packages/db/src/schema/release.ts (1)
releaseTarget(18-40)packages/rule-engine/src/types.ts (1)
RuleEngineContext(35-40)
packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
packages/rule-engine/src/types.ts (3)
RuleEngineFilter(55-61)RuleEngineContext(35-40)RuleEngineRuleResult(42-45)
packages/rule-engine/src/types.ts (2)
packages/db/src/schema/resource.ts (1)
Resource(105-105)packages/db/src/schema/policy.ts (2)
policy(23-39)Policy(104-104)
packages/rule-engine/src/rules/version-approval-rule.ts (2)
packages/rule-engine/src/types.ts (3)
RuleEngineContext(35-40)RuleEngineFilter(55-61)RuleEngineRuleResult(42-45)packages/rule-engine/src/version-rule-engine.ts (1)
Version(10-16)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (48)
packages/rule-engine/src/index.ts (1)
3-3: Correctly updated export to use versioned rule engineThe export statement has been updated to use the new versioned rule engine implementation, which aligns with the refactoring effort to adopt a more generic, version-specific approach.
packages/rule-engine/src/rules/__tests__/version-approval-rule.test.ts (1)
3-3: Proper type transition to RuleEngineContextThe import and type declaration have been correctly updated to use
RuleEngineContextinstead ofDeploymentResourceContext, maintaining consistency with the broader rule engine refactoring.Also applies to: 10-10
packages/rule-engine/src/repositories/types.ts (2)
3-3: Updated import statement to use RuleEngineContextThe import statement has been correctly updated to import
RuleEngineContextinstead ofDeploymentResourceContext, maintaining consistency with the rule engine refactoring.
73-73: Updated getCtx method return typeThe
getCtxmethod's return type has been properly updated to useRuleEngineContextinstead ofDeploymentResourceContext, which aligns with the changes in other files and maintains type consistency throughout the codebase.packages/rule-engine/src/releases.ts (2)
1-1: Updated import statement to use RuleEngineContextThe import statement has been correctly updated to import
RuleEngineContextinstead ofDeploymentResourceContext, maintaining consistency with the broader refactoring effort.
103-103: Updated method parameter types to RuleEngineContextThe parameter types for
getDesiredandgetEffectiveTargetmethods have been properly updated to useRuleEngineContextinstead ofDeploymentResourceContext. This ensures type consistency across the codebase while maintaining the same functionality.Also applies to: 120-120
packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts (2)
5-5: Adoption of newRuleEngineContextimport looks good
This change aligns with the broader shift fromDeploymentResourceContexttoRuleEngineContext.
11-11: Ensure full migration toRuleEngineContext
Double-check for any lingering references to the oldDeploymentResourceContexttype across test utilities or fixtures to avoid inconsistent context usage.packages/rule-engine/src/evaluate.ts (6)
4-6: Updated imports align with new engine types
The addition ofRuleEngineFilter,RuleEngineSelectionResult, andVersionensures clarity and consistency with the revised architecture.
14-14: Switch toVersionRuleEngine
Replacing the straightforwardRuleEnginewith the version-specificVersionRuleEnginereflects the new domain-specific approach. No concerns here.
21-25: ParameterizingDeploymentDenyRulewith<Version>
Using the generic form forDeploymentDenyRuleand providinggetCandidateIdclarifies type handling while ensuring each version can be correctly identified.
129-130: Consistent policy retrieval
Retrieving the policy viarepository.getPolicy()maintains a straightforward approach; no issues found.
136-136: Instantiation ofVersionRuleEngine
Initializingnew VersionRuleEngine(rules)is correct in context of typed filters.
138-141: Evaluating context and versions
Passingctxand mapped versions toengine.evaluateis consistent with the updated filtering methodology. Looks solid.packages/rule-engine/src/repositories/db-release-repository.ts (5)
8-8: Renamed import toRuleEngineContext
This matches the broader refactor away fromDeploymentResourceContext.
27-33:DatabaseReleaseTargetinterface
This new interface name clarifies that the release target is database-centric, aligning with the naming consistency.
46-53: Factory methodcreateforDatabaseReleaseRepository
The async creation pattern matches the overall approach. No immediate issues.
60-62: Constructor now leveragesDatabaseReleaseTarget
Integrates the renamed interface consistently. No concerns from a correctness standpoint.
237-246:getCtxmethod returningRuleEngineContext | undefined
Ensure the returned object shape fully matches theRuleEngineContextdefinition (deployment, environment, resource, etc.). The usage of.findFirst()is straightforward, but verify each nested relation is present in the final result.packages/rule-engine/src/rules/deployment-deny-rule.ts (7)
11-13: Replaced old rule engine imports
Upgrading toRuleEngineContext,RuleEngineFilter, andRuleEngineRuleResultis consistent with the new architecture.
45-55: GenericDeploymentDenyRuleOptions
The addedgetCandidateIdfunction and generic<T>parameter support more flexible candidate types. Well-structured approach.
57-64:DeploymentDenyRulenow implementsRuleEngineFilter<T>
Great alignment with the typed filter approach. The class signature is correct, specifyingnameand a compliantfiltermethod.
67-67: Constructor enhancements
StoringgetCandidateIdensures each candidate can be mapped to a reason. The usage of time-based logic remains consistent.Also applies to: 71-71, 73-73, 74-74, 77-77, 78-78
100-102:filtermethod signature
Accepting generic candidates and returning aRuleEngineRuleResult<T>aligns with the new filter pattern.
109-114: Building rejection reasons for each candidate
Mapping candidates to thegetCandidateIdensures detailed, candidate-specific denial reasons.
118-118: Returning all candidates if not denied
Clear logic ensures a straightforward pass-through when no restriction is active.packages/rule-engine/src/version-rule-engine.ts (7)
4-7: Updated type imports look consistent.
No concerns with the new imports from./types.js; this aligns well with the new architecture.
28-28: Class rename is aligned with the new approach.
ImplementingRuleEngine<Version>channels the new design correctly.
71-73: Method signature aligns with the new generics.
AcceptingRuleEngineContextandVersion[]fits well. No concerns here.
81-81: Use ofallowedCandidatesis properly integrated.
This ensures the new filter outputs are uniformly handled. Merging with existing rejection reasons in subsequent lines also looks good.Also applies to: 83-86
102-103: Selecting the final release is cleanly refactored.
Switching tochosenCandidateand returningnullif none are found is consistent with the new result type.Also applies to: 106-106, 110-110
137-140: Signature updated forselectFinalRelease.
PassingRuleEngineContextandVersion[]reflects the new architecture.
147-149: Use of_.minBy/_.maxByis appropriate.
This straightforward selection logic is easy to follow.packages/rule-engine/src/types.ts (5)
35-40: Context type change is consistent with the refactor.
RuleEngineContextbetter communicates the new, generic rule engine scope.
42-45: Renamed fields align with candidate-based filtering.
Switching fromallowedReleasestoallowedCandidatesis clear and cohesive with the rest of the types.
47-50:RuleEngineSelectionResultproperly referenceschosenCandidate.
This naming works consistently with the new design.
55-60:RuleEngineFilterinterface is clear.
Accepting a context and a candidate array, returning aRuleEngineRuleResult<T>matches the updated architecture.
82-87:RuleEngineinterface is straightforward.
Yourevaluatemethod signature is consistent across the codebase, ensuring easy extensibility.packages/rule-engine/src/rules/version-approval-rule.ts (10)
8-12: Import changes align with the new rule engine types.
Shifting toRuleEngineContextandRuleEngineRuleResultis consistent with the reorganized architecture.
22-22:GetApprovalRecordsFuncnow takesRuleEngineContext.
Properly updated to match the rest of the approach.
32-32: ImplementsRuleEngineFilter<Version>suitably.
Renaming toVersionApprovalRuleclarifies its intent and usage.
38-40: New signature forfiltermethod.
AccommodatesRuleEngineContextandVersion[]. Looks good.
42-43: Use of lodash chain for IDs is efficient.
Extracting version IDs into a unique list ensures minimal duplication.
51-52: Filtering logic is straightforward.
Rejecting based on any rejections present and requiring a minimum number of approvals is clear.
68-68: Properly returningallowedCandidateswith rejections.
This matches the newRuleEngineRuleResult<Version>structure.
73-73: Context parameter naming underscores we are not using it here.
Using_is a valid approach for unused parameters.
89-89: Same pattern for role approval records retrieval.
The new context parameter is consistent.
105-105: Likewise for user approval records.
No concerns found here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/rule-engine/src/version-rule-engine.ts (3)
10-16: Optional: Add documentation for each field of theVersiontype.To improve clarity and maintainability, consider adding short doc comments describing each property (e.g.,
id,tag,config,metadata,createdAt) so future maintainers understand their purpose at a glance.
27-41: Consider handling rule execution errors more gracefully.If a rule (or its promise) throws an error, the engine will fail entirely. Depending on your requirements, you might want to catch exceptions and annotate rejection reasons instead of abruptly crashing the evaluation process.
152-157: Consider validatingmetadata.requiresSequentialUpgradeas a boolean.Currently, this check strictly compares against the string
"true". If metadata ever stores this flag in a different form (e.g., uppercase or real boolean), the filter would fail. Structuring it as a typed boolean can make the logic more robust.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rule-engine/src/version-rule-engine.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/version-rule-engine.ts
🧬 Code Definitions (1)
packages/rule-engine/src/version-rule-engine.ts (2)
packages/rule-engine/src/types.ts (4)
RuleEngine(82-87)RuleEngineFilter(55-61)RuleEngineContext(35-40)RuleEngineSelectionResult(47-50)packages/job-dispatch/src/release-job-trigger.ts (1)
versions(73-76)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
packages/rule-engine/src/version-rule-engine.ts (3)
1-2: No issues with Lodash import usage.The usage of Lodash for
minByandmaxByis justified and clear.
67-110: Evaluation logic looks correct and straightforward.Sequentially applying rules, accumulating rejection reasons, and short-circuiting when all candidates are disqualified aligns well with typical rule-based filtering workflows.
128-141: Robust version selection strategy.Choosing the oldest version for sequential upgrades and falling back to the newest version by creation date is consistent with the documented intent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/db/src/schema/release.ts (6)
42-55: Consider indexing for performance.The new
version_releasetable looks good. However, if you often query by bothreleaseTargetIdandversionId, consider adding an index on these columns (together or separately) for improved query performance.export const versionRelease = pgTable("version_release", { id: uuid("id").primaryKey().defaultRandom(), releaseTargetId: uuid("release_target_id") .notNull() .references(() => releaseTarget.id, { onDelete: "cascade" }), versionId: uuid("version_id") .notNull() .references(() => deploymentVersion.id, { onDelete: "cascade" }), createdAt: timestamp("created_at", { withTimezone: true }) .notNull() .defaultNow(), }) +(t) => ({ + idxVersionRelease: uniqueIndex("version_release_target_version_idx") + .on(t.releaseTargetId, t.versionId), +})
57-65: Potential need for uniqueness constraints.
variable_releasereferences a singlereleaseTargetId. If the functional requirements demand only onevariable_releaseper target, add a unique index onreleaseTargetIdto prevent duplicates. Otherwise, this is fine as is.
67-78: Secure sensitive values.The
variable_release_valuetable flags sensitive data but still stores it as plain JSON. Consider storing sensitive values in an encrypted form or in an external secrets manager to enhance security.
81-95: Optional indexing for frequent lookups.Referencing
versionReleaseIdandvariableReleaseIdis correct. If you anticipate frequent lookups or joins on these columns, adding indexes can improve performance. Otherwise, this looks good.
123-136: Rename relation for clarity.The
release: many(release)relationship can confuse readers since it suggests a single release rather than multiple. Consider naming itreleasesto better reflect the nature of the relationship.
138-149: Likewise rename for plural consistency.Currently,
release: many(release)might cause confusion. Consider renaming it toreleasesfor consistency with its one-to-many cardinality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/event-worker/src/workers/evaluate-release-target.ts(2 hunks)apps/event-worker/src/workers/job-dispatch/github.ts(1 hunks)packages/db/src/schema/release.ts(4 hunks)packages/rule-engine/src/repositories/db-release-repository.ts(6 hunks)packages/rule-engine/src/repositories/get-releases.ts(3 hunks)packages/rule-engine/src/repositories/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous 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.
apps/event-worker/src/workers/job-dispatch/github.tspackages/rule-engine/src/repositories/get-releases.tspackages/rule-engine/src/repositories/types.tsapps/event-worker/src/workers/evaluate-release-target.tspackages/db/src/schema/release.ts
🧬 Code Definitions (4)
packages/rule-engine/src/repositories/get-releases.ts (2)
packages/db/src/schema/job.ts (1)
JobStatus(136-136)packages/db/src/schema/release.ts (1)
releaseTarget(18-40)
packages/rule-engine/src/repositories/types.ts (1)
packages/rule-engine/src/types.ts (1)
RuleEngineContext(35-40)
apps/event-worker/src/workers/evaluate-release-target.ts (2)
packages/db/src/schema/release.ts (1)
release(81-95)packages/rule-engine/src/evaluate.ts (1)
evaluateRepository(103-142)
packages/db/src/schema/release.ts (2)
packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(109-134)packages/db/src/schema/job.ts (1)
job(74-106)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (18)
packages/rule-engine/src/repositories/types.ts (3)
3-3: Import updated to use RuleEngineContextThe import statement has been correctly updated to use
RuleEngineContextinstead ofDeploymentResourceContext, which aligns with the broader refactoring to make the rule engine more generic and version-specific.
9-9: Base Release type updated to use versionReleaseThe base Release type has been appropriately updated to use
schema.versionRelease.$inferSelectinstead of the previousschema.release.$inferSelect, maintaining type safety while reflecting the schema changes.
73-73: Method signature updated for getCtx()The return type of
getCtx()has been properly updated fromPromise<DeploymentResourceContext | undefined>toPromise<RuleEngineContext | undefined>, which is consistent with the import changes and aligns with the more generic rule engine approach.apps/event-worker/src/workers/job-dispatch/github.ts (2)
78-80: Join statements updated for versionReleaseThe inner join has been correctly updated to use
SCHEMA.versionReleaseinstead ofSCHEMA.release, maintaining the relationship withSCHEMA.releaseJobthrough the appropriate ID field.
83-83: Join condition updated for versionReleaseThe join condition has been properly updated to reference
SCHEMA.versionRelease.releaseTargetIdinstead ofSCHEMA.release.releaseTargetId, maintaining the relationship withSCHEMA.releaseTarget.id.apps/event-worker/src/workers/evaluate-release-target.ts (3)
22-23: createJobForRelease updated to use versionReleaseThe query in
createJobForReleasehas been correctly updated to useversionReleasetable instead ofrelease, reflecting the schema changes.
94-95: Variable renamed from chosenRelease to chosenCandidateThe variable destructured from
evaluateRepositoryhas been renamed fromchosenReleasetochosenCandidate, reflecting changes in the rule engine which now returns a more genericchosenCandidateinstead of a specificchosenRelease.
101-101: Function argument updated to use chosenCandidateThe argument passed to
createJobForReleasehas been updated to usechosenCandidate.idinstead ofchosenRelease.id, consistent with the variable renaming.packages/rule-engine/src/repositories/get-releases.ts (7)
55-56: Inner join updated for schema.release referenceThe database query has been updated to maintain the correct relationship between tables, with the inner join now properly referencing
schema.releaseand using the appropriate join condition withschema.release.releaseId.Also applies to: 59-59
65-65: Order by clause updated to use versionReleaseThe order by clause has been updated to sort by
schema.versionRelease.createdAtinstead ofschema.release.createdAt, consistent with the schema changes.
87-90: findMany query updated to use versionReleaseThe query method has been updated from
db.query.release.findManytodb.query.versionRelease.findManywith appropriate where conditions targetingversionRelease.releaseTargetIdandversionRelease.createdAt, reflecting the schema changes.Also applies to: 95-96
98-102: Date comparison updated for desiredReleaseThe date comparison logic for
desiredReleasehas been updated to correctly useschema.versionRelease.createdAt, maintaining the same functionality while working with the new schema.
108-108: Result ordering updated to use versionReleaseThe order by clause in the result has been updated to use
schema.versionRelease.createdAt, ensuring consistent ordering with the updated schema.
128-131: findFirst query updated to use versionReleaseThe query for finding the latest policy-matching release has been updated to use
versionReleasewith appropriate conditions and ordering, maintaining functionality with the new schema.
136-139: findFirst query with policy filter updatedThe second query that filters by policy has been updated to use
versionReleasewith appropriate conditions and ordering, correctly implementing the filtering logic with the new schema.Also applies to: 145-145
packages/db/src/schema/release.ts (3)
150-158: Looks clean and consistent.No issues spotted with the
variableReleaseValuerelations. The referencing logic is straightforward, and the naming is clear.
160-173: Relationships are properly defined.Your
releaseRelationsconsistently referenceversionRelease,variableRelease, andjob. This appears well-organized and clear. Nice work.
97-97:✅ Verification successful
Verify cyclical references.
releaseTargetreferencesdesiredRelease, while the newly addedversionReleasesandvariableReleasespoint back toreleaseTarget. Ensure this circular dependency does not cause migration or relationship resolution issues.Also applies to: 118-122
🏁 Script executed:
#!/bin/bash # Check for potential circular references in table definitions. ast-grep --pattern $'pgTable($_, { $$$ }, (t) => { $$$ })' --jsonLength of output: 69
Circular Dependency Verification Completed
We ran an AST search for table definitions using the provided pattern and found no evidence of problematic circular dependencies in the schema. Based on the verification, the relationships between
releaseTarget,desiredRelease,versionReleases, andvariableReleasesappear to be properly defined and should not introduce migration or relationship resolution issues.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/rule-engine/src/version-manager/version-manager.ts (1)
39-41: Implement thefindLatestReleasemethod.The
findLatestReleasemethod is currently a placeholder. Consider implementing it to find the latest version release for the given release target.Would you like me to provide an implementation suggestion for this method based on the database schema?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rule-engine/src/version-manager/version-manager.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/version-manager/version-manager.ts
🧬 Code Definitions (1)
packages/rule-engine/src/version-manager/version-manager.ts (2)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/release.ts (2)
releaseTarget(18-40)release(81-95)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (1)
packages/rule-engine/src/version-manager/version-manager.ts (1)
8-14: Verify theworkspaceIdproperty inReleaseTargetinterface.The
ReleaseTargetinterface includes aworkspaceIdproperty that doesn't appear in therelease_targettable schema from the provided code snippets. Verify if this property is necessary or if it's retrieved from another related table.
| export class VersionReleaseManager { | ||
| private constructor( | ||
| private readonly db: Tx = dbClient, | ||
| private readonly releaseTarget: ReleaseTarget, | ||
| ) {} |
There was a problem hiding this comment.
Class needs a static factory method for instantiation.
The VersionReleaseManager class has a private constructor, making it impossible to create instances directly. However, there's no public static factory method provided to instantiate the class. You should add a static factory method to allow consumers to create instances.
export class VersionReleaseManager {
private constructor(
private readonly db: Tx = dbClient,
private readonly releaseTarget: ReleaseTarget,
) {}
+ static forReleaseTarget(releaseTarget: ReleaseTarget, db: Tx = dbClient) {
+ return new VersionReleaseManager(db, releaseTarget);
+ }📝 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.
| export class VersionReleaseManager { | |
| private constructor( | |
| private readonly db: Tx = dbClient, | |
| private readonly releaseTarget: ReleaseTarget, | |
| ) {} | |
| export class VersionReleaseManager { | |
| private constructor( | |
| private readonly db: Tx = dbClient, | |
| private readonly releaseTarget: ReleaseTarget, | |
| ) {} | |
| static forReleaseTarget(releaseTarget: ReleaseTarget, db: Tx = dbClient) { | |
| return new VersionReleaseManager(db, releaseTarget); | |
| } | |
| } |
| async canadiates() { | ||
| // ... | ||
| } |
There was a problem hiding this comment.
Fix typo in method name and implement the method.
There's a typo in the method name - "canadiates" should be "candidates". Also, this method is currently a placeholder and needs implementation.
- async canadiates() {
+ async candidates() {
// ...
}Committable suggestion skipped: line range outside the PR's diff.
| async upsertRelease(versionId: string) { | ||
| const release = await this.db.query.versionRelease.findFirst({ | ||
| where: and( | ||
| eq(schema.versionRelease.releaseTargetId, this.releaseTarget.id), | ||
| eq(schema.versionRelease.versionId, versionId), | ||
| ), | ||
| orderBy: desc(schema.versionRelease.createdAt), | ||
| }); | ||
|
|
||
| if (release?.versionId === versionId) return release; | ||
|
|
||
| return this.db | ||
| .insert(schema.versionRelease) | ||
| .values({ releaseTargetId: this.releaseTarget.id, versionId }) | ||
| .returning(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for database operations.
The upsertRelease method returns the database insert promise directly without awaiting it. This could cause issues if the caller expects a resolved value rather than a promise. Also, there's no error handling for database operations.
async upsertRelease(versionId: string) {
const release = await this.db.query.versionRelease.findFirst({
where: and(
eq(schema.versionRelease.releaseTargetId, this.releaseTarget.id),
eq(schema.versionRelease.versionId, versionId),
),
orderBy: desc(schema.versionRelease.createdAt),
});
if (release?.versionId === versionId) return release;
- return this.db
+ const [newRelease] = await this.db
.insert(schema.versionRelease)
.values({ releaseTargetId: this.releaseTarget.id, versionId })
.returning();
+ return newRelease;
}📝 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.
| async upsertRelease(versionId: string) { | |
| const release = await this.db.query.versionRelease.findFirst({ | |
| where: and( | |
| eq(schema.versionRelease.releaseTargetId, this.releaseTarget.id), | |
| eq(schema.versionRelease.versionId, versionId), | |
| ), | |
| orderBy: desc(schema.versionRelease.createdAt), | |
| }); | |
| if (release?.versionId === versionId) return release; | |
| return this.db | |
| .insert(schema.versionRelease) | |
| .values({ releaseTargetId: this.releaseTarget.id, versionId }) | |
| .returning(); | |
| } | |
| async upsertRelease(versionId: string) { | |
| const release = await this.db.query.versionRelease.findFirst({ | |
| where: and( | |
| eq(schema.versionRelease.releaseTargetId, this.releaseTarget.id), | |
| eq(schema.versionRelease.versionId, versionId), | |
| ), | |
| orderBy: desc(schema.versionRelease.createdAt), | |
| }); | |
| if (release?.versionId === versionId) return release; | |
| const [newRelease] = await this.db | |
| .insert(schema.versionRelease) | |
| .values({ releaseTargetId: this.releaseTarget.id, versionId }) | |
| .returning(); | |
| return newRelease; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/rule-engine/src/manager/version-manager.ts (1)
16-24: Consider adding input validation for versionIdThe
upsertReleasemethod doesn't validate theversionIdparameter. Consider adding validation to ensure it's not empty or invalid before proceeding with database operations.async upsertRelease(versionId: string) { + if (!versionId) { + throw new Error("Version ID cannot be empty"); + } + const release = await this.findLatestRelease(); if (release?.versionId === versionId) return release; return this.db .insert(schema.versionRelease) .values({ releaseTargetId: this.releaseTarget.id, versionId }) .returning() .then(takeFirst); }packages/rule-engine/src/manager/variable-manager.ts (1)
20-32: Add comments to explain variable transformationsThe lodash transformations for
oldVarsandnewVarscould benefit from a comment explaining the purpose of converting the variable arrays to key-value objects for comparison.+ // Convert variable arrays to key-value objects for efficient comparison const oldVars = _(latestRelease?.values ?? []) .map((v) => [v.key, v.value]) .fromPairs() .value(); const newVars = _(variables) .compact() .map((v) => [v.key, v.value]) .fromPairs() .value(); const isSame = _.isEqual(oldVars, newVars); if (isSame) return { created: false, latestRelease };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rule-engine/src/manager/types.ts(1 hunks)packages/rule-engine/src/manager/variable-manager.ts(1 hunks)packages/rule-engine/src/manager/version-manager.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/manager/types.tspackages/rule-engine/src/manager/variable-manager.tspackages/rule-engine/src/manager/version-manager.ts
🧬 Code Definitions (2)
packages/rule-engine/src/manager/variable-manager.ts (4)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (2)
releaseTarget(18-40)release(81-95)packages/rule-engine/src/manager/types.ts (1)
ReleaseTarget(1-7)packages/rule-engine/src/repositories/variables/types.ts (1)
MaybeVariable(9-9)
packages/rule-engine/src/manager/version-manager.ts (3)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (2)
releaseTarget(18-40)release(81-95)packages/rule-engine/src/manager/types.ts (1)
ReleaseTarget(1-7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (1)
packages/rule-engine/src/manager/types.ts (1)
1-7: Well-defined interface for release targetsThe
ReleaseTargetinterface is clearly defined with appropriate properties for identifying a release target within the system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/rule-engine/src/manager/types.ts (2)
1-7: Consider adding doc comments to clarifyReleaseTarget.
Providing short JSDoc descriptions for each property would improve readability and help new contributors understand its purpose.
9-11: Use more explicit parameter types forupsertRelease.
Currently,(...args: any[])weakens type safety. Consider adopting a more specific parameter signature to ensure correctness and maintainability of implementations.packages/rule-engine/src/manager/variable-manager.ts (3)
11-15: Revisit private constructor usage.
A private constructor can make external instantiation impossible. If that's intended, consider a factory method or comment explaining how instances should be created.
35-53: Consider partial upsert strategies.
Creating a new release whenever variables differ ensures version tracking, but you might also consider partial updates if your use case allows it. This can avoid storing large duplicate patterns.
55-61: Return type consistency.
findLatestReleasemay returnnullif no release is found. Consider clarifying the expected return type or explicitly handling the absence of a release.packages/rule-engine/src/manager/version-manager.ts (2)
10-14: Clarify private constructor intent.
Similar toVariableReleaseManager, if this class must be directly instantiated elsewhere, consider making the constructor public or offering a factory method.
30-35: Handlenullcase infindLatestRelease.
Add a consistent approach to manage the scenario when no recent release exists, e.g., a default object or explicit null checks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rule-engine/src/manager/types.ts(1 hunks)packages/rule-engine/src/manager/variable-manager.ts(1 hunks)packages/rule-engine/src/manager/version-manager.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/manager/version-manager.tspackages/rule-engine/src/manager/types.tspackages/rule-engine/src/manager/variable-manager.ts
🧬 Code Definitions (1)
packages/rule-engine/src/manager/version-manager.ts (3)
packages/rule-engine/src/manager/types.ts (2)
ReleaseManager(9-11)ReleaseTarget(1-7)packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (2)
releaseTarget(18-40)release(81-95)
🔇 Additional comments (2)
packages/rule-engine/src/manager/variable-manager.ts (1)
17-33:❓ Verification inconclusive
Verify concurrency handling for
upsertRelease.
If multiple calls occur simultaneously, overwriting changes is possible. Check if concurrency is mitigated through higher-level locking or single-thread usage.
🏁 Script executed:
#!/bin/bash # Verify all callers of the upsertRelease method to see if concurrency is addressed rg -A 5 'upsertRelease'Length of output: 4843
Action Required: Review Concurrency Control in upsertRelease
The current scan of caller usages did not reveal any explicit concurrency safeguards (such as locking or atomic operations) within the
upsertReleaseimplementations (in both variable-manager and related modules). Given that multiple concurrent invocations could potentially lead to an overwrite of changes, please verify that this risk is mitigated at a higher level—whether that’s through a locking mechanism in the repository/version manager or by relying on the single-threaded guarantees of the runtime.packages/rule-engine/src/manager/version-manager.ts (1)
16-28:❓ Verification inconclusive
Confirm atomic behavior for
upsertRelease.
Two concurrent calls with differentversionIds could generate multiple nearly identical releases. Validate if this is acceptable or if additional constraints are needed.
🏁 Script executed:
#!/bin/bash # Search for concurrency handling or usage patterns for upsertRelease rg -A 5 'upsertRelease'Length of output: 4843
Concurrency Consideration: Verify Atomic Behavior in upsertRelease
The current implementation of
upsertReleaseuses a check-then-act pattern without explicit concurrency controls. This means that if two concurrent calls with differentversionIds occur, both might pass the check (findLatestRelease) and proceed to insert new releases, potentially creating multiple nearly identical releases. Please confirm whether this behavior is acceptable or if additional safeguards (e.g., database unique constraints, full transactional handling, or distributed locking) should be added to ensure atomicity.
- Location:
packages/rule-engine/src/manager/version-manager.ts(Lines 16-28)- Action: Confirm intended concurrent behavior. If atomicity is required, consider refactoring this logic to enforce a single release update per target.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rule-engine/src/manager/version-manager.ts (1)
17-20: Constructor privacy noted.A previous commit comment already addressed the need for a factory method or public constructor. Since this is intentionally private, no further changes appear necessary here.
🧹 Nitpick comments (1)
packages/rule-engine/src/manager/version-rule-engine.ts (1)
148-149: Consider using a boolean rather than a string inmetadata.requiresSequentialUpgrade.Relying on
v.metadata.requiresSequentialUpgrade === "true"can be error-prone if there is a mismatch in the string. Having a typed property or boolean field would be more robust.- (v) => v.metadata.requiresSequentialUpgrade === "true" + (v) => v.metadata.requiresSequentialUpgrade === true
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/rule-engine/src/evaluate.ts(0 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/manager/variable-manager.ts(1 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(1 hunks)packages/rule-engine/src/manager/version-manager.ts(1 hunks)packages/rule-engine/src/manager/version-rule-engine.ts(1 hunks)packages/rule-engine/src/rules/version-approval-rule.ts(6 hunks)
💤 Files with no reviewable changes (1)
- packages/rule-engine/src/evaluate.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rule-engine/src/index.ts
- packages/rule-engine/src/manager/variable-manager.ts
- packages/rule-engine/src/rules/version-approval-rule.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/manager/version-manager-rules.tspackages/rule-engine/src/manager/version-rule-engine.tspackages/rule-engine/src/manager/version-manager.ts
🧬 Code Definitions (2)
packages/rule-engine/src/manager/version-manager-rules.ts (4)
packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
DeploymentDenyRule(57-176)packages/rule-engine/src/manager/version-rule-engine.ts (1)
Version(10-16)packages/rule-engine/src/rules/version-approval-rule.ts (4)
VersionApprovalRule(32-70)getAnyApprovalRecords(72-86)getRoleApprovalRecords(88-102)getUserApprovalRecords(104-118)packages/rule-engine/src/types.ts (1)
RuleEngineFilter(55-61)
packages/rule-engine/src/manager/version-manager.ts (7)
packages/rule-engine/src/manager/types.ts (2)
ReleaseManager(9-11)ReleaseTarget(1-7)packages/db/src/common.ts (1)
Tx(22-22)packages/rule-engine/src/db/get-applicable-policies.ts (1)
getApplicablePolicies(108-139)packages/rule-engine/src/utils/merge-policies.ts (1)
mergePolicies(29-47)packages/rule-engine/src/types.ts (1)
RuleEngineContext(35-40)packages/rule-engine/src/manager/version-manager-rules.ts (1)
getRules(63-72)packages/rule-engine/src/manager/version-rule-engine.ts (1)
VersionRuleEngine(27-151)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
packages/rule-engine/src/manager/version-manager-rules.ts (2)
50-61: Check usage of approval data inversionUserApprovalRule.Currently, the code ignores the mapped item within the loop (lines 55-56) and always sets
minApprovals: 1regardless of theapprovalRulescontent. This may be intentional, but please confirm this logic or consider allowing each user approval rule to configureminApprovalsmore dynamically.
63-72: Overall approach looks good.The flexible composition of deny windows and approval rules is clean and matches the policy-based design. The code is easy to read, modular, and each function serves a single purpose.
packages/rule-engine/src/manager/version-rule-engine.ts (1)
27-103: Kudos on a solid rule evaluation flow.Sequential application of rules, accumulating rejection reasons, and early exit when no candidates remain is a clear, reliable pattern. The approach cleanly handles both sync and async filter steps.
packages/rule-engine/src/manager/version-manager.ts (1)
77-78: Verify passing an empty array into the rule engine.
engine.evaluate(ctx, [])will produce a null candidate because there are no versions to filter or select. If this is not intentional, consider fetching actual version candidates and passing them in.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/rule-engine/src/manager/variable-manager.ts (2)
13-14: UnusedcachedPolicyproperty.
Theprivate cachedPolicyfield is declared but never used in this class. Consider removing it or leveraging it if you plan to cache or merge policies here.
12-12: Explicitly specify the generic parameter forReleaseManager.
By default,ReleaseManagerexpects a release type of{ id: string; createdAt: Date }. Ifschema.variableReleasedoes not strictly conform to this shape, you may want to specify a more precise type parameter to ensure type safety.packages/rule-engine/src/manager/version-manager.ts (1)
56-77: Validate environment references inevaluate.
You correctly handle a missing release target by throwing an error. Consider adding additional checks (e.g., verifying related environment/resource references are not stale) to ensure consistent context for rule evaluation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rule-engine/src/manager/variable-manager.ts(1 hunks)packages/rule-engine/src/manager/version-manager.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/manager/variable-manager.tspackages/rule-engine/src/manager/version-manager.ts
🧬 Code Definitions (1)
packages/rule-engine/src/manager/variable-manager.ts (4)
packages/rule-engine/src/manager/types.ts (2)
ReleaseManager(9-11)ReleaseTarget(1-7)packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (2)
releaseTarget(18-40)release(81-95)packages/rule-engine/src/repositories/variables/types.ts (1)
MaybeVariable(9-9)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
packages/rule-engine/src/manager/variable-manager.ts (2)
28-35: Potential concurrency issue on repeated writes.
If multiple calls toupsertReleaseoccur in parallel with the same new variable set, a short timing gap might allow more than one new record to be created. Consider adding a more robust concurrency check, or unique constraints on the underlying table, if duplicates would cause problems.Would you like to verify concurrency handling or add a uniqueness constraint to ensure only one release is created for the same variable set?
38-57: Transaction usage is appropriate, but watch for partial inserts.
Wrapping inserts in a transaction is a good approach. If an error occurs when inserting variable values, the partial release record is rolled back correctly. Ensure error handling is tested thoroughly (e.g., ifvariableReleaseValueinsertion fails).packages/rule-engine/src/manager/version-manager.ts (1)
43-54: Handle null policy scenario.
ThegetPolicymethod can returnnullif no policy is found. Ensure the call sites (such asevaluate) are prepared to handle anullpolicy object and avoid runtime errors down the line.Would you like a script to check if
getRules(policy)orVersionRuleEnginecan handle anullpolicy?
| async upsertRelease(versionId: string) { | ||
| const latestRelease = await this.findLatestRelease(); | ||
| if (latestRelease?.versionId === versionId) | ||
| return { created: false, release: latestRelease }; | ||
|
|
||
| const release = await this.db | ||
| .insert(schema.versionRelease) | ||
| .values({ releaseTargetId: this.releaseTarget.id, versionId }) | ||
| .returning() | ||
| .then(takeFirst); | ||
|
|
||
| return { created: true, release }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider concurrency guard for upsertRelease.
No transaction is used here. If multiple requests try to create a release with the same versionId concurrently, two releases may be created. If uniqueness is required, consider wrapping these inserts in a transaction or defining a unique constraint to avoid duplicates.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/db/src/schema/release.ts (1)
33-35: 🛠️ Refactor suggestionType safety concern with reference casting
The
desiredReleaseIdfield references therelease.idwith a type cast toany. This bypasses TypeScript's type checking and could lead to runtime errors. Consider using a proper type reference to ensure type safety.desiredReleaseId: uuid("desired_release_id") - .references((): any => release.id, { onDelete: "set null" }) + .references(() => release.id, { onDelete: "set null" }) .default(sql`NULL`),
♻️ Duplicate comments (1)
packages/rule-engine/src/manager/version-manager.ts (1)
22-34: 🛠️ Refactor suggestionConsider adding concurrency protection for upsertRelease
No transaction is used here. If multiple requests try to create a release with the same
versionIdconcurrently, two releases may be created. Consider wrapping these operations in a transaction or adding a unique constraint on(releaseTargetId, versionId)to prevent duplicates.async upsertRelease(versionId: string) { - const latestRelease = await this.findLatestRelease(); - if (latestRelease?.versionId === versionId) - return { created: false, release: latestRelease }; - - const release = await this.db - .insert(schema.versionRelease) - .values({ releaseTargetId: this.releaseTarget.id, versionId }) - .returning() - .then(takeFirst); - - return { created: true, release }; + return this.db.transaction(async (tx) => { + const latestRelease = await tx.query.versionRelease.findFirst({ + where: eq(schema.versionRelease.releaseTargetId, this.releaseTarget.id), + orderBy: desc(schema.versionRelease.createdAt), + }); + + if (latestRelease?.versionId === versionId) + return { created: false, release: latestRelease }; + + const release = await tx + .insert(schema.versionRelease) + .values({ releaseTargetId: this.releaseTarget.id, versionId }) + .returning() + .then(takeFirst); + + return { created: true, release }; + }); }
🧹 Nitpick comments (4)
apps/event-worker/src/workers/evaluate-release-target.ts (2)
31-31: Improve error message specificityThe error message could be more descriptive to help with debugging. Consider including the ID in the error message.
- if (!versionRelease) throw new Error("Failed to get release"); + if (!versionRelease) throw new Error(`Failed to get version release with ID: ${versionReleaseId}`);
45-45: Improve error message specificitySimilar to the previous error message, this one could also be more descriptive.
- if (!variableRelease) throw new Error("Failed to get variable release"); + if (!variableRelease) throw new Error(`Failed to get variable release with ID: ${variableReleaseId}`);packages/rule-engine/src/manager/version-manager.ts (1)
36-41: Add explicit return type for findLatestRelease methodThe
findLatestReleasemethod doesn't explicitly specify its return type, which could lead to confusion. Consider adding a specific return type to improve code readability and type safety.- async findLatestRelease() { + async findLatestRelease(): Promise<typeof schema.versionRelease.$inferSelect | undefined> { return this.db.query.versionRelease.findFirst({ where: eq(schema.versionRelease.releaseTargetId, this.releaseTarget.id), orderBy: desc(schema.versionRelease.createdAt), }); }packages/db/src/schema/release.ts (1)
57-65: Consider adding an index on releaseTargetIdThe
variableReleasetable defines a foreign key toreleaseTarget.idbut doesn't have an index on this column. Consider adding an index to improve query performance, especially since this column will likely be used in WHERE clauses frequently.export const variableRelease = pgTable("variable_release", { id: uuid("id").primaryKey().defaultRandom(), releaseTargetId: uuid("release_target_id") .notNull() .references(() => releaseTarget.id, { onDelete: "cascade" }), createdAt: timestamp("created_at", { withTimezone: true }) .notNull() .defaultNow(), +}, (t) => ({ + releaseTargetIdx: index("variable_release_target_id_idx").on(t.releaseTargetId), }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/event-worker/src/workers/evaluate-release-target.ts(3 hunks)packages/db/src/schema/release.ts(4 hunks)packages/rule-engine/src/manager/variable-manager.ts(1 hunks)packages/rule-engine/src/manager/version-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rule-engine/src/manager/variable-manager.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/event-worker/src/workers/evaluate-release-target.tspackages/rule-engine/src/manager/version-manager.tspackages/db/src/schema/release.ts
🧬 Code Definitions (3)
apps/event-worker/src/workers/evaluate-release-target.ts (5)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (3)
versionRelease(42-55)variableRelease(57-65)releaseTarget(18-40)packages/db/src/schema/job.ts (1)
job(74-106)packages/rule-engine/src/manager/version-manager.ts (1)
VersionReleaseManager(15-78)packages/rule-engine/src/manager/variable-manager.ts (1)
VariableReleaseManager(12-66)
packages/rule-engine/src/manager/version-manager.ts (7)
packages/rule-engine/src/manager/types.ts (2)
ReleaseManager(9-11)ReleaseTarget(1-7)packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/rule-engine/src/db/get-applicable-policies.ts (1)
getApplicablePolicies(108-139)packages/rule-engine/src/utils/merge-policies.ts (1)
mergePolicies(29-47)packages/rule-engine/src/types.ts (1)
RuleEngineContext(35-40)packages/rule-engine/src/manager/version-manager-rules.ts (1)
getRules(63-72)packages/rule-engine/src/manager/version-rule-engine.ts (1)
VersionRuleEngine(27-151)
packages/db/src/schema/release.ts (2)
packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(109-134)packages/db/src/schema/job.ts (1)
job(74-106)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (8)
apps/event-worker/src/workers/evaluate-release-target.ts (3)
19-79: Function refactored for versioned release architectureThis function has been renamed from
createJobForReleasetocreateReleaseand updated to work with the new versioned release architecture. The implementation now handles both version releases and variable releases separately, which aligns with the modernized rule engine structure.
102-126: Clean implementation of the new rule engine architectureThe code now uses the
VersionReleaseManagerandVariableReleaseManagerclasses to manage releases, which is a cleaner architecture compared to the legacy deployment evaluation logic. This makes the code more modular and easier to understand.
128-134:❓ Verification inconclusive
Verify job creation behavior for production environments
The code only creates and dispatches a job in development environments. If this is intentional, it's fine, but if jobs should also be created in production, you should add the necessary code for production environments.
🏁 Script executed:
#!/bin/bash # Check if there's a separate job creation mechanism for production environments rg -A 5 'createRelease\(' --type tsLength of output: 1760
Production Job Creation Verification
The current implementation in
apps/event-worker/src/workers/evaluate-release-target.tscreates and dispatches a job only whenenv.NODE_ENVis set to"development". The search did not reveal any separate job creation mechanism for production environments. Please verify that this behavior is intentional. If job creation should also occur in production, consider implementing the corresponding logic or adding an inline comment that clarifies where production jobs are handled.packages/rule-engine/src/manager/version-manager.ts (2)
15-20: Well-structured manager class implementationThe
VersionReleaseManagerclass provides a clean interface for managing version releases, with good separation of concerns and appropriate use of dependency injection for the database client.
56-77: Well-implemented evaluation methodThe
evaluatemethod correctly retrieves the context, fetches policies and rules, and evaluates them using the rule engine. It includes appropriate error handling and returns the evaluation result.packages/db/src/schema/release.ts (3)
42-55: Good schema restructuring for versioned releasesThe
versionReleasetable correctly represents the new architecture where releases are versioned. The structure includes all necessary fields and appropriate references to related tables.
81-95: Well-designed release join tableThe
releasetable effectively joins version releases, variable releases, and jobs. This design allows for flexible management of releases while maintaining data integrity through appropriate foreign key constraints.
97-174: Comprehensive relation definitionsThe relations section properly defines the relationships between all the tables, which will facilitate querying the data with proper joins. Good job ensuring all the necessary relationships are defined.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
97-101: 🛠️ Refactor suggestionImprove type safety by adding a proper interface instead of using
anySimilar to the previous comment, this function also uses
anyfor thereleaseTargetparameter, which reduces type safety.-const handleVariableRelease = async (releaseTarget: any) => { +const handleVariableRelease = async (releaseTarget: schema.ReleaseTarget & { + resource: schema.Resource; + environment: schema.Environment; + deployment: schema.Deployment; +}) => { const varrm = new VariableReleaseManager(db, { ...releaseTarget, workspaceId: releaseTarget.resource.workspaceId, });
🧹 Nitpick comments (5)
apps/event-worker/src/workers/evaluate-release-target.ts (2)
131-132: Consider wrapping both release operations in a single transactionThe version and variable release operations are performed separately. If one succeeds and the other fails, you may end up with an inconsistent state. Consider wrapping both operations in a single transaction.
- const versionRelease = await handleVersionRelease(releaseTarget); - const variableRelease = await handleVariableRelease(releaseTarget); + const { versionRelease, variableRelease } = await db.transaction(async (tx) => { + const vrmTx = new VersionReleaseManager(tx, { + ...releaseTarget, + workspaceId: releaseTarget.resource.workspaceId, + }); + const varrmTx = new VariableReleaseManager(tx, { + ...releaseTarget, + workspaceId: releaseTarget.resource.workspaceId, + }); + + const { chosenCandidate } = await vrmTx.evaluate(); + if (!chosenCandidate) throw new Error("Failed to get chosen release"); + const { release: versionRelease } = await vrmTx.upsertRelease(chosenCandidate.id); + + const { chosenCandidate: variableValues } = await varrmTx.evaluate(); + const { release: variableRelease } = await varrmTx.upsertRelease(variableValues); + + return { versionRelease, variableRelease }; + });
17-17: Update worker name in logger to match file purposeThe worker name in the logger is "policy-evaluate" but the file is about evaluating release targets. Consider updating the worker name for consistency.
-const log = logger.child({ worker: "policy-evaluate" }); +const log = logger.child({ worker: "evaluate-release-target" });packages/rule-engine/src/manager/variable-manager.ts (3)
35-37: Potential performance optimization for large variable setsThe deep comparison with
_.isEqualcan be expensive for large variable sets. For very large sets, consider using a more optimized approach like comparing object keys count first or using hashing.- const isSame = _.isEqual(oldVars, newVars); + // Quick length check before deep comparison + const isSame = Object.keys(oldVars).length === Object.keys(newVars).length && + _.isEqual(oldVars, newVars);
13-14: Cached policy field is unusedThe
cachedPolicyfield is initialized but never used in this class. Consider removing it if it's not needed, or implement the caching mechanism if it's intended for future use.export class VariableReleaseManager implements ReleaseManager { - private cachedPolicy: Policy | null = null;
1-73: Missing method documentation commentsConsider adding JSDoc comments for the public methods to improve code documentation. This helps other developers understand how to use these methods properly.
+ /** + * Creates or retrieves a variable release. + * If the variables are the same as the latest release, returns the existing release. + * Otherwise, creates a new release with the provided variables. + * @param variables - Array of variables (possibly including nulls) to be associated with the release + * @returns Object containing created flag and the release + */ async upsertRelease(variables: MaybeVariable[]) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/event-worker/src/workers/evaluate-release-target.ts(2 hunks)apps/event-worker/src/workers/job-dispatch/github.ts(2 hunks)apps/event-worker/src/workers/new-deployment-version.ts(0 hunks)apps/event-worker/src/workers/update-deployment-variable.ts(0 hunks)apps/event-worker/src/workers/update-resource-variable.ts(0 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/manager/variable-manager.ts(1 hunks)packages/rule-engine/src/repositories/db-release-repository.ts(0 hunks)packages/rule-engine/src/repositories/get-releases.ts(0 hunks)packages/rule-engine/src/repositories/index.ts(0 hunks)packages/rule-engine/src/repositories/types.ts(0 hunks)
💤 Files with no reviewable changes (7)
- apps/event-worker/src/workers/new-deployment-version.ts
- apps/event-worker/src/workers/update-deployment-variable.ts
- packages/rule-engine/src/repositories/db-release-repository.ts
- apps/event-worker/src/workers/update-resource-variable.ts
- packages/rule-engine/src/repositories/index.ts
- packages/rule-engine/src/repositories/types.ts
- packages/rule-engine/src/repositories/get-releases.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/event-worker/src/workers/job-dispatch/github.ts
- packages/rule-engine/src/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.
apps/event-worker/src/workers/evaluate-release-target.tspackages/rule-engine/src/manager/variable-manager.ts
🧬 Code Definitions (2)
apps/event-worker/src/workers/evaluate-release-target.ts (5)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (3)
versionRelease(42-55)variableRelease(57-65)releaseTarget(18-40)packages/db/src/schema/job.ts (1)
job(74-106)packages/rule-engine/src/manager/version-manager.ts (1)
VersionReleaseManager(15-78)packages/rule-engine/src/manager/variable-manager.ts (1)
VariableReleaseManager(13-73)
packages/rule-engine/src/manager/variable-manager.ts (5)
packages/rule-engine/src/manager/types.ts (2)
ReleaseManager(9-11)ReleaseTarget(1-7)packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (2)
releaseTarget(18-40)release(81-95)packages/rule-engine/src/manager/variables/types.ts (1)
MaybeVariable(9-9)packages/rule-engine/src/manager/variables/variables.ts (1)
VariableManager(31-68)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
apps/event-worker/src/workers/evaluate-release-target.ts (2)
19-79: Code looks well structured with appropriate error handlingThe
createReleasefunction is well structured with appropriate error handling for missing data. It correctly:
- Retrieves necessary data
- Checks for required entities
- Creates a job with the right configuration
- Associates variables with the job
- Creates a release record
110-149: Error handling looks appropriate with proper mutex managementThe worker implementation correctly handles locking, error catching, and unlocking in a finally block to ensure the mutex is always released, even if an error occurs.
packages/rule-engine/src/manager/variable-manager.ts (2)
21-58: Well-implemented upsert with optimizations to prevent unnecessary database writesThe
upsertReleasemethod efficiently:
- Checks if variables have changed before creating a new release
- Uses transactions for data consistency
- Handles empty variable arrays
- Returns appropriate metadata about the operation
68-72:❓ Verification inconclusive
Consider adding error handling for variable retrieval
The
evaluatemethod doesn't include explicit error handling. IfvariableManager.getVariables()fails, the error will propagate up. Consider adding try/catch if there are specific recovery actions needed.
🏁 Script executed:
#!/bin/bash # Check for error handling patterns in similar evaluate methods rg -A 2 -B 2 "async evaluate\(\)" --type tsLength of output: 864
Action: Review Error Handling Strategy in
evaluateMethodAfter verifying similar implementations in the codebase (e.g., in
packages/rule-engine/src/manager/version-manager.ts), it appears that the absence of local try/catch blocks in theevaluatemethods is a deliberate design choice to let errors propagate for centralized handling. If there’s no specific recovery action required immediately at this layer, the current behavior is consistent with other parts of the system. However, if you foresee a need for more granular error management or improved logging/debugging at this level, consider wrapping the variable retrieval in a try/catch block. For example:
Option 1 (Keep current approach):
Rely on the higher-level error handler if the design intent is to allow error propagation.Option 2 (Add error handling):
async evaluate() { try { const variableManager = await VariableManager.database(this.releaseTarget); const variables = await variableManager.getVariables(); return { chosenCandidate: variables }; } catch (error) { // Optionally log the error or perform recovery actions throw error; } }Please verify if the current design’s error propagation aligns with the broader error management strategy of the project, or if adding localized error handling would provide additional benefits.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rule-engine/src/manager/__tests__/version-rule-engine.test.ts (2)
16-31: Consider using a descriptive parameter name instead of_While using
_for unused parameters is common, consider using a more descriptive name likecontexteven if it's unused. This makes the code more self-documenting, especially if future implementations might need to use the context parameter.- filter( - _: RuleEngineContext, - candidates: Version[], - ): RuleEngineRuleResult<Version> { + filter( + context: RuleEngineContext, + candidates: Version[], + ): RuleEngineRuleResult<Version> {
88-158: Consider adding test for async rule behaviorThe RuleEngineFilter interface supports returning either a direct result or a Promise, but the tests only cover the synchronous case. Consider adding a test with a mock rule that returns a Promise to verify that the engine properly handles asynchronous rules.
class AsyncMockRule implements RuleEngineFilter<Version> { public readonly name = "AsyncMockRule"; constructor(private readonly allowedIds: string[]) {} async filter( context: RuleEngineContext, candidates: Version[], ): Promise<RuleEngineRuleResult<Version>> { // Simulate async operation await new Promise(resolve => setTimeout(resolve, 10)); const rejectionReasons = new Map<string, string>(); const allowedCandidates = candidates.filter((candidate) => { if (this.allowedIds.includes(candidate.id)) { return true; } rejectionReasons.set(candidate.id, `Rejected by ${this.name}`); return false; }); return { allowedCandidates, rejectionReasons }; } } it("should handle async rules", async () => { const asyncRule = new AsyncMockRule(["ver-2", "ver-3"]); const engine = new VersionRuleEngine([asyncRule]); const result = await engine.evaluate(context, candidates); expect(result.chosenCandidate).not.toBeNull(); expect(result.chosenCandidate?.id).toBe("ver-3"); // Should pick ver-3 as it's the oldest sequential upgrade expect(result.rejectionReasons.get("ver-1")).toBe("Rejected by AsyncMockRule"); expect(result.rejectionReasons.get("ver-4")).toBe("Rejected by AsyncMockRule"); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rule-engine/src/manager/__tests__/version-rule-engine.test.ts(1 hunks)packages/rule-engine/src/releases.ts(0 hunks)packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts(10 hunks)packages/rule-engine/src/rules/__tests__/version-approval-rule.test.ts(4 hunks)packages/rule-engine/src/types.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/rule-engine/src/releases.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rule-engine/src/rules/tests/version-approval-rule.test.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/manager/__tests__/version-rule-engine.test.tspackages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.tspackages/rule-engine/src/types.ts
🧬 Code Definitions (3)
packages/rule-engine/src/manager/__tests__/version-rule-engine.test.ts (2)
packages/rule-engine/src/types.ts (3)
RuleEngineFilter(43-49)RuleEngineContext(23-28)RuleEngineRuleResult(30-33)packages/rule-engine/src/manager/version-rule-engine.ts (2)
Version(10-16)VersionRuleEngine(27-151)
packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts (2)
packages/rule-engine/src/types.ts (1)
RuleEngineContext(23-28)packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
DeploymentDenyRule(57-176)
packages/rule-engine/src/types.ts (1)
packages/db/src/schema/resource.ts (1)
Resource(105-105)
🪛 Biome (1.9.4)
packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts
[error] 6-17: Do not export from a test file.
(lint/suspicious/noExportsInTest)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (33)
packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts (24)
4-4: LGTM! Updated import correctly.The import has been updated from
DeploymentResourceContexttoRuleEngineContextwhich aligns with the broader refactoring effort in the rule engine.
20-21: LGTM! Variable types successfully updated.The variables
releasesandcontexthave been properly updated to reflect the new type definitions (ResolvedRelease[]andRuleEngineContext).
72-77: LGTM! Generic type parameter correctly added.The
DeploymentDenyRuleconstructor now uses the generic type parameter<ResolvedRelease>and includes the requiredgetCandidateIdfunction, which aligns with the updated interface.
86-89: LGTM! Test assertion updated to use new property name.The test assertion has been correctly updated to check
allowedCandidates.lengthinstead ofallowedReleases.length, reflecting the property name change in the result type.
93-99: LGTM! Constructor call correctly includes generic type and callback.The
DeploymentDenyRuleconstructor now properly uses the generic type parameter and includes thegetCandidateIdcallback function.
108-117: LGTM! Tests updated to verify rejection behavior.The assertion has been correctly updated to check
allowedCandidates.lengthand the rejection reasons are properly verified.
121-127: LGTM! Custom reason test updated correctly.The constructor call now includes the generic type parameter and the
getCandidateIdfunction, maintaining the test's functionality while adapting to the new API.
144-151: LGTM! Time interval test updated correctly.The constructor call has been properly updated with the generic type parameter and
getCandidateIdfunction while maintaining the time-based rule configuration.
157-160: LGTM! Test assertions properly updated.The test assertion has been correctly updated to check
allowedCandidates.lengthfor the within-denied-period test case.
165-168: LGTM! Test assertions updated for outside-denied-period case.The assertion has been correctly updated to check
allowedCandidates.lengthfor the outside-denied-period test case.
173-176: LGTM! Test assertions updated for evening time case.The assertion has been correctly updated to check
allowedCandidates.lengthfor the evening outside-denied-period test case.
180-187: LGTM! Timezone test updated correctly.The constructor call now includes the generic type parameter and
getCandidateIdfunction while maintaining the timezone-specific configuration.
193-196: LGTM! Timezone within-denied-period test assertion updated.The assertion has been correctly updated to check
allowedCandidates.lengthfor the timezone-specific within-denied-period test case.
201-204: LGTM! Timezone outside-denied-period test assertion updated.The assertion has been correctly updated to check
allowedCandidates.lengthfor the timezone-specific outside-denied-period test case.
207-213: LGTM! DST change test updated correctly.The constructor call now includes the generic type parameter and
getCandidateIdfunction while maintaining the daylight saving time transition test configuration.
225-228: LGTM! DST change before-transition test updated correctly.The assertion has been correctly updated to check
allowedCandidates.lengthfor the DST before-transition test case.
232-235: LGTM! DST change after-transition test updated correctly.The assertion has been correctly updated to check
allowedCandidates.lengthfor the DST after-transition test case.
246-249: LGTM! Morning DST test assertion updated correctly.The assertion has been correctly updated to check
allowedCandidates.lengthfor the morning before-DST-transition test case.
253-256: LGTM! Morning after-DST test assertion updated correctly.The assertion has been correctly updated to check
allowedCandidates.lengthfor the morning after-DST-transition test case.
259-265: LGTM! Fall DST test updated correctly.The constructor call now includes the generic type parameter and
getCandidateIdfunction while maintaining the fall DST transition test configuration.
277-280: LGTM! Fall DST before-transition test assertion updated.The assertion has been correctly updated to check
allowedCandidates.lengthfor the fall DST before-transition test case.
284-287: LGTM! Fall DST after-transition test assertion updated.The assertion has been correctly updated to check
allowedCandidates.lengthfor the fall DST after-transition test case.
298-301: LGTM! Evening fall DST before-transition test assertion updated.The assertion has been correctly updated to check
allowedCandidates.lengthfor the evening fall DST before-transition test case.
305-308: LGTM! Evening fall DST after-transition test assertion updated.The assertion has been correctly updated to check
allowedCandidates.lengthfor the evening fall DST after-transition test case.packages/rule-engine/src/types.ts (5)
23-28: LGTM! Successfully renamed context type.The type has been renamed from
DeploymentResourceContexttoRuleEngineContextwhile maintaining the same properties, which aligns with the broader refactoring of the rule engine.
30-33: LGTM! Rule result type successfully updated.The type has been renamed from
DeploymentResourceRuleResulttoRuleEngineRuleResult<T>and the property has been changed fromallowedReleasestoallowedCandidates. This makes the type more generic and reusable for different types of candidates.
35-38: LGTM! Selection result type successfully updated.The type has been renamed from
DeploymentResourceSelectionResulttoRuleEngineSelectionResult<T>and the property has been changed fromchosenReleasetochosenCandidate, making it more generic and consistent with the other type changes.
43-49: LGTM! Filter interface successfully updated.The interface has been renamed from
DeploymentResourceRuletoRuleEngineFilter<T>and the method signature has been updated to use the new types. The generic type parameter makes the interface more flexible for different types of candidates.
65-70: LGTM! New RuleEngine type added.A new type
RuleEngine<T>has been added with anevaluatemethod that processes a context and candidates to determine the best candidate. This addition aligns with the refactoring effort to make the rule engine more generic and flexible.packages/rule-engine/src/manager/__tests__/version-rule-engine.test.ts (4)
11-31: Good implementation of the mock rule filterThe MockRule implementation correctly follows the RuleEngineFilter interface. It provides a simple way to filter versions by ID and accumulate rejection reasons, which is perfect for testing the VersionRuleEngine.
33-86: Well-structured test setup with comprehensive test dataThe test setup is well-organized with appropriate sample data for the context and candidates. The use of
beforeEachensures consistent test data across all test cases.
88-158: Comprehensive test suite covering critical scenariosThe test cases cover a good range of scenarios including sequential rule application, empty candidates, sequential upgrades, and rejection reason accumulation. This provides good coverage for the VersionRuleEngine implementation.
75-76:❓ Verification inconclusive
Note on metadata boolean representation
The metadata uses string values ("true") for boolean flags. While this works, consider if actual boolean values would be more appropriate for the
requiresSequentialUpgradeflag, depending on how the implementation handles these values.Also applies to: 82-83
🏁 Script executed:
#!/bin/bash # Check how Version metadata is used in the codebase grep -r "requiresSequentialUpgrade" --include="*.ts" .Length of output: 434
Action Required: Revisit the Metadata Boolean Representation
After verifying the usage in the codebase, it appears that the
requiresSequentialUpgradeflag is intentionally represented as a string in both tests and implementation:
- The tests in
packages/rule-engine/src/manager/__tests__/version-rule-engine.test.ts(lines 75–76 and 82–83) set the flag as"true".- The implementation in
packages/rule-engine/src/manager/version-rule-engine.tsexplicitly checksv.metadata.requiresSequentialUpgrade === "true".While this consistent usage works as-is, if you decide that using an actual boolean would be clearer or more type-safe, you'll need to update both the test fixtures and the comparison logic accordingly.
| export type ResolvedRelease = { | ||
| id: string; | ||
| createdAt: Date; | ||
| version: { | ||
| id: string; | ||
| tag: string; | ||
| config: Record<string, any>; | ||
| metadata: Record<string, string>; | ||
| }; | ||
| variables: Record<string, unknown>; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving this type definition out of the test file.
Exporting type definitions from test files is generally not recommended as it could lead to circular dependencies or make the test file part of your public API unintentionally.
#!/bin/bash
# Check if this type is being used elsewhere in the codebase
rg "import.*ResolvedRelease.*from.*deployment-deny-rule\.test" --type=ts🧰 Tools
🪛 Biome (1.9.4)
[error] 6-17: Do not export from a test file.
(lint/suspicious/noExportsInTest)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rule-engine/src/manager/version-manager.ts (1)
31-43: Consider concurrency guard forupsertRelease.No transaction is used here. If multiple requests try to create a release with the same
versionIdconcurrently, two releases may be created. If uniqueness is required, consider wrapping these inserts in a transaction or defining a unique constraint to avoid duplicates.
🧹 Nitpick comments (2)
packages/rule-engine/src/manager/version-manager.ts (2)
64-64: Typo in method name.Please rename
findLastestDeployedVersiontofindLatestDeployedVersionfor clarity.- async findLastestDeployedVersion() { + async findLatestDeployedVersion() {
142-153: Caching policy implementation is reasonable.A basic per-instance in-memory cache may be sufficient unless your system experiences multiple concurrent requests in long-lived processes. If you need a more robust caching approach, you might consider an external store or time-based invalidation strategy.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rule-engine/src/manager/version-manager.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/manager/version-manager.ts
🧬 Code Definitions (1)
packages/rule-engine/src/manager/version-manager.ts (8)
packages/rule-engine/src/manager/types.ts (2)
ReleaseManager(9-11)ReleaseTarget(1-7)packages/db/src/schema/release.ts (2)
releaseTarget(18-40)release(81-95)packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(109-134)packages/db/src/schema/job.ts (1)
JobStatus(136-136)packages/rule-engine/src/db/get-applicable-policies.ts (1)
getApplicablePolicies(108-139)packages/rule-engine/src/utils/merge-policies.ts (1)
mergePolicies(29-47)packages/rule-engine/src/types.ts (1)
RuleEngineContext(23-28)packages/rule-engine/src/manager/version-rule-engine.ts (1)
VersionRuleEngine(27-151)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
packages/rule-engine/src/manager/version-manager.ts (5)
1-3: Imports look good.Clean usage of external libraries. No issues to report.
24-29: Class definition appears correct.The constructor is public, and dependency injection is clearly handled.
89-133: Logic for version filtering is well-structured.The use of condition-based filtering and in-place metadata transformation is clear and efficient.
135-140: Sufficient approach to retrieving the latest release.The ordering by descending creation date is standard and appropriate.
155-176: Confirm robustness of the evaluation logic.If no versions are found, the engine might return null or an empty result. Verify that the caller of
evaluate()properly handles that scenario.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/db/src/schema/release.ts (1)
35-36: Consider addressing the circular referenceThere's a circular reference between
releaseTargetandreleasewhere:
releaseTargetreferencesrelease.idon line 35- The new
releasetable (line 100) indirectly referencesreleaseTargetthroughversionReleaseandvariableSetReleaseThis circular nature is handled by
: anytype cast, but it would be better to use a forward reference pattern consistently.- .references((): any => release.id, { onDelete: "set null" }) + // Use consistent forward reference pattern + .references(() => release.id as any, { onDelete: "set null" })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/db/src/schema/release.ts(5 hunks)packages/rule-engine/src/manager/variable-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rule-engine/src/manager/variable-manager.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/db/src/schema/release.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (8)
packages/db/src/schema/release.ts (8)
17-17: Good addition of workspace importThis import is properly added to support the reference to the workspace table in the new variableValueSnapshot table.
43-56: LGTM: Proper renaming of release table to versionReleaseThe renamed table properly captures the specific version-related aspects of a release with appropriate references to deploymentVersion and releaseTarget.
58-66: LGTM: Good addition of variableSetRelease tableThis table correctly represents variable set releases with proper references to the releaseTarget table.
68-82: LGTM: Well-structured variableSetReleaseValue tableThe table properly links variable set releases with their corresponding variable value snapshots, and includes a unique index constraint to prevent duplicates.
84-98: LGTM: Good design of variableValueSnapshot tableThis table effectively implements immutable snapshots of variable values with proper workspace association and uniqueness constraints.
100-111: LGTM: Well-designed release tableThe new release table properly ties together both version releases and variable releases, creating a cohesive release object that can be referenced by other parts of the system.
123-144: LGTM: Updated releaseTarget relationsThe relations are properly updated to reference the new table names and add connections to both versionReleases and variableReleases.
146-208: LGTM: Well-defined relation objectsAll new tables have properly defined relations that accurately reflect the foreign key constraints in the schema. The bidirectional relationships between entities are maintained consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/db/src/schema/deployment-variables.ts (1)
82-93: Consider updating the validation schema to include the new sensitive field.The
createDeploymentVariableValueschema doesn't explicitly account for the newsensitivefield. While this will work due to the default value, consider extending the schema to allow clients to specify this field during creation.export const createDeploymentVariableValue = createInsertSchema( deploymentVariableValue, { resourceSelector: resourceCondition }, ) .omit({ id: true, }) .extend({ default: z.boolean().optional(), + sensitive: z.boolean().optional().default(false), });packages/db/src/schema/variable-sets.ts (1)
50-80: Consider adding validation schema for variableSetValue.Unlike
deploymentVariableValue, there's no explicit validation schema for creating or updatingvariableSetValueentries. For consistency, consider adding one that includes the newsensitivefield.export const createVariableSetValue = createInsertSchema(variableSetValue) .omit({ id: true }) .extend({ sensitive: z.boolean().optional().default(false), }); export const updateVariableSetValue = createVariableSetValue.partial();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/db/src/schema/deployment-variables.ts(2 hunks)packages/db/src/schema/variable-sets.ts(2 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/deployment-variables.tspackages/db/src/schema/variable-sets.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
packages/db/src/schema/deployment-variables.ts (1)
64-64: LGTM - Addition of sensitive field for handling secure data.Adding the
sensitivefield with a default value offalseis a good approach for marking variable values that contain sensitive information. This will allow filtering or special handling of sensitive values in the application layer.packages/db/src/schema/variable-sets.ts (2)
3-10: LGTM - Improved import organization.Grouping related imports from the same module is a good practice for better code organization and readability.
65-65: LGTM - Consistent implementation of sensitive field.Adding the
sensitivefield to thevariableSetValuetable maintains consistency with the similar field indeploymentVariableValue. This parallel structure will make working with both tables more intuitive.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
79-83: Improve type safety by adding a proper interface instead of usinganyThe
releaseTargetparameter is typed asany, which reduces type safety. Consider creating or using an existing interface that describes the structure of the release target.-const handleVersionRelease = async (releaseTarget: any) => { +const handleVersionRelease = async (releaseTarget: schema.ReleaseTarget & { + resource: schema.Resource; + environment: schema.Environment; + deployment: schema.Deployment; +}) => { const vrm = new VersionReleaseManager(db, { ...releaseTarget, workspaceId: releaseTarget.resource.workspaceId, });
🧹 Nitpick comments (1)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
132-140: Consider handling conflicts explicitlyThe
onConflictDoNothing()method will silently ignore conflicts when inserting records. This could make debugging issues difficult if you expect a record to be created but it isn't due to a conflict. Consider handling conflicts explicitly or logging when they occur.const release = await db .insert(schema.release) .values({ versionReleaseId: versionRelease.id, variableReleaseId: variableRelease.id, }) - .onConflictDoNothing() + .onConflictDoUpdate({ + target: [schema.release.versionReleaseId, schema.release.variableReleaseId], + set: { updatedAt: new Date() } + }) .returning() .then(takeFirst); + + if (!release) { + log.warn("Release already exists", { + versionReleaseId: versionRelease.id, + variableReleaseId: variableRelease.id, + }); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/event-worker/src/workers/evaluate-release-target.ts(2 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/evaluate-release-target.ts
🧬 Code Graph Analysis (1)
apps/event-worker/src/workers/evaluate-release-target.ts (4)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (3)
release(110-121)versionRelease(43-57)releaseTarget(19-41)packages/rule-engine/src/manager/version-manager.ts (1)
VersionReleaseManager(24-177)packages/rule-engine/src/manager/variable-manager.ts (1)
VariableReleaseManager(13-76)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
apps/event-worker/src/workers/evaluate-release-target.ts (5)
33-35: Consider checking for nested property existenceBefore destructuring
versionRelease.version.deployment, it would be safer to verify thatversionRelease.versionexists to prevent potential runtime errors if the data structure is incomplete.- const { jobAgent, jobAgentConfig: deploymentJobAgentConfig } = - versionRelease.version.deployment; - if (!jobAgent) throw new Error("Deployment has no Job Agent"); + if (!versionRelease.version) throw new Error("Version data not found"); + const { jobAgent, jobAgentConfig: deploymentJobAgentConfig } = + versionRelease.version.deployment; + if (!jobAgent) throw new Error("Deployment has no Job Agent");
19-77: LGTM: createRelease function is well-structuredThe renamed and refactored
createReleasefunction is well-structured with clear separation of concerns, proper error handling, and logical flow. The function extracts the necessary information from version and variable releases, creates a job with the correct configuration, and associates variables and release records properly.
85-93: LGTM: Version release handling logic is clearThe version release evaluation and upserting logic is clear and follows a good pattern of evaluating first, then upserting the result. The error handling for missing chosen candidates is appropriately included.
101-106: LGTM: Variable release handling logic is clearSimilar to the version release handling, the variable release logic follows a clean evaluate-then-upsert pattern, maintaining consistency in the codebase.
142-145:❓ Verification inconclusive
Verify the conditional job creation logic
Job creation only happens in development mode. Is this intentional? If job creation is required in production as well, this condition should be removed. If there's another mechanism for production, consider adding a comment explaining why this is dev-only.
🏁 Script executed:
#!/bin/bash # Check if there's any other code handling job creation in production # Search for imports or uses of createRelease function rg -A 5 "createRelease" --type ts # Search for dispatchJob channel usage to see if jobs are created elsewhere rg -A 5 "Channel.DispatchJob" --type tsLength of output: 23544
Action Required: Clarify Conditional Job Creation in evaluate-release-target
Our search confirms that the code in
apps/event-worker/src/workers/evaluate-release-target.tsonly creates a job whenNODE_ENVequals"development". We found no alternative production logic in this worker; production job creation appears to be handled elsewhere (e.g., viacreateReleaseJobTriggersin various API routes). Please verify whether this design is intentional. If so, a clarifying comment should be added to explain why this worker is limited to development mode. Otherwise, consider updating the conditional to also create jobs in production.
| const handleVariableRelease = async (releaseTarget: any) => { | ||
| const varrm = new VariableReleaseManager(db, { | ||
| ...releaseTarget, | ||
| workspaceId: releaseTarget.resource.workspaceId, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper typing to function parameter
Similar to handleVersionRelease, this function uses any for the releaseTarget parameter, which reduces type safety. Use the same interface to ensure consistent typing across functions.
-const handleVariableRelease = async (releaseTarget: any) => {
+const handleVariableRelease = async (releaseTarget: schema.ReleaseTarget & {
+ resource: schema.Resource;
+ environment: schema.Environment;
+ deployment: schema.Deployment;
+}) => {
const varrm = new VariableReleaseManager(db, {
...releaseTarget,
workspaceId: releaseTarget.resource.workspaceId,
});📝 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 handleVariableRelease = async (releaseTarget: any) => { | |
| const varrm = new VariableReleaseManager(db, { | |
| ...releaseTarget, | |
| workspaceId: releaseTarget.resource.workspaceId, | |
| }); | |
| const handleVariableRelease = async (releaseTarget: schema.ReleaseTarget & { | |
| resource: schema.Resource; | |
| environment: schema.Environment; | |
| deployment: schema.Deployment; | |
| }) => { | |
| const varrm = new VariableReleaseManager(db, { | |
| ...releaseTarget, | |
| workspaceId: releaseTarget.resource.workspaceId, | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/event-worker/src/workers/evaluate-release-target.ts (2)
79-79: Use stronger typing forreleaseTarget.
Usinganyreduces type safety and maintainability. A typed interface ensures clarity.
95-95: Use stronger typing forreleaseTargetinhandleVariableRelease.
This repeats the earlier suggestion for typed parameters.packages/rule-engine/src/manager/version-manager.ts (1)
31-43:upsertRelease(versionId: string)concurrency concern.
If multiple processes callupsertReleaseconcurrently with the sameversionId, it might create duplicate records unless there's a DB-level constraint. Consider a transaction or a unique index on(releaseTargetId, versionId)to ensure uniqueness.
🧹 Nitpick comments (2)
apps/event-worker/src/workers/evaluate-release-target.ts (2)
30-30: ValidateversionRelease.versionexistence.
Though unlikely, consider checkingversionRelease.versionorversionRelease.version.deploymentfor null references to avoid runtime errors, especially if data is malformed.if (!versionRelease || !versionRelease.version?.deployment) { throw new Error("Failed to get version/deployment"); }Also applies to: 32-35
128-136: Insert newreleaserecord withonConflictDoNothing().
This prevents duplicate records but might silently skip needed updates if changes occur..onConflictDoNothing() +.onConflictUseUpdate({ + versionReleaseId: versionRelease.id, + variableReleaseId: variableRelease.id, +})
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/event-worker/src/workers/evaluate-release-target.ts(1 hunks)packages/rule-engine/src/manager/variable-manager.ts(1 hunks)packages/rule-engine/src/manager/version-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rule-engine/src/manager/variable-manager.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/event-worker/src/workers/evaluate-release-target.tspackages/rule-engine/src/manager/version-manager.ts
🧬 Code Graph Analysis (2)
apps/event-worker/src/workers/evaluate-release-target.ts (5)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (3)
release(110-121)versionRelease(43-57)releaseTarget(19-41)packages/db/src/schema/job.ts (1)
job(74-106)packages/rule-engine/src/manager/version-manager.ts (1)
VersionReleaseManager(24-181)packages/rule-engine/src/manager/variable-manager.ts (1)
VariableReleaseManager(13-88)
packages/rule-engine/src/manager/version-manager.ts (7)
packages/rule-engine/src/manager/types.ts (2)
ReleaseManager(9-11)ReleaseTarget(1-7)packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(109-134)packages/db/src/schema/job.ts (1)
JobStatus(136-136)packages/rule-engine/src/db/get-applicable-policies.ts (1)
getApplicablePolicies(108-139)packages/rule-engine/src/utils/merge-policies.ts (1)
mergePolicies(29-47)packages/rule-engine/src/types.ts (1)
RuleEngineContext(23-28)packages/rule-engine/src/manager/version-rule-engine.ts (1)
VersionRuleEngine(27-151)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (30)
apps/event-worker/src/workers/evaluate-release-target.ts (20)
17-17: Looks good!
Using a child logger instance improves log contextualization.
19-23: Good use of typed parameters increateRelease.
The function signature properly delineates thereleaseobject structure.
25-25: Query correctness check.
Ensures the correctversionReleaseIdis fetched. No issues found.
39-46: Variable release check.
FetchingvariableReleasewith strong constraints ensures fast failure if data is missing. This is appropriate for mission-critical flows.
58-59: Clear comment on adding job variables.
This clarifies the intent and reduces confusion for other developers.
70-70: ReleaseJob relation creation.
Associating the newly-createdreleaserecord with a job is consistent with the schema. This helps track and retrieve related job metadata.Also applies to: 72-72
80-83: Instantiation ofVersionReleaseManager.
Looks correct and passes the necessary workspace context.
85-87: Evaluation result check.
This early throw is good for preventing usage of undefined data.
88-91: Upsert release logic is straightforward.
No concurrency concerns here since the manager handles creation or reuse.
92-92: Return statement and function closure inhandleVersionRelease.
Implementation is concise and clear.Also applies to: 93-93
96-99: VariableReleaseManager instantiation.
Mirrors howVersionReleaseManageris used, maintaining parity in approach.
101-103: Evaluation and upsert for variables.
Logic is consistent and ensures the correct variable release is created.
105-105: Return statement and function closure inhandleVariableRelease.
The function is succinct and clear.Also applies to: 106-106
112-112: Mutex usage.
Locking the release target ensures concurrency control, well done.
114-114: Comment for retrieving the release target.
Keeps the code self-documenting.
121-121: Eager loading of related data.
Fetchingresource,environment,deploymentin one query is effective.
123-123: Guard clause for missing release target.
Immediate throw helps troubleshoot missing data quickly.
125-126: Sequential handling of version and variable release.
Both operations are well-isolated in dedicated helper functions.
139-139: Transactional approach forcreateRelease.
Wrapping in a transaction ensures atomic operations.
144-146: Verbose error logging.
CapturingerrorandjobIdis helpful for troubleshooting.packages/rule-engine/src/manager/version-manager.ts (10)
1-2: Imports for external libraries.
Everything looks standard.
4-16: Re-exported DB utilities.
All relevant DB methods (and,desc, etc.) are properly imported.
17-23: Import of local rule-engine modules.
Correct references to the internal modules and types.
24-29: Class initialization and property definitions.
ThecachedPolicyproperty ensures we don’t re-fetch policies unnecessarily.
45-62:findLatestVersionMatchingPolicy()
Pulls the relevant policy, then retrieves the highest-createdAt version fulfilling that policy. Logic is clear.
64-91:findLastestDeployedVersion()
Joins multiple tables to find a successful job, ensuring only valid, successfully deployed versions are considered.
93-137:findVersionsForEvaluate()
Combines policy-based conditions, plus a time-window approach using the deployed and matching versions. This is elegantly structured.
139-144:findLatestRelease()
Returns the most recent release for the target. Straightforward usage of descending order.
146-157:getPolicy(forceRefresh)
Caches the merged policy to reduce redundant lookups. This is efficient.
159-181:evaluate()
- Retrieves context.
- Throws an error if missing.
- Fetches & merges policies, then uses the rule engine to evaluate.
All steps are cohesive, simplifying debugging.
| variableRelease.values.map((v) => ({ | ||
| jobId: job.id, | ||
| key: v.key, | ||
| sensitive: v.sensitive, | ||
| value: v.value, | ||
| key: v.variableValueSnapshot.key, | ||
| sensitive: v.variableValueSnapshot.sensitive, | ||
| value: v.variableValueSnapshot.value, |
There was a problem hiding this comment.
Potential security concern for sensitive values.
Storing sensitive variables in plain text might pose risks. Consider encrypting or masking sensitive values in the database or restricting access to these fields.
// Example approach for encryption (pseudo-code illustration)
- value: v.variableValueSnapshot.value,
+ value: encrypt(v.variableValueSnapshot.value),Also applies to: 68-68
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/db/drizzle/0085_sloppy_mister_fear.sql (3)
14-21: [SQL] Creating table "variable_value_snapshot".
The table is established correctly, with a JSONB column forvalueand a booleansensitivefield defaulting to false. As a minor note, please verify that applying a unique index later on a JSONB column (if needed) meets your requirements for equality.
35-38: [SQL] Adding new columns to tables.
The addition of thesensitivecolumn to both"deployment_variable_value"and"variable_set_value", as well as the new"version_release_id"and"variable_release_id"columns on the"release"table, are clearly necessary for the updated schema.Note: When adding
NOT NULLcolumns to an existing table, ensure that data migration is handled (e.g., setting default values or backfilling data) to avoid issues with existing rows.
75-76: [SQL] Creating unique indexes.
The unique index on"variable_set_release_value"and the composite unique index on"variable_value_snapshot"are well-defined.Suggestion: Double-check that a unique index on a JSONB column (i.e.,
"value") functions as intended for your equality semantics.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/db/drizzle/0085_sloppy_mister_fear.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (15)
packages/db/drizzle/0085_sloppy_mister_fear.sql (14)
1-5: [SQL] Creating table "variable_set_release".
The table is defined with a UUID primary key generated bygen_random_uuid()and a default timestamp forcreated_at. This design appears sound.
7-12: [SQL] Creating table "variable_set_release_value".
The table uses UUIDs with proper defaults and includes acreated_atfield. The structure is clear and aligns with the new schema requirements.
23-28: [SQL] Creating table "version_release".
This new table effectively captures versioning with appropriate foreign key references. The design is straightforward and consistent with the overall schema modernization.
30-30: [SQL] Dropping legacy table "release_variable".
Removing the legacy table is appropriate for cleaning up the outdated deployment evaluation logic. Verify that downstream code no longer depends on this table.
31-34: [SQL] Dropping foreign key constraints from "release".
The removal of constraints"release_release_target_id_release_target_id_fk"and"release_version_id_deployment_version_id_fk"is in line with the revised schema. Please ensure that all related application logic has been updated to reference the new relationships.
39-43: [SQL] Adding foreign key constraint on "variable_set_release".
The DO block applies a foreign key on"release_target_id"referencing"public"."release_target". The exception handling for a duplicate constraint is a pragmatic approach in migration scripts.
45-49: [SQL] Adding foreign key constraint on "variable_set_release_value" for "variable_set_release_id".
This constraint ensures referential integrity between"variable_set_release_value"and"variable_set_release". The pattern is consistent with other parts of the migration.
51-55: [SQL] Adding foreign key constraint on "variable_set_release_value" for "variable_value_snapshot_id".
The addition of the foreign key is straightforward and follows the established practice in this script.
57-61: [SQL] Adding foreign key constraint on "variable_value_snapshot" for "workspace_id".
This block properly enforces the relationship with the"workspace"table while handling duplicate constraint exceptions.
63-67: [SQL] Adding foreign key constraint on "version_release" for "release_target_id".
The constraint on"version_release"linking to"release_target"is correctly implemented with cascading deletes.
69-73: [SQL] Adding foreign key constraint on "version_release" for "version_id".
This DO block sets up the foreign key to"deployment_version", ensuring integrity between version releases and deployment versions.
77-81: [SQL] Adding foreign key constraint on "release" for "version_release_id".
This constraint adds a necessary relationship between the"release"and"version_release"tables. The use of exception handling helps maintain idempotency in migrations.
83-87: [SQL] Adding foreign key constraint on "release" for "variable_release_id".
The foreign key is set up to link the"release"table with"variable_set_release", aligning with the new relational design.
89-91: [SQL] Dropping obsolete columns from "release" and "release_job".
Removing"release_target_id"and"version_id"from the"release"table, and"created_at"from"release_job", cleans up legacy elements of the schema. Ensure that any dependencies on these columns are properly migrated.packages/db/drizzle/meta/_journal.json (1)
599-606: [JSON] Adding new journal entry for migration "0085_sloppy_mister_fear".
The new entry withidx: 85 is correctly formatted and integrated into the journal. Verify that the timestamp (when) and tag values accurately reflect the changes in this migration.
Summary by CodeRabbit
New Features
VariableReleaseManagerclass for managing variable releases.VersionReleaseManagerclass to handle version releases and related policies.VersionRuleEnginefor structured version filtering and selection based on rules.sensitivefield in thedeploymentVariableValueandvariableSetValuetables to indicate sensitive information.Refactor
Bug Fixes
Chore
tailwind.config.jsto.gitignoreto prevent it from being tracked in version control.