chore: workspace engine variable manager #671
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughReplaces VariableManager.database(...) with a local getVariableManager(...) that composes ResourceVariableProvider and DeploymentVariableProvider; adds repositories and wiring for resource-relationship rules, an in-memory variable snapshot repo, a VariableManager.create API, and a secrets dependency. Changes
Sequence Diagram(s)sequenceDiagram
actor Evaluator as ReleaseTarget Evaluator
participant VRM as variable-release-manager
participant GVM as getVariableManager()
participant Repo as Workspace.repository
participant DB as DB
participant VM as VariableManager
participant RVP as ResourceVariableProvider
participant DVP as DeploymentVariableProvider
Evaluator->>VRM: evaluate(...)
VRM->>GVM: getVariableManager(workspace, releaseTarget)
GVM->>Repo: deploymentVariableRepository.getAll()
Repo->>DB: query deployment variables (workspace)
DB-->>Repo: deployment variables
GVM->>RVP: new ResourceVariableProvider(...)
GVM->>DVP: new DeploymentVariableProvider(...)
GVM->>VM: VariableManager.create({keys}, [RVP, DVP])
VM-->>VRM: VariableManager
note right of DVP: DVP resolves reference variables via rule repositories,\nresource queries and may decrypt sensitive values
VRM->>VM: resolve(key)
VM->>RVP: getVariable(key) (provider 1)
RVP->>Repo: resourceVariableRepository.getAll()
Repo->>DB: fetch resource variables
DB-->>RVP: values
VM->>DVP: getVariable(key) (provider 2)
DVP->>Repo: rule/resource/metadata queries
Repo->>DB: queries
DB-->>DVP: rule & resources
DVP-->>VM: resolved value
VM-->>VRM: final value(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.ts (1)
12-28: Avoid refetching all resource variables on every lookupLine 27 triggers
getVariables(), which re-queries and filters the entire resource-variable set for each key requested by theVariableManager. During evaluation we iterate over every deployment variable key, so this turns into N identical loads. Cache the filtered list (or memoize it per instance) so we only hit the repository once per evaluation run.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
apps/event-queue/src/release-targets/evaluate/variable-release-manager.ts(2 hunks)apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts(1 hunks)apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.ts(1 hunks)apps/event-queue/src/release-targets/evaluate/variables/variable-manager.ts(1 hunks)apps/event-queue/src/repository/db-resource-relationship-rule-metadata-match-repository.ts(1 hunks)apps/event-queue/src/repository/db-resource-relationship-rule-repository.ts(1 hunks)apps/event-queue/src/repository/db-resource-relationship-rule-source-metadata-equals-repository.ts(1 hunks)apps/event-queue/src/repository/db-resource-relationship-rule-target-metadata-equals-repository.ts(1 hunks)apps/event-queue/src/repository/repository.ts(2 hunks)apps/event-queue/src/workspace/workspace.ts(2 hunks)packages/db/src/schema/resource-relationship-rule.ts(1 hunks)packages/rule-engine/src/manager/variables/variables.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.tspackages/db/src/schema/resource-relationship-rule.tsapps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.tspackages/rule-engine/src/manager/variables/variables.tsapps/event-queue/src/release-targets/evaluate/variables/variable-manager.tsapps/event-queue/src/release-targets/evaluate/variable-release-manager.tsapps/event-queue/src/repository/repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-target-metadata-equals-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-source-metadata-equals-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-metadata-match-repository.tsapps/event-queue/src/workspace/workspace.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.tspackages/db/src/schema/resource-relationship-rule.tsapps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.tspackages/rule-engine/src/manager/variables/variables.tsapps/event-queue/src/release-targets/evaluate/variables/variable-manager.tsapps/event-queue/src/release-targets/evaluate/variable-release-manager.tsapps/event-queue/src/repository/repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-target-metadata-equals-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-source-metadata-equals-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-metadata-match-repository.tsapps/event-queue/src/workspace/workspace.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.tspackages/db/src/schema/resource-relationship-rule.tsapps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.tspackages/rule-engine/src/manager/variables/variables.tsapps/event-queue/src/release-targets/evaluate/variables/variable-manager.tsapps/event-queue/src/release-targets/evaluate/variable-release-manager.tsapps/event-queue/src/repository/repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-target-metadata-equals-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-source-metadata-equals-repository.tsapps/event-queue/src/repository/db-resource-relationship-rule-metadata-match-repository.tsapps/event-queue/src/workspace/workspace.ts
🧬 Code graph analysis (11)
apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.ts (4)
packages/rule-engine/src/manager/variables/types.ts (2)
VariableProvider(11-13)MaybeVariable(9-9)apps/event-queue/src/workspace/workspace.ts (1)
Workspace(165-199)packages/events/src/kafka/events.ts (1)
FullReleaseTarget(66-70)packages/api/src/router/resource/resource-variables.ts (1)
resourceVariables(11-138)
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts (5)
packages/rule-engine/src/manager/variables/types.ts (2)
VariableProvider(11-13)MaybeVariable(9-9)packages/events/src/kafka/events.ts (2)
FullReleaseTarget(66-70)FullResource(62-64)packages/secrets/src/index.ts (1)
variablesAES256(50-51)packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValue(174-176)apps/event-queue/src/selector/in-memory/resource-match.ts (1)
resourceMatchesSelector(32-87)
packages/rule-engine/src/manager/variables/variables.ts (1)
packages/rule-engine/src/manager/variables/types.ts (1)
VariableProvider(11-13)
apps/event-queue/src/release-targets/evaluate/variables/variable-manager.ts (5)
apps/event-queue/src/workspace/workspace.ts (1)
Workspace(165-199)packages/events/src/kafka/events.ts (1)
FullReleaseTarget(66-70)apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.ts (1)
ResourceVariableProvider(6-37)apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts (1)
DeploymentVariableProvider(11-254)packages/rule-engine/src/manager/variables/variables.ts (1)
VariableManager(31-75)
apps/event-queue/src/release-targets/evaluate/variable-release-manager.ts (1)
apps/event-queue/src/release-targets/evaluate/variables/variable-manager.ts (1)
getVariableManager(9-32)
apps/event-queue/src/repository/repository.ts (1)
packages/db/src/schema/resource-relationship-rule.ts (4)
ResourceRelationshipRule(210-211)ResourceRelationshipRuleTargetMetadataEquals(216-217)ResourceRelationshipRuleSourceMetadataEquals(218-219)ResourceRelationshipRuleMetadataMatch(212-213)
apps/event-queue/src/repository/db-resource-relationship-rule-repository.ts (3)
apps/event-queue/src/repository/repository.ts (1)
Repository(8-15)packages/db/src/schema/resource-relationship-rule.ts (1)
ResourceRelationshipRule(210-211)packages/db/src/common.ts (3)
Tx(22-22)takeFirstOrNull(15-20)takeFirst(9-13)
apps/event-queue/src/repository/db-resource-relationship-rule-target-metadata-equals-repository.ts (3)
apps/event-queue/src/repository/repository.ts (1)
Repository(8-15)packages/db/src/schema/resource-relationship-rule.ts (1)
ResourceRelationshipRuleTargetMetadataEquals(216-217)packages/db/src/common.ts (3)
Tx(22-22)takeFirstOrNull(15-20)takeFirst(9-13)
apps/event-queue/src/repository/db-resource-relationship-rule-source-metadata-equals-repository.ts (3)
apps/event-queue/src/repository/repository.ts (1)
Repository(8-15)packages/db/src/schema/resource-relationship-rule.ts (1)
ResourceRelationshipRuleSourceMetadataEquals(218-219)packages/db/src/common.ts (3)
Tx(22-22)takeFirstOrNull(15-20)takeFirst(9-13)
apps/event-queue/src/repository/db-resource-relationship-rule-metadata-match-repository.ts (3)
apps/event-queue/src/repository/repository.ts (1)
Repository(8-15)packages/db/src/schema/resource-relationship-rule.ts (1)
ResourceRelationshipRuleMetadataMatch(212-213)packages/db/src/common.ts (3)
Tx(22-22)takeFirstOrNull(15-20)takeFirst(9-13)
apps/event-queue/src/workspace/workspace.ts (4)
apps/event-queue/src/repository/db-resource-relationship-rule-repository.ts (1)
DbResourceRelationshipRuleRepository(9-72)apps/event-queue/src/repository/db-resource-relationship-rule-target-metadata-equals-repository.ts (1)
DbResourceRelationshipRuleTargetMetadataEqualsRepository(9-82)apps/event-queue/src/repository/db-resource-relationship-rule-source-metadata-equals-repository.ts (1)
DbResourceRelationshipRuleSourceMetadataEqualsRepository(9-82)apps/event-queue/src/repository/db-resource-relationship-rule-metadata-match-repository.ts (1)
DbResourceRelationshipRuleMetadataMatchRepository(9-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
apps/event-queue/src/repository/db-resource-relationship-rule-metadata-match-repository.ts (1)
19-77: Enforce workspace scoping on CRUD operationsAll of
get,update,delete, andexistsfilter solely byid, so any caller with a UUID can reach across workspaces and read or mutate another tenant’s metadata match row. That breaks our multi-tenant isolation guarantees and is a blocker. Please scope every operation tothis.workspaceId, just likegetAllalready does, e.g. by joining toresourceRelationshipRule(for reads) and constraining updates/deletes with a workspace-bound subquery. A concrete patch would look like:-import { eq, takeFirst, takeFirstOrNull } from "@ctrlplane/db"; +import { and, eq, inArray, takeFirst, takeFirstOrNull } from "@ctrlplane/db"; @@ - get(id: string) { - return this.db - .select() - .from(schema.resourceRelationshipRuleMetadataMatch) - .where(eq(schema.resourceRelationshipRuleMetadataMatch.id, id)) - .then(takeFirstOrNull); - } + get(id: string) { + return this.db + .select() + .from(schema.resourceRelationshipRuleMetadataMatch) + .innerJoin( + schema.resourceRelationshipRule, + eq( + schema.resourceRelationshipRuleMetadataMatch.resourceRelationshipRuleId, + schema.resourceRelationshipRule.id, + ), + ) + .where( + and( + eq(schema.resourceRelationshipRuleMetadataMatch.id, id), + eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId), + ), + ) + .then(takeFirstOrNull) + .then((row) => row?.resource_relationship_rule_metadata_match ?? null); + } @@ - update(entity: schema.ResourceRelationshipRuleMetadataMatch) { + update(entity: schema.ResourceRelationshipRuleMetadataMatch) { return this.db .update(schema.resourceRelationshipRuleMetadataMatch) .set(entity) - .where(eq(schema.resourceRelationshipRuleMetadataMatch.id, entity.id)) + .where( + and( + eq(schema.resourceRelationshipRuleMetadataMatch.id, entity.id), + inArray( + schema.resourceRelationshipRuleMetadataMatch.resourceRelationshipRuleId, + this.db + .select({ id: schema.resourceRelationshipRule.id }) + .from(schema.resourceRelationshipRule) + .where( + eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId), + ), + ), + ), + ) .returning() .then(takeFirst); } @@ - delete(id: string) { + delete(id: string) { return this.db .delete(schema.resourceRelationshipRuleMetadataMatch) - .where(eq(schema.resourceRelationshipRuleMetadataMatch.id, id)) + .where( + and( + eq(schema.resourceRelationshipRuleMetadataMatch.id, id), + inArray( + schema.resourceRelationshipRuleMetadataMatch.resourceRelationshipRuleId, + this.db + .select({ id: schema.resourceRelationshipRule.id }) + .from(schema.resourceRelationshipRule) + .where( + eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId), + ), + ), + ), + ) .returning() .then(takeFirstOrNull); } @@ - exists(id: string) { + exists(id: string) { return this.db .select() - .from(schema.resourceRelationshipRuleMetadataMatch) - .where(eq(schema.resourceRelationshipRuleMetadataMatch.id, id)) + .from(schema.resourceRelationshipRuleMetadataMatch) + .innerJoin( + schema.resourceRelationshipRule, + eq( + schema.resourceRelationshipRuleMetadataMatch.resourceRelationshipRuleId, + schema.resourceRelationshipRule.id, + ), + ) + .where( + and( + eq(schema.resourceRelationshipRuleMetadataMatch.id, id), + eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId), + ), + ) .then(takeFirstOrNull) .then((r) => r != null); }Apply the same pattern anywhere else we expose workspace-scoped repositories.
⛔ Skipped due to learnings
Learnt from: adityachoudhari26 PR: ctrlplanedev/ctrlplane#664 File: apps/event-queue/src/repository/db-resource-repository.ts:42-49 Timestamp: 2025-08-26T20:10:10.062Z Learning: In the ctrlplane codebase, resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping in WHERE clauses is not necessary when querying by ID.
Comment on lines
+28
to
+60
| private async getRuleByReference(reference: string) { | ||
| const allRelationshipRules = | ||
| await this.workspace.repository.resourceRelationshipRuleRepository.getAll(); | ||
| const relationshipRule = allRelationshipRules.find( | ||
| (r) => r.reference === reference, | ||
| ); | ||
| if (relationshipRule == null) return null; | ||
| const [ | ||
| allRelationshipRuleSourceMetadataEquals, | ||
| allRelationshipRuleTargetMetadataEquals, | ||
| allRelationshipRuleMetadataMatch, | ||
| ] = await Promise.all([ | ||
| this.workspace.repository.resourceRelationshipRuleSourceMetadataEqualsRepository.getAll(), | ||
| this.workspace.repository.resourceRelationshipRuleTargetMetadataEqualsRepository.getAll(), | ||
| this.workspace.repository.resourceRelationshipRuleMetadataMatchRepository.getAll(), | ||
| ]); | ||
|
|
||
| const metadataKeysMatch = allRelationshipRuleMetadataMatch.filter( | ||
| (r) => r.resourceRelationshipRuleId === relationshipRule.id, | ||
| ); | ||
| const targetMetadataEquals = allRelationshipRuleTargetMetadataEquals.filter( | ||
| (r) => r.resourceRelationshipRuleId === relationshipRule.id, | ||
| ); | ||
| const sourceMetadataEquals = allRelationshipRuleSourceMetadataEquals.filter( | ||
| (r) => r.resourceRelationshipRuleId === relationshipRule.id, | ||
| ); | ||
| return { | ||
| ...relationshipRule, | ||
| metadataKeysMatch, | ||
| targetMetadataEquals, | ||
| sourceMetadataEquals, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Scope DB queries down to rule reference
getRuleByReference pulls entire rule and metadata tables into memory before filtering, which will not scale when the tables grow. We need repository helpers that load a single rule (and its metadata) by reference to keep evaluation performant.
🤖 Prompt for AI Agents
In
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
around lines 28 to 60, the current implementation loads entire relationship rule
and metadata tables then filters in-memory which won't scale; replace these
broad queries with repository helpers that fetch a single relationship rule by
reference and the associated metadata rows by resourceRelationshipRuleId. Add or
use repository methods such as getByReference(reference) on
resourceRelationshipRuleRepository and getByRuleId(ruleId) (or similarly named)
on the three metadata repositories, call them (Promise.all is fine) to load only
the needed rows, and return the same object shape ({ ...relationshipRule,
metadataKeysMatch, targetMetadataEquals, sourceMetadataEquals }). Ensure null
handling remains the same when the rule isn't found.
Contributor
Author
There was a problem hiding this comment.
right now this is not really an issue. if we have issues with memory then we'll fix it then.
Comment on lines
+82
to
+141
| private async getSourceResourceCandidates( | ||
| relationshipRule: schema.ResourceRelationshipRule, | ||
| ) { | ||
| const { sourceKind, sourceVersion } = relationshipRule; | ||
| const allResources = | ||
| await this.workspace.repository.resourceRepository.getAll(); | ||
| return allResources.filter( | ||
| (r) => | ||
| r.kind === sourceKind && | ||
| r.version === sourceVersion && | ||
| r.deletedAt == null, | ||
| ); | ||
| } | ||
|
|
||
| private sourceResourceMatchesRule( | ||
| relationshipRule: schema.ResourceRelationshipRule, | ||
| sourceMetadataEqualsRules: schema.ResourceRelationshipRuleSourceMetadataEquals[], | ||
| metadataKeysMatchRules: schema.ResourceRelationshipRuleMetadataMatch[], | ||
| candidate: FullResource, | ||
| ) { | ||
| const { resource } = this.releaseTarget; | ||
| for (const s of sourceMetadataEqualsRules) { | ||
| const sourceMetadata = candidate.metadata[s.key]; | ||
| if (sourceMetadata == null || sourceMetadata !== s.value) return false; | ||
| } | ||
|
|
||
| for (const m of metadataKeysMatchRules) { | ||
| const sourceMetadata = candidate.metadata[m.sourceKey]; | ||
| const targetMetadata = resource.metadata[m.targetKey]; | ||
| if ( | ||
| sourceMetadata == null || | ||
| targetMetadata == null || | ||
| sourceMetadata !== targetMetadata | ||
| ) | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| private async getFullSource( | ||
| resource: FullResource, | ||
| ): Promise< | ||
| FullResource & { variables: Record<string, string | number | boolean> } | ||
| > { | ||
| const allVariables = | ||
| await this.workspace.repository.resourceVariableRepository.getAll(); | ||
| const variables = Object.fromEntries( | ||
| allVariables | ||
| .filter((v) => v.resourceId === resource.id) | ||
| .filter((v) => v.valueType === "direct") | ||
| .map((v) => { | ||
| const { value, key } = v; | ||
| if (v.sensitive) | ||
| return [key, variablesAES256().decrypt(String(value))]; | ||
| if (typeof value === "object") return [key, JSON.stringify(value)]; | ||
| return [key, value]; | ||
| }), | ||
| ); | ||
| return { ...resource, variables }; |
Contributor
There was a problem hiding this comment.
Narrow resource/variable lookups
Both getSourceResourceCandidates and getFullSource fetch all resources/variables and then filter locally. This is expensive and risks timeouts. Please add targeted repository queries (e.g., listByKindVersion, listByResourceIdAndType) to avoid pulling entire tables for each resolution.
Contributor
Author
There was a problem hiding this comment.
same as above, if performance becomes an actual issue we can change it then
Comment on lines
+160
to
+171
| const sourceResource = sourceResourceCandidates.find((r) => | ||
| this.sourceResourceMatchesRule( | ||
| relationshipRule, | ||
| sourceMetadataEquals, | ||
| metadataKeysMatch, | ||
| r, | ||
| ), | ||
| ); | ||
| if (sourceResource == null) return defaultValue; | ||
|
|
||
| const fullSource = await this.getFullSource(sourceResource); | ||
| const resolvedPath = _.get(fullSource, path, defaultValue); |
Contributor
There was a problem hiding this comment.
Load full source resource before filtering
sourceResourceCandidates are still partial resources (metadata not populated), so attempting to read candidate.metadata is unsafe and will throw once metadata isn’t eagerly joined. Ensure candidates are hydrated with metadata (e.g., via a repository method that returns FullResource) before evaluating metadata rules.
🤖 Prompt for AI Agents
In
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
around lines 160-171, the code calls sourceResourceMatchesRule against partial
sourceResourceCandidates (whose metadata may be undefined) then only hydrates
the chosen candidate, which will throw when metadata is not eagerly joined; fix
by hydrating candidates before applying the rule: map sourceResourceCandidates
to full resources (await this.getFullSource for each candidate or call a
repository method that returns FullResource), then run sourceResourceMatchesRule
against those fully populated resources (or adapt the match function to
accept/guard missing metadata), and finally select the resolved resource and
read the path from the hydrated full object.
Contributor
Author
There was a problem hiding this comment.
the resource repository provides FullResource, which has a metadata map attached to it
Comment on lines
+211
to
+228
| private async getDeploymentVariable(key: string) { | ||
| const allDeploymentVariables = | ||
| await this.workspace.repository.deploymentVariableRepository.getAll(); | ||
| const deploymentVariable = allDeploymentVariables.find( | ||
| (v) => | ||
| v.deploymentId === this.releaseTarget.deploymentId && v.key === key, | ||
| ); | ||
| if (deploymentVariable == null) return null; | ||
| const allDeploymentVariableValues = | ||
| await this.workspace.repository.deploymentVariableValueRepository.getAll(); | ||
| const values = allDeploymentVariableValues.filter( | ||
| (v) => v.variableId === deploymentVariable.id, | ||
| ); | ||
| const defaultValue = values.find( | ||
| (value) => value.id === deploymentVariable.defaultValueId, | ||
| ); | ||
| return { ...deploymentVariable, values, defaultValue }; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid repeated full-table scans in hot path
getDeploymentVariable issues two getAll() calls and filters client-side. Under load this will hammer the database with full-table scans per variable lookup. Please introduce repository methods (e.g., getByDeploymentIdAndKey, getValuesByVariableId) so we fetch only the relevant rows.
🤖 Prompt for AI Agents
In
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
around lines 211-228, the method getDeploymentVariable performs two
repository.getAll() calls and filters in-memory, causing full-table scans;
replace these with targeted repository methods (e.g., add
repository.deploymentVariableRepository.getByDeploymentIdAndKey(deploymentId,
key) to fetch the single variable, and
repository.deploymentVariableValueRepository.getValuesByVariableId(variableId)
to fetch only its values) and update getDeploymentVariable to call those new
methods and compute defaultValue from the returned values.
Contributor
Author
There was a problem hiding this comment.
same as above comments, not worried about performance right now. remember, these are partitioned to workspace. even thousands shoudl not be an issue at all
Comment on lines
26
to
35
| async getVariable(key: string): Promise<MaybeVariable> { | ||
| const variables = await this.getVariables(); | ||
| const variable = variables.find((v) => v.key === key) ?? null; | ||
| if (variable == null) return null; | ||
| return { | ||
| id: variable.id, | ||
| key, | ||
| value: variable.value, | ||
| sensitive: variable.sensitive, | ||
| }; |
Contributor
There was a problem hiding this comment.
Decrypt sensitive resource variable values before emitting them
Line 32 returns variable.value as-is. Resource variables marked sensitive are stored encrypted (see packages/api/src/router/resource/resource-variables.ts), and the old DatabaseResourceVariableProvider decrypted them before handing them to the release flow. With this direct return, every sensitive value will surface as ciphertext, breaking consumers of the release payload. Mirror the decryption logic used on the write path (e.g., variablesAES256().decrypt(...)) before constructing the MaybeVariable.
🤖 Prompt for AI Agents
In
apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.ts
around lines 26 to 35, the code returns variable.value directly which leaves
sensitive resource variables encrypted; decrypt sensitive values before emitting
them by calling the same decryption used on write (e.g.,
variablesAES256().decrypt(variable.value)) when variable.sensitive is true, and
return the decrypted plaintext in the MaybeVariable.value; ensure you handle
null/undefined values and any decryption errors (bubble or log) consistently
with existing providers.
Comment on lines
+28
to
+31
| return VariableManager.create({ keys }, [ | ||
| resourceVariableProvider, | ||
| deploymentVariableProvider, | ||
| ]); |
Contributor
There was a problem hiding this comment.
Reintroduce the system variable provider to keep parity with the database flow
Lines 28-31 now instantiate only the resource and deployment providers. The previous wiring via VariableManager.database(...) always included DatabaseSystemVariableSetProvider, so system-scoped variables (e.g. environment/workspace defaults) will now come back as null. Please restore an equivalent provider in this stack so those values continue to resolve.
🤖 Prompt for AI Agents
In apps/event-queue/src/release-targets/evaluate/variables/variable-manager.ts
around lines 28-31, the VariableManager.create call only registers resource and
deployment providers which omits the system-scoped provider and causes system
variables to resolve to null; restore the equivalent
DatabaseSystemVariableSetProvider (or the system variable provider used by
VariableManager.database(...)) by importing/instantiating it and adding it into
the provider array (in the same ordering as the database wiring) so
system-scoped variables (environment/workspace defaults) are resolved.
Contributor
Author
There was a problem hiding this comment.
not using the system variable provider at all, if we need it we will reintroduce it
Comment on lines
+56
to
+71
| delete(id: string) { | ||
| return this.db | ||
| .delete(schema.resourceRelationshipRule) | ||
| .where(eq(schema.resourceRelationshipRule.id, id)) | ||
| .returning() | ||
| .then(takeFirstOrNull); | ||
| } | ||
|
|
||
| exists(id: string) { | ||
| return this.db | ||
| .select() | ||
| .from(schema.resourceRelationshipRule) | ||
| .where(eq(schema.resourceRelationshipRule.id, id)) | ||
| .then(takeFirstOrNull) | ||
| .then((r) => r != null); | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce workspace scoping on update/delete/exists
update, delete, and exists don’t filter by workspaceId, so a workspace can mutate or observe rules belonging to another workspace. Add the workspace predicate to each query to maintain tenant isolation.
🤖 Prompt for AI Agents
In apps/event-queue/src/repository/db-resource-relationship-rule-repository.ts
around lines 56 to 71, delete, exists (and the update method) currently only
filter by id and therefore allow cross-workspace access; add the workspace
predicate to each query by Constraining queries with
eq(schema.resourceRelationshipRule.workspaceId, workspaceId) alongside the id
predicate (combined via AND), and ensure each method accepts or uses the
workspaceId parameter from the caller/context so update/delete/exists only
operate on rows matching both id and workspaceId.
Comment on lines
+55
to
+80
| update(entity: schema.ResourceRelationshipRuleSourceMetadataEquals) { | ||
| return this.db | ||
| .update(schema.resourceRelationshipSourceRuleMetadataEquals) | ||
| .set(entity) | ||
| .where( | ||
| eq(schema.resourceRelationshipSourceRuleMetadataEquals.id, entity.id), | ||
| ) | ||
| .returning() | ||
| .then(takeFirst); | ||
| } | ||
|
|
||
| delete(id: string) { | ||
| return this.db | ||
| .delete(schema.resourceRelationshipSourceRuleMetadataEquals) | ||
| .where(eq(schema.resourceRelationshipSourceRuleMetadataEquals.id, id)) | ||
| .returning() | ||
| .then(takeFirstOrNull); | ||
| } | ||
|
|
||
| exists(id: string) { | ||
| return this.db | ||
| .select() | ||
| .from(schema.resourceRelationshipSourceRuleMetadataEquals) | ||
| .where(eq(schema.resourceRelationshipSourceRuleMetadataEquals.id, id)) | ||
| .then(takeFirstOrNull) | ||
| .then((r) => r != null); |
Contributor
There was a problem hiding this comment.
Workspace filter missing on mutating ops
The update, delete, and exists methods lack the workspace constraint; a caller could touch metadata rows for other workspaces. Mirror the join/where logic used in getAll when mutating/checking existence.
🤖 Prompt for AI Agents
In
apps/event-queue/src/repository/db-resource-relationship-rule-source-metadata-equals-repository.ts
around lines 55 to 80, the update, delete, and exists methods do not constrain
operations to a workspace and therefore can affect rows from other workspaces;
modify each method to accept the workspaceId (or use the existing
caller-provided workspaceId) and apply the same join/where logic used in getAll:
join resourceRelationshipSourceRuleMetadataEquals -> resourceRelationshipRule ->
resourceRelationship and add a where clause that filters by
eq(resourceRelationship.workspaceId, workspaceId) (keeping the existing id
equality), then perform the update/delete/select and return the same
returning/takeFirst or takeFirstOrNull results as before.
Comment on lines
+19
to
+81
| get(id: string) { | ||
| return this.db | ||
| .select() | ||
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | ||
| .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id)) | ||
| .then(takeFirstOrNull); | ||
| } | ||
|
|
||
| getAll() { | ||
| return this.db | ||
| .select() | ||
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | ||
| .innerJoin( | ||
| schema.resourceRelationshipRule, | ||
| eq( | ||
| schema.resourceRelationshipTargetRuleMetadataEquals | ||
| .resourceRelationshipRuleId, | ||
| schema.resourceRelationshipRule.id, | ||
| ), | ||
| ) | ||
| .where(eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId)) | ||
| .then((rows) => | ||
| rows.map( | ||
| (row) => row.resource_relationship_rule_target_metadata_equals, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| create(entity: schema.ResourceRelationshipRuleTargetMetadataEquals) { | ||
| return this.db | ||
| .insert(schema.resourceRelationshipTargetRuleMetadataEquals) | ||
| .values(entity) | ||
| .returning() | ||
| .then(takeFirst); | ||
| } | ||
|
|
||
| update(entity: schema.ResourceRelationshipRuleTargetMetadataEquals) { | ||
| return this.db | ||
| .update(schema.resourceRelationshipTargetRuleMetadataEquals) | ||
| .set(entity) | ||
| .where( | ||
| eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, entity.id), | ||
| ) | ||
| .returning() | ||
| .then(takeFirst); | ||
| } | ||
|
|
||
| delete(id: string) { | ||
| return this.db | ||
| .delete(schema.resourceRelationshipTargetRuleMetadataEquals) | ||
| .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id)) | ||
| .returning() | ||
| .then(takeFirstOrNull); | ||
| } | ||
|
|
||
| exists(id: string) { | ||
| return this.db | ||
| .select() | ||
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | ||
| .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id)) | ||
| .then(takeFirstOrNull) | ||
| .then((r) => r != null); | ||
| } |
Contributor
There was a problem hiding this comment.
Lock target-metadata-equals operations to the current workspace
Similar to the metadata-match repository, these CRUD methods look up rows only by id. That lets a caller operate on another workspace’s relationships if they know the UUID, breaking tenant isolation. Please constrain every query/update/delete to this.workspaceId (e.g. via a join against resourceRelationshipRule or a workspace-bounded subquery). One way to address it:
-import { eq, takeFirst, takeFirstOrNull } from "@ctrlplane/db";
+import { and, eq, inArray, takeFirst, takeFirstOrNull } from "@ctrlplane/db";
@@
- get(id: string) {
- return this.db
- .select()
- .from(schema.resourceRelationshipTargetRuleMetadataEquals)
- .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id))
- .then(takeFirstOrNull);
- }
+ get(id: string) {
+ return this.db
+ .select()
+ .from(schema.resourceRelationshipTargetRuleMetadataEquals)
+ .innerJoin(
+ schema.resourceRelationshipRule,
+ eq(
+ schema.resourceRelationshipTargetRuleMetadataEquals
+ .resourceRelationshipRuleId,
+ schema.resourceRelationshipRule.id,
+ ),
+ )
+ .where(
+ and(
+ eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id),
+ eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId),
+ ),
+ )
+ .then(takeFirstOrNull)
+ .then(
+ (row) => row?.resource_relationship_rule_target_metadata_equals ?? null,
+ );
+ }
@@
- update(entity: schema.ResourceRelationshipRuleTargetMetadataEquals) {
+ update(entity: schema.ResourceRelationshipRuleTargetMetadataEquals) {
return this.db
.update(schema.resourceRelationshipTargetRuleMetadataEquals)
.set(entity)
- .where(
- eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, entity.id),
- )
+ .where(
+ and(
+ eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, entity.id),
+ inArray(
+ schema.resourceRelationshipTargetRuleMetadataEquals
+ .resourceRelationshipRuleId,
+ this.db
+ .select({ id: schema.resourceRelationshipRule.id })
+ .from(schema.resourceRelationshipRule)
+ .where(
+ eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId),
+ ),
+ ),
+ ),
+ )
.returning()
.then(takeFirst);
}
@@
- delete(id: string) {
+ delete(id: string) {
return this.db
.delete(schema.resourceRelationshipTargetRuleMetadataEquals)
- .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id))
+ .where(
+ and(
+ eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id),
+ inArray(
+ schema.resourceRelationshipTargetRuleMetadataEquals
+ .resourceRelationshipRuleId,
+ this.db
+ .select({ id: schema.resourceRelationshipRule.id })
+ .from(schema.resourceRelationshipRule)
+ .where(
+ eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId),
+ ),
+ ),
+ ),
+ )
.returning()
.then(takeFirstOrNull);
}
@@
- exists(id: string) {
+ exists(id: string) {
return this.db
.select()
- .from(schema.resourceRelationshipTargetRuleMetadataEquals)
- .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id))
+ .from(schema.resourceRelationshipTargetRuleMetadataEquals)
+ .innerJoin(
+ schema.resourceRelationshipRule,
+ eq(
+ schema.resourceRelationshipTargetRuleMetadataEquals
+ .resourceRelationshipRuleId,
+ schema.resourceRelationshipRule.id,
+ ),
+ )
+ .where(
+ and(
+ eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id),
+ eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId),
+ ),
+ )
.then(takeFirstOrNull)
.then((r) => r != null);
}Please apply the same workspace guardrails to any companion repositories to keep the isolation invariant intact.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| get(id: string) { | |
| return this.db | |
| .select() | |
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id)) | |
| .then(takeFirstOrNull); | |
| } | |
| getAll() { | |
| return this.db | |
| .select() | |
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .innerJoin( | |
| schema.resourceRelationshipRule, | |
| eq( | |
| schema.resourceRelationshipTargetRuleMetadataEquals | |
| .resourceRelationshipRuleId, | |
| schema.resourceRelationshipRule.id, | |
| ), | |
| ) | |
| .where(eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId)) | |
| .then((rows) => | |
| rows.map( | |
| (row) => row.resource_relationship_rule_target_metadata_equals, | |
| ), | |
| ); | |
| } | |
| create(entity: schema.ResourceRelationshipRuleTargetMetadataEquals) { | |
| return this.db | |
| .insert(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .values(entity) | |
| .returning() | |
| .then(takeFirst); | |
| } | |
| update(entity: schema.ResourceRelationshipRuleTargetMetadataEquals) { | |
| return this.db | |
| .update(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .set(entity) | |
| .where( | |
| eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, entity.id), | |
| ) | |
| .returning() | |
| .then(takeFirst); | |
| } | |
| delete(id: string) { | |
| return this.db | |
| .delete(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id)) | |
| .returning() | |
| .then(takeFirstOrNull); | |
| } | |
| exists(id: string) { | |
| return this.db | |
| .select() | |
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .where(eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id)) | |
| .then(takeFirstOrNull) | |
| .then((r) => r != null); | |
| } | |
| // at the top of the file | |
| import { and, eq, inArray, takeFirst, takeFirstOrNull } from "@ctrlplane/db"; | |
| get(id: string) { | |
| return this.db | |
| .select() | |
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .innerJoin( | |
| schema.resourceRelationshipRule, | |
| eq( | |
| schema.resourceRelationshipTargetRuleMetadataEquals | |
| .resourceRelationshipRuleId, | |
| schema.resourceRelationshipRule.id, | |
| ), | |
| ) | |
| .where( | |
| and( | |
| eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id), | |
| eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId), | |
| ), | |
| ) | |
| .then(takeFirstOrNull) | |
| .then( | |
| (row) => row?.resource_relationship_rule_target_metadata_equals ?? null, | |
| ); | |
| } | |
| getAll() { | |
| return this.db | |
| .select() | |
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .innerJoin( | |
| schema.resourceRelationshipRule, | |
| eq( | |
| schema.resourceRelationshipTargetRuleMetadataEquals | |
| .resourceRelationshipRuleId, | |
| schema.resourceRelationshipRule.id, | |
| ), | |
| ) | |
| .where(eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId)) | |
| .then((rows) => | |
| rows.map( | |
| (row) => row.resource_relationship_rule_target_metadata_equals, | |
| ), | |
| ); | |
| } | |
| create(entity: schema.ResourceRelationshipRuleTargetMetadataEquals) { | |
| return this.db | |
| .insert(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .values(entity) | |
| .returning() | |
| .then(takeFirst); | |
| } | |
| update(entity: schema.ResourceRelationshipRuleTargetMetadataEquals) { | |
| return this.db | |
| .update(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .set(entity) | |
| .where( | |
| and( | |
| eq( | |
| schema.resourceRelationshipTargetRuleMetadataEquals.id, | |
| entity.id, | |
| ), | |
| inArray( | |
| schema.resourceRelationshipTargetRuleMetadataEquals | |
| .resourceRelationshipRuleId, | |
| this.db | |
| .select({ id: schema.resourceRelationshipRule.id }) | |
| .from(schema.resourceRelationshipRule) | |
| .where( | |
| eq( | |
| schema.resourceRelationshipRule.workspaceId, | |
| this.workspaceId, | |
| ), | |
| ), | |
| ), | |
| ), | |
| ) | |
| .returning() | |
| .then(takeFirst); | |
| } | |
| delete(id: string) { | |
| return this.db | |
| .delete(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .where( | |
| and( | |
| eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id), | |
| inArray( | |
| schema.resourceRelationshipTargetRuleMetadataEquals | |
| .resourceRelationshipRuleId, | |
| this.db | |
| .select({ id: schema.resourceRelationshipRule.id }) | |
| .from(schema.resourceRelationshipRule) | |
| .where( | |
| eq( | |
| schema.resourceRelationshipRule.workspaceId, | |
| this.workspaceId, | |
| ), | |
| ), | |
| ), | |
| ), | |
| ) | |
| .returning() | |
| .then(takeFirstOrNull); | |
| } | |
| exists(id: string) { | |
| return this.db | |
| .select() | |
| .from(schema.resourceRelationshipTargetRuleMetadataEquals) | |
| .innerJoin( | |
| schema.resourceRelationshipRule, | |
| eq( | |
| schema.resourceRelationshipTargetRuleMetadataEquals | |
| .resourceRelationshipRuleId, | |
| schema.resourceRelationshipRule.id, | |
| ), | |
| ) | |
| .where( | |
| and( | |
| eq(schema.resourceRelationshipTargetRuleMetadataEquals.id, id), | |
| eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId), | |
| ), | |
| ) | |
| .then(takeFirstOrNull) | |
| .then((r) => r != null); | |
| } |
🤖 Prompt for AI Agents
In
apps/event-queue/src/repository/db-resource-relationship-rule-target-metadata-equals-repository.ts
lines 19-81: the CRUD methods only filter by id which allows cross-workspace
access; update every read/update/delete/exists query to innerJoin
schema.resourceRelationshipRule and add
.where(eq(schema.resourceRelationshipRule.workspaceId, this.workspaceId)) so the
row is scoped to the current workspace (getAll already does this); for update
and delete ensure the same join+workspace where is used before returning the
row; for create, perform a quick existence check that the referenced
resourceRelationshipRuleId exists and belongs to this.workspaceId (select from
schema.resourceRelationshipRule where id = entity.resourceRelationshipRuleId and
workspaceId = this.workspaceId) and fail/throw if not, then insert; ensure all
returned rows still map to resource_relationship_rule_target_metadata_equals
where appropriate.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/event-queue/package.json(1 hunks)apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts(1 hunks)apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/event-queue/src/release-targets/evaluate/variables/resource-variable-provider.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/event-queue/package.jsonapps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
🧠 Learnings (2)
📚 Learning: 2025-06-01T19:10:47.122Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/rule-engine/src/rules/concurrency-rule.ts:8-11
Timestamp: 2025-06-01T19:10:47.122Z
Learning: In packages/rule-engine/src/rules/concurrency-rule.ts, the ConcurrencyRule should remain simple without additional validation since database and Zod schemas already handle concurrency validation. The user prefers this rule to be "dumb" and just perform the comparison check rather than duplicating validation logic.
Applied to files:
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
📚 Learning: 2024-10-29T02:04:50.312Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Applied to files:
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
🧬 Code graph analysis (1)
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts (7)
packages/rule-engine/src/manager/variables/types.ts (2)
VariableProvider(11-13)MaybeVariable(9-9)apps/event-queue/src/workspace/workspace.ts (1)
Workspace(165-199)packages/events/src/kafka/events.ts (2)
FullReleaseTarget(66-70)FullResource(62-64)packages/secrets/src/index.ts (1)
variablesAES256(50-51)packages/db/src/schema/resource-relationship-rule.ts (4)
ResourceRelationshipRule(210-211)ResourceRelationshipRuleTargetMetadataEquals(216-217)ResourceRelationshipRuleSourceMetadataEquals(218-219)ResourceRelationshipRuleMetadataMatch(212-213)packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValue(174-176)apps/event-queue/src/selector/in-memory/resource-match.ts (1)
resourceMatchesSelector(32-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
Comment on lines
+82
to
+93
| private async getSourceResourceCandidates( | ||
| relationshipRule: schema.ResourceRelationshipRule, | ||
| ) { | ||
| const { sourceKind, sourceVersion } = relationshipRule; | ||
| const allResources = | ||
| await this.workspace.repository.resourceRepository.getAll(); | ||
| return allResources.filter( | ||
| (r) => | ||
| r.kind === sourceKind && | ||
| r.version === sourceVersion && | ||
| r.deletedAt == null, | ||
| ); |
Contributor
There was a problem hiding this comment.
Fix source-kind/version matching logic
Rules that omit sourceKind and/or sourceVersion should act as wildcards, but the current filter requires strict equality, so those rules never match any resource. This breaks reference resolution whenever the rule leaves one of those fields null. Please treat nulls as “match any” before filtering.
- return allResources.filter(
- (r) =>
- r.kind === sourceKind &&
- r.version === sourceVersion &&
- r.deletedAt == null,
- );
+ return allResources.filter((r) => {
+ const kindMatches = sourceKind == null || r.kind === sourceKind;
+ const versionMatches =
+ sourceVersion == null || r.version === sourceVersion;
+ return kindMatches && versionMatches && r.deletedAt == null;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private async getSourceResourceCandidates( | |
| relationshipRule: schema.ResourceRelationshipRule, | |
| ) { | |
| const { sourceKind, sourceVersion } = relationshipRule; | |
| const allResources = | |
| await this.workspace.repository.resourceRepository.getAll(); | |
| return allResources.filter( | |
| (r) => | |
| r.kind === sourceKind && | |
| r.version === sourceVersion && | |
| r.deletedAt == null, | |
| ); | |
| private async getSourceResourceCandidates( | |
| relationshipRule: schema.ResourceRelationshipRule, | |
| ) { | |
| const { sourceKind, sourceVersion } = relationshipRule; | |
| const allResources = | |
| await this.workspace.repository.resourceRepository.getAll(); | |
| - return allResources.filter( | |
| - (r) => | |
| - r.kind === sourceKind && | |
| - r.version === sourceVersion && | |
| - r.deletedAt == null, | |
| return allResources.filter((r) => { | |
| const kindMatches = sourceKind == null || r.kind === sourceKind; | |
| const versionMatches = | |
| sourceVersion == null || r.version === sourceVersion; | |
| return kindMatches && versionMatches && r.deletedAt == null; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
around lines 82 to 93, the current filter uses strict equality for sourceKind
and sourceVersion so rules that leave those fields null never match; change the
predicate to treat null/undefined as wildcards by only checking r.kind ===
sourceKind when sourceKind is not null/undefined and only checking r.version ===
sourceVersion when sourceVersion is not null/undefined (e.g. use conditions like
(sourceKind == null || r.kind === sourceKind) and (sourceVersion == null ||
r.version === sourceVersion) while still filtering out deleted resources).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/event-queue/src/release-targets/evaluate/variable-release-manager.ts(4 hunks)apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts(1 hunks)apps/event-queue/src/repository/in-memory/variable-value-snapshot.ts(1 hunks)e2e/tests/api/resource-variables.spec.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/event-queue/src/release-targets/evaluate/variable-release-manager.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
e2e/tests/api/resource-variables.spec.tsapps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.tsapps/event-queue/src/repository/in-memory/variable-value-snapshot.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
e2e/tests/api/resource-variables.spec.tsapps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.tsapps/event-queue/src/repository/in-memory/variable-value-snapshot.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
e2e/tests/api/resource-variables.spec.tsapps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.tsapps/event-queue/src/repository/in-memory/variable-value-snapshot.ts
🧠 Learnings (3)
📚 Learning: 2025-06-01T19:10:47.122Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/rule-engine/src/rules/concurrency-rule.ts:8-11
Timestamp: 2025-06-01T19:10:47.122Z
Learning: In packages/rule-engine/src/rules/concurrency-rule.ts, the ConcurrencyRule should remain simple without additional validation since database and Zod schemas already handle concurrency validation. The user prefers this rule to be "dumb" and just perform the comparison check rather than duplicating validation logic.
Applied to files:
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
📚 Learning: 2024-10-29T02:04:50.312Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Applied to files:
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
📚 Learning: 2025-06-30T21:19:43.866Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#604
File: packages/rule-engine/src/manager/version-manager.ts:124-139
Timestamp: 2025-06-30T21:19:43.866Z
Learning: In packages/rule-engine/src/manager/version-manager.ts, the findDesiredVersion method should use takeFirst (not takeFirstOrNull) because if a desiredVersionId is set but the query fails to find exactly one matching version, it indicates a data integrity or configuration issue that should fail loudly rather than be handled silently.
Applied to files:
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
🧬 Code graph analysis (2)
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts (6)
packages/rule-engine/src/manager/variables/types.ts (2)
VariableProvider(11-13)MaybeVariable(9-9)apps/event-queue/src/workspace/workspace.ts (1)
Workspace(165-199)packages/events/src/kafka/events.ts (2)
FullReleaseTarget(66-70)FullResource(62-64)packages/secrets/src/index.ts (1)
variablesAES256(50-51)packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValue(174-176)apps/event-queue/src/selector/in-memory/resource-match.ts (1)
resourceMatchesSelector(32-87)
apps/event-queue/src/repository/in-memory/variable-value-snapshot.ts (1)
packages/db/src/common.ts (1)
Tx(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
e2e/tests/api/resource-variables.spec.ts (1)
175-176: Preserving slug casing looks correctVariable resolution now compares metadata and selectors using the exact slug segment that upstream systems emit. Dropping the redundant
.toLowerCase()keeps the test fixtures aligned with the production casing, avoiding false positives from lowercased keys. Thanks for syncing all occurrences.Also applies to: 274-275, 438-439, 608-610, 779-781, 910-912, 1097-1099
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts (1)
88-93: Handle optional source kind/version as wildcardsRules that leave
sourceKindorsourceVersionunset are supposed to match any resource, but the current filter insists on exact equality, so those rules never match and references silently fall back to defaults. Treat the nulls as wildcards while still excluding deleted resources.- return allResources.filter( - (r) => - r.kind === sourceKind && - r.version === sourceVersion && - r.deletedAt == null, - ); + return allResources.filter((r) => { + const kindMatches = sourceKind == null || r.kind === sourceKind; + const versionMatches = + sourceVersion == null || r.version === sourceVersion; + return kindMatches && versionMatches && r.deletedAt == null; + });
Comment on lines
+125
to
+140
| FullResource & { variables: Record<string, string | number | boolean> } | ||
| > { | ||
| const allVariables = | ||
| await this.workspace.repository.resourceVariableRepository.getAll(); | ||
| const variables = Object.fromEntries( | ||
| allVariables | ||
| .filter((v) => v.resourceId === resource.id) | ||
| .filter((v) => v.valueType === "direct") | ||
| .map((v) => { | ||
| const { value, key } = v; | ||
| if (v.sensitive) | ||
| return [key, variablesAES256().decrypt(String(value))]; | ||
| if (typeof value === "object") return [key, JSON.stringify(value)]; | ||
| return [key, value]; | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
Keep resource variable objects intact for path resolution
By stringifying non-sensitive JSON values, fullSource.variables loses its structure, so lookups like variables.foo.bar can never succeed and every reference falls back to the default. Return the JSON as-is (and parse decrypted values when possible) so nested paths keep working.
- ): Promise<
- FullResource & { variables: Record<string, string | number | boolean> }
- > {
+ ): Promise<FullResource & { variables: Record<string, unknown> }> {
const allVariables =
await this.workspace.repository.resourceVariableRepository.getAll();
const variables = Object.fromEntries(
allVariables
.filter((v) => v.resourceId === resource.id)
.filter((v) => v.valueType === "direct")
.map((v) => {
const { value, key } = v;
- if (v.sensitive)
- return [key, variablesAES256().decrypt(String(value))];
- if (typeof value === "object") return [key, JSON.stringify(value)];
- return [key, value];
+ if (v.sensitive) {
+ const decrypted = variablesAES256().decrypt(String(value));
+ try {
+ return [key, JSON.parse(decrypted)];
+ } catch {
+ return [key, decrypted];
+ }
+ }
+ return [key, value];
}),
);
return { ...resource, variables };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FullResource & { variables: Record<string, string | number | boolean> } | |
| > { | |
| const allVariables = | |
| await this.workspace.repository.resourceVariableRepository.getAll(); | |
| const variables = Object.fromEntries( | |
| allVariables | |
| .filter((v) => v.resourceId === resource.id) | |
| .filter((v) => v.valueType === "direct") | |
| .map((v) => { | |
| const { value, key } = v; | |
| if (v.sensitive) | |
| return [key, variablesAES256().decrypt(String(value))]; | |
| if (typeof value === "object") return [key, JSON.stringify(value)]; | |
| return [key, value]; | |
| }), | |
| ); | |
| ): Promise<FullResource & { variables: Record<string, unknown> }> { | |
| const allVariables = | |
| await this.workspace.repository.resourceVariableRepository.getAll(); | |
| const variables = Object.fromEntries( | |
| allVariables | |
| .filter((v) => v.resourceId === resource.id) | |
| .filter((v) => v.valueType === "direct") | |
| .map((v) => { | |
| const { value, key } = v; | |
| if (v.sensitive) { | |
| const decrypted = variablesAES256().decrypt(String(value)); | |
| try { | |
| return [key, JSON.parse(decrypted)]; | |
| } catch { | |
| return [key, decrypted]; | |
| } | |
| } | |
| return [key, value]; | |
| }), | |
| ); | |
| return { ...resource, variables }; | |
| } |
🤖 Prompt for AI Agents
In
apps/event-queue/src/release-targets/evaluate/variables/deployment-variable-provider.ts
around lines 125 to 140, the current code stringifies non-sensitive JSON
variable values which destroys nested structure and breaks path lookups;
instead, when v.sensitive decrypt the value and attempt to JSON.parse it (fall
back to the decrypted string if parse fails), and when the value is a
non-sensitive object return it as-is (do not JSON.stringify), so
fullSource.variables preserves nested objects and path resolution works
correctly; update types if needed to allow objects in the resulting variables
map.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests