Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a "variable sets" feature: DB tables and migrations, Drizzle/pg/sqlc mappings, OpenAPI schemas and endpoints, API route handlers, backend conversion/querying, resolver integration (CEL-based filtering + priority resolution), tests and SDK types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
participant Resolver
participant CEL
Client->>API: POST /v1/workspaces/{id}/variable-sets (create)
API->>DB: INSERT variable_set + variable_set_variable rows
DB-->>API: Created rows
API-->>Client: 201 Created
Client->>Resolver: Resolve variables (workspace scope)
Resolver->>DB: ListVariableSetsWithVariablesByWorkspaceID(workspaceID)
DB-->>Resolver: [VariableSetWithVariables...]
loop evaluate selectors
Resolver->>CEL: compile & eval selector against scope
CEL-->>Resolver: true / false
alt true
Resolver->>Resolver: include set in filtered list
end
end
loop for each variable key
Resolver->>Resolver: resolveFromResource()
alt found
Resolver-->>Client: use resource value
else
Resolver->>Resolver: resolveFromValues()
alt found
Resolver-->>Client: use value
else
Resolver->>Resolver: resolveFromVariableSets(filtered)
alt found
Resolver-->>Client: use highest-priority variable-set value
else
Resolver-->>Client: unresolved
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go (1)
328-351: Silent error handling may hide selector configuration issues.
filterVariableSetssilently skips variable sets with invalid CEL selectors (lines 338-341). While this prevents errors from blocking resolution, it may hide configuration issues from operators. Consider logging compilation failures at debug/warn level.🔍 Suggested logging for debugging
+import "github.com/charmbracelet/log" + func filterVariableSets( scope *Scope, variableSets []oapi.VariableSetWithVariables, ) []oapi.VariableSetWithVariables { celCtx := cel.BuildEntityContext(scope.Resource, scope.Deployment, scope.Environment) matched := make([]oapi.VariableSetWithVariables, 0, len(variableSets)) for _, vs := range variableSets { if vs.Selector == "" { continue } program, err := cel.CompileProgram(vs.Selector) if err != nil { + log.Debug("failed to compile variable set selector", "variable_set_id", vs.Id, "selector", vs.Selector, "error", err) continue } result, _ := celutil.EvalBool(program, celCtx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go` around lines 328 - 351, In filterVariableSets, do not silently ignore compile errors for vs.Selector; capture the error returned by cel.CompileProgram(vs.Selector) and log it (at debug or warn) including the selector value, the error, and identifying context (e.g., scope.Resource/Deployment/Environment or variable set ID/name) so operators can detect misconfigured selectors; keep the existing behavior of skipping the invalid selector after logging.apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go (1)
1007-1211: Good test coverage for variable set resolution.The test cases effectively validate the key behaviors:
- Selector matching and value injection
- Precedence hierarchy (resource vars > deployment vars > variable sets)
- Priority-based resolution among multiple matching sets
- Non-matching selectors are ignored
Consider adding edge case tests for:
- Variable set with reference values (not just literals)
- Error handling when
GetVariableSetsWithVariablesfails- Empty selector string behavior (matches all vs matches none)
These would strengthen the test suite but aren't blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go` around lines 1007 - 1211, Add three focused tests to the Resolve suite: (1) a test that uses makeVariableSet to inject a non-literal/reference value (e.g., a reference like "${other_var}") and ensures Resolve resolves references correctly (use mockGetter with deploymentVars/resourceVars containing the referenced key); (2) a test that simulates GetVariableSetsWithVariables failing by having mockGetter return an error for that method and asserting Resolve returns the error (or appropriate handling); and (3) a test for empty selector behavior by creating a VariableSetWithVariables with an empty Selector string via makeVariableSet and asserting whether Resolve treats it as matching-all or matching-none per the intended contract. Ensure each test invokes Resolve(context.Background(), getter, scope, scope.Deployment.Id, scope.Resource.Id) and checks outcomes for keys like "log_level" as in existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/db/convert.go`:
- Around line 350-351: The code uses uuid.MustParse on fields Id and
VariableSetId which will panic on malformed UUIDs; replace these calls with
uuid.Parse and handle the error in the surrounding conversion function (e.g.,
the converter that constructs the struct from v) by returning a descriptive
error (or skipping the record if that is preferred) instead of panicking; update
both occurrences (Id and VariableSetId) and any other uuid.MustParse uses in
convert.go to parse, check err, and propagate or handle it cleanly so malformed
DB values don't crash the process.
In `@apps/workspace-engine/test/controllers/harness/mocks.go`:
- Around line 467-469: The mock method
DesiredReleaseGetter.GetVariableSetsWithVariables currently returns nil, nil
instead of the stored mock data; change it to return the struct's VariableSets
field and the mock error field (e.g., return g.VariableSets, g.Err) so it
behaves like other mocks (for example GetDeploymentVariables) and honors the
VariableSets test data on the DesiredReleaseGetter type.
---
Nitpick comments:
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go`:
- Around line 1007-1211: Add three focused tests to the Resolve suite: (1) a
test that uses makeVariableSet to inject a non-literal/reference value (e.g., a
reference like "${other_var}") and ensures Resolve resolves references correctly
(use mockGetter with deploymentVars/resourceVars containing the referenced key);
(2) a test that simulates GetVariableSetsWithVariables failing by having
mockGetter return an error for that method and asserting Resolve returns the
error (or appropriate handling); and (3) a test for empty selector behavior by
creating a VariableSetWithVariables with an empty Selector string via
makeVariableSet and asserting whether Resolve treats it as matching-all or
matching-none per the intended contract. Ensure each test invokes
Resolve(context.Background(), getter, scope, scope.Deployment.Id,
scope.Resource.Id) and checks outcomes for keys like "log_level" as in existing
tests.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go`:
- Around line 328-351: In filterVariableSets, do not silently ignore compile
errors for vs.Selector; capture the error returned by
cel.CompileProgram(vs.Selector) and log it (at debug or warn) including the
selector value, the error, and identifying context (e.g.,
scope.Resource/Deployment/Environment or variable set ID/name) so operators can
detect misconfigured selectors; keep the existing behavior of skipping the
invalid selector after logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c188170b-ff9a-4b4f-a1cf-39a965cf9c7a
📒 Files selected for processing (21)
apps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/main.jsonnetapps/workspace-engine/oapi/spec/schemas/variablesets.jsonnetapps/workspace-engine/pkg/db/convert.goapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/queries/variable_sets.sqlapps/workspace-engine/pkg/db/sqlc.yamlapps/workspace-engine/pkg/db/variable_sets.sql.goapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters_postgres.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.goapps/workspace-engine/test/controllers/harness/mocks.gopackages/db/drizzle/0182_silent_blazing_skull.sqlpackages/db/drizzle/meta/0182_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/index.tspackages/db/src/schema/variable-set.ts
Comment on lines
+467
to
+469
| func (g *DesiredReleaseGetter) GetVariableSetsWithVariables(ctx context.Context, workspaceID uuid.UUID) ([]oapi.VariableSetWithVariables, error) { | ||
| return nil, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Mock method doesn't use the VariableSets field.
The GetVariableSetsWithVariables method always returns nil, nil, ignoring the VariableSets field added at line 122. This is inconsistent with other mock methods like GetDeploymentVariables which return their corresponding field values.
🔧 Proposed fix
func (g *DesiredReleaseGetter) GetVariableSetsWithVariables(ctx context.Context, workspaceID uuid.UUID) ([]oapi.VariableSetWithVariables, error) {
- return nil, nil
+ return g.VariableSets, nil
}📝 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
| func (g *DesiredReleaseGetter) GetVariableSetsWithVariables(ctx context.Context, workspaceID uuid.UUID) ([]oapi.VariableSetWithVariables, error) { | |
| return nil, nil | |
| } | |
| func (g *DesiredReleaseGetter) GetVariableSetsWithVariables(ctx context.Context, workspaceID uuid.UUID) ([]oapi.VariableSetWithVariables, error) { | |
| return g.VariableSets, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/test/controllers/harness/mocks.go` around lines 467 -
469, The mock method DesiredReleaseGetter.GetVariableSetsWithVariables currently
returns nil, nil instead of the stored mock data; change it to return the
struct's VariableSets field and the mock error field (e.g., return
g.VariableSets, g.Err) so it behaves like other mocks (for example
GetDeploymentVariables) and honors the VariableSets test data on the
DesiredReleaseGetter type.
There was a problem hiding this comment.
Pull request overview
Adds “variable sets” as a new workspace-scoped data model and integrates them into the workspace-engine variable resolution flow so release-target variables can be provided by prioritized, selector-matched sets.
Changes:
- Introduces new DB tables
variable_setandvariable_set_variable(Drizzle schema + migration + snapshot/journal). - Extends desired-release variable resolution to load, filter (CEL selector), and apply variable sets (between deployment variable values and defaults), with tests.
- Adds sqlc query + Go DB conversion and OAPI schema/models for variable sets.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db/src/schema/variable-set.ts | Defines new Drizzle tables for variable sets and their variables. |
| packages/db/src/schema/index.ts | Exports the new schema module. |
| packages/db/drizzle/meta/0182_snapshot.json | Drizzle snapshot updated to include new tables. |
| packages/db/drizzle/meta/_journal.json | Drizzle journal updated with migration entry. |
| packages/db/drizzle/0182_silent_blazing_skull.sql | Creates the new tables and constraints in SQL migration. |
| apps/workspace-engine/test/controllers/harness/mocks.go | Updates test harness getter to satisfy new interface method. |
| apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go | Loads, filters, and resolves variables from variable sets. |
| apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go | Adds unit tests covering variable-set resolution precedence and priority. |
| apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters.go | Extends Getter interface with variable set retrieval. |
| apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters_postgres.go | Implements variable set retrieval via sqlc query + conversion. |
| apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go | Updates reconcile test mock to satisfy new Getter interface. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Adds generated OAPI models for variable sets. |
| apps/workspace-engine/pkg/db/variable_sets.sql.go | Adds generated sqlc code for listing variable sets with variables. |
| apps/workspace-engine/pkg/db/sqlc.yaml | Registers new query file and type overrides for variable set columns. |
| apps/workspace-engine/pkg/db/queries/variable_sets.sql | Adds sqlc query to fetch variable sets and aggregated variables for a workspace. |
| apps/workspace-engine/pkg/db/queries/schema.sql | Adds table definitions for sqlc schema context. |
| apps/workspace-engine/pkg/db/models.go | Adds DB model structs for variable sets. |
| apps/workspace-engine/pkg/db/convert.go | Adds row→OAPI conversion for variable sets with variables. |
| apps/workspace-engine/oapi/spec/schemas/variablesets.jsonnet | Adds OpenAPI schema definitions for variable sets. |
| apps/workspace-engine/oapi/spec/main.jsonnet | Includes the new variable set schemas into the spec build. |
| apps/workspace-engine/oapi/openapi.json | Generated OpenAPI updated with variable set schemas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| func (g *DesiredReleaseGetter) GetVariableSetsWithVariables(ctx context.Context, workspaceID uuid.UUID) ([]oapi.VariableSetWithVariables, error) { | ||
| return nil, nil |
There was a problem hiding this comment.
GetVariableSetsWithVariables currently returns (nil, nil) and ignores the DesiredReleaseGetter.VariableSets field. With variable-set resolution now part of variable resolving, this mock should return the configured VariableSets so harness-based tests can exercise the behavior (and to avoid surprising empty results when VariableSets is populated).
Suggested change
| return nil, nil | |
| if g == nil { | |
| return nil, nil | |
| } | |
| return g.VariableSets, nil |
Comment on lines
+10
to
+37
| } from "drizzle-orm/pg-core"; | ||
|
|
||
| import { workspace } from "./workspace.js"; | ||
|
|
||
| export const variableSet = pgTable("variable_set", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| name: text("name").notNull(), | ||
| description: text("description").notNull().default(""), | ||
| selector: text("selector").notNull(), | ||
|
|
||
| metadata: jsonb("metadata") | ||
| .notNull() | ||
| .default("{}") | ||
| .$type<Record<string, string>>(), | ||
|
|
||
| priority: integer("priority").notNull().default(0), | ||
|
|
||
| workspaceId: uuid("workspace_id") | ||
| .notNull() | ||
| .references(() => workspace.id, { onDelete: "cascade" }), | ||
| createdAt: timestamp("created_at", { withTimezone: true }) | ||
| .notNull() | ||
| .defaultNow(), | ||
| updatedAt: timestamp("updated_at", { withTimezone: true }) | ||
| .notNull() | ||
| .defaultNow() | ||
| .$onUpdate(() => new Date()), | ||
| }); |
There was a problem hiding this comment.
variable_set is queried by workspace_id (e.g., ListVariableSetsWithVariablesByWorkspaceID) but the table definition does not add an index on workspaceId. Consider adding an index (or a composite index like (workspaceId, priority DESC)) to avoid full table scans as variable sets grow; this matches the pattern used by other workspace-scoped tables (e.g., system has index().on(workspaceId)).
Suggested change
| } from "drizzle-orm/pg-core"; | |
| import { workspace } from "./workspace.js"; | |
| export const variableSet = pgTable("variable_set", { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| name: text("name").notNull(), | |
| description: text("description").notNull().default(""), | |
| selector: text("selector").notNull(), | |
| metadata: jsonb("metadata") | |
| .notNull() | |
| .default("{}") | |
| .$type<Record<string, string>>(), | |
| priority: integer("priority").notNull().default(0), | |
| workspaceId: uuid("workspace_id") | |
| .notNull() | |
| .references(() => workspace.id, { onDelete: "cascade" }), | |
| createdAt: timestamp("created_at", { withTimezone: true }) | |
| .notNull() | |
| .defaultNow(), | |
| updatedAt: timestamp("updated_at", { withTimezone: true }) | |
| .notNull() | |
| .defaultNow() | |
| .$onUpdate(() => new Date()), | |
| }); | |
| index, | |
| } from "drizzle-orm/pg-core"; | |
| import { workspace } from "./workspace.js"; | |
| export const variableSet = pgTable( | |
| "variable_set", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| name: text("name").notNull(), | |
| description: text("description").notNull().default(""), | |
| selector: text("selector").notNull(), | |
| metadata: jsonb("metadata") | |
| .notNull() | |
| .default("{}") | |
| .$type<Record<string, string>>(), | |
| priority: integer("priority").notNull().default(0), | |
| workspaceId: uuid("workspace_id") | |
| .notNull() | |
| .references(() => workspace.id, { onDelete: "cascade" }), | |
| createdAt: timestamp("created_at", { withTimezone: true }) | |
| .notNull() | |
| .defaultNow(), | |
| updatedAt: timestamp("updated_at", { withTimezone: true }) | |
| .notNull() | |
| .defaultNow() | |
| .$onUpdate(() => new Date()), | |
| }, | |
| (t) => [index("variable_set_workspace_id_idx").on(t.workspaceId)], | |
| ); |
| "workspace_id" uuid NOT NULL, | ||
| "created_at" timestamp with time zone DEFAULT now() NOT NULL, | ||
| "updated_at" timestamp with time zone DEFAULT now() NOT NULL | ||
| ); |
There was a problem hiding this comment.
The migration creates variable_set without an index on workspace_id. Since reads filter by workspace_id, add an index (optionally composite with priority) to avoid full scans as data grows.
Suggested change
| ); | |
| ); | |
| CREATE INDEX "variable_set_workspace_id_priority_idx" ON "variable_set" ("workspace_id","priority"); |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go (1)
337-341: Surface invalid selector failures in telemetry.A selector that fails to compile or evaluate is silently treated as a non-match here, which makes a broken variable set indistinguishable from a legitimate miss. Since
Resolveis already instrumented, a span event or counter for invalid selectors would make these fallbacks much easier to debug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go` around lines 337 - 341, The selector compilation/evaluation currently swallows errors (in the cel.CompileProgram(vs.Selector) and celutil.EvalBool(program, celCtx) paths) so invalid selectors look like non-matches; update Resolve to record telemetry when a selector fails by adding a span event or incrementing a counter/tag on the active span for both compile failures (err from cel.CompileProgram) and evaluation failures (error returned from celutil.EvalBool), and include the selector (vs.Selector) and error message in the telemetry to make broken variable sets diagnosable.apps/api/openapi/paths/variablesets.jsonnet (1)
52-71: Usepatchhere if omitted fields are preserved.
UpdateVariableSetis modeled as a partial body downstream, so documenting this asputmakes the contract read like full replacement. If the handler merges fields, switch this topatch; otherwise the update schema should make every field required.Suggested change
- put: { + patch: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/openapi/paths/variablesets.jsonnet` around lines 52 - 71, The OpenAPI operation currently uses put with operationId 'updateVariableSet' and request schema 'UpdateVariableSet', but that schema is a partial/merge-style update; change the HTTP verb from put to patch (replace the key "put" with "patch") so the contract reflects a partial update, and keep the same parameters and requestBody referencing openapi.schemaRef('UpdateVariableSet'); alternatively, if you intend full replacement, make the 'UpdateVariableSet' schema require all fields instead of being partial.apps/api/openapi/openapi.json (1)
2442-2464: Make this update contract clearly patch-like or replacement-like.
UpdateVariableSetmakes every field optional, but the operation is exposed asPUT. Right now callers cannot tell whether omitted fields are preserved or cleared. Either switch this toPATCHor require the full replacement shape inUpdateVariableSet.Also applies to: 8925-8957
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/openapi/openapi.json` around lines 2442 - 2464, The UpdateVariableSet schema currently presents an object with all optional fields but is used by a PUT operation, causing ambiguity whether omitted fields are preserved or cleared; update the API to be explicit by either (A) changing the operation from PUT to PATCH and keeping UpdateVariableSet as a partial/patch schema (document semantics as partial update), or (B) making UpdateVariableSet a full replacement schema by marking required properties (e.g., name, variables, priority, selector, description as needed) and updating any endpoints that reference UpdateVariableSet to use the replacement semantics; ensure you update the OpenAPI components/schemas entry for "UpdateVariableSet" and the corresponding path operation(s) that reference it so the contract clearly indicates patch-like or replacement behavior.apps/api/src/routes/v1/workspaces/variable-sets.ts (1)
24-29: Refactor mixed.then()chains to pureawaitfor consistency.The queries mix
awaitwith.then()chains, which violates the guideline to prefer async/await over raw promises. Use intermediate variables or helper functions with pureawaitinstead:// Before const total = await db .select({ total: count() }) .from(schema.variableSet) .where(eq(schema.variableSet.workspaceId, workspaceId)) .then(takeFirst) .then(({ total }) => total); // After const result = await db .select({ total: count() }) .from(schema.variableSet) .where(eq(schema.variableSet.workspaceId, workspaceId)) .then(takeFirst); const total = result.total;Apply to lines: 24–29, 62–67, 89–98, 117–128, 160–169.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/workspaces/variable-sets.ts` around lines 24 - 29, Replace the mixed await/.then() promise chains in the variable-set queries with pure async/await using intermediate variables: for each occurrence where you call db.select(...).from(schema.variableSet).where(eq(schema.variableSet.workspaceId, workspaceId)).then(takeFirst).then(({ total }) => total) (and the similar blocks at the other line ranges), await the query once into a result variable (e.g., const result = await db.select(...).from(schema.variableSet).where(...).then(takeFirst);) and then extract the value with const total = result.total; ensure you update all instances referencing count(), takeFirst, schema.variableSet and workspaceId to follow this pattern for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/openapi/openapi.json`:
- Around line 268-295: The OpenAPI schemas are inconsistent: the
CreateVariableSet write schema (CreateVariableSet) allows omitting description
and priority while the VariableSet response schema requires them; update the
contract so clients aren’t confused by missing fields. Either make description
and priority optional in the VariableSet response schema (VariableSet) or add
defaulting/documentation and mark them required in CreateVariableSet;
specifically adjust the "required" arrays for CreateVariableSet and VariableSet
(and mirror changes where referenced around the other occurrences you noted) and
add clear default values/descriptions for description and priority if the server
populates them automatically.
In `@apps/api/src/routes/v1/workspaces/variable-sets.ts`:
- Around line 31-36: The query that fetches sets via schema.variableSet
currently uses .limit(limit).offset(offset) without ordering, which makes
pagination unstable; update the db query (the select from schema.variableSet
where eq(schema.variableSet.workspaceId, workspaceId)) to include an explicit
.orderBy(...) using the intended sort key (e.g., schema.variableSet.priority)
and add a stable tie-breaker (e.g., schema.variableSet.id or createdAt) to
ensure deterministic ordering across pages.
- Around line 60-76: The code dereferences variables.length without guarding for
null/undefined; before using variables in the POST handler (where created and
tx.insert(schema.variableSetVariable) are used) normalize it to an array (e.g.,
const variables = Array.isArray(req.body.variables) ? req.body.variables : [])
so subsequent checks like variables.length and the variables.map call are safe;
update the code that builds setData to use the normalized variables variable,
and ensure the same defensive pattern is applied where variables is read in this
file (e.g., around created, tx.insert(schema.variableSetVariable), and any PUT
handlers).
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go`:
- Around line 82-91: Update the exported Resolve function doc comment to reflect
the new precedence chain that includes variable sets before defaults: locate the
Resolve docblock (and the related exported comment around lines covering the
Resolve declaration and the block referred to at 136-160) and change the
description from the old "three-step precedence chain" to explicitly list the
current precedence order (including variable sets, e.g., runtime
inputs/overrides -> variable sets -> defaults or whatever the implemented order
is), and mention any scope/filtering behavior (referencing symbols like Resolve,
filterVariableSets, and getter.GetVariableSetsWithVariables) so the exported
contract matches the implementation.
- Around line 306-325: The resolution is nondeterministic when variable sets
share the same Priority; update the ordering in both the SQL query
(variable_sets.sql) and in the Go-side sort used by filterVariableSets to add a
deterministic secondary key (e.g., CreatedAt DESC then Id ASC, or Name) so
equal-priority sets are consistently ordered; specifically, modify the SQL ORDER
BY to include the secondary column(s) after Priority and update
filterVariableSets to sort variableSets by the same fields (referencing the
VariableSet struct fields like Priority, CreatedAt, Id or Name) before
resolveFromVariableSets iterates.
---
Nitpick comments:
In `@apps/api/openapi/openapi.json`:
- Around line 2442-2464: The UpdateVariableSet schema currently presents an
object with all optional fields but is used by a PUT operation, causing
ambiguity whether omitted fields are preserved or cleared; update the API to be
explicit by either (A) changing the operation from PUT to PATCH and keeping
UpdateVariableSet as a partial/patch schema (document semantics as partial
update), or (B) making UpdateVariableSet a full replacement schema by marking
required properties (e.g., name, variables, priority, selector, description as
needed) and updating any endpoints that reference UpdateVariableSet to use the
replacement semantics; ensure you update the OpenAPI components/schemas entry
for "UpdateVariableSet" and the corresponding path operation(s) that reference
it so the contract clearly indicates patch-like or replacement behavior.
In `@apps/api/openapi/paths/variablesets.jsonnet`:
- Around line 52-71: The OpenAPI operation currently uses put with operationId
'updateVariableSet' and request schema 'UpdateVariableSet', but that schema is a
partial/merge-style update; change the HTTP verb from put to patch (replace the
key "put" with "patch") so the contract reflects a partial update, and keep the
same parameters and requestBody referencing
openapi.schemaRef('UpdateVariableSet'); alternatively, if you intend full
replacement, make the 'UpdateVariableSet' schema require all fields instead of
being partial.
In `@apps/api/src/routes/v1/workspaces/variable-sets.ts`:
- Around line 24-29: Replace the mixed await/.then() promise chains in the
variable-set queries with pure async/await using intermediate variables: for
each occurrence where you call
db.select(...).from(schema.variableSet).where(eq(schema.variableSet.workspaceId,
workspaceId)).then(takeFirst).then(({ total }) => total) (and the similar blocks
at the other line ranges), await the query once into a result variable (e.g.,
const result = await
db.select(...).from(schema.variableSet).where(...).then(takeFirst);) and then
extract the value with const total = result.total; ensure you update all
instances referencing count(), takeFirst, schema.variableSet and workspaceId to
follow this pattern for consistency.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go`:
- Around line 337-341: The selector compilation/evaluation currently swallows
errors (in the cel.CompileProgram(vs.Selector) and celutil.EvalBool(program,
celCtx) paths) so invalid selectors look like non-matches; update Resolve to
record telemetry when a selector fails by adding a span event or incrementing a
counter/tag on the active span for both compile failures (err from
cel.CompileProgram) and evaluation failures (error returned from
celutil.EvalBool), and include the selector (vs.Selector) and error message in
the telemetry to make broken variable sets diagnosable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 698140c8-1475-4619-b0aa-56e0b0563617
📒 Files selected for processing (15)
apps/api/openapi/main.jsonnetapps/api/openapi/openapi.jsonapps/api/openapi/paths/variablesets.jsonnetapps/api/openapi/schemas/variablesets.jsonnetapps/api/src/routes/v1/workspaces/index.tsapps/api/src/routes/v1/workspaces/variable-sets.tsapps/api/src/types/openapi.tsapps/workspace-engine/pkg/db/convert.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters_postgres.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.goapps/workspace-engine/test/controllers/harness/mocks.gopackages/workspace-engine-sdk/src/schema.ts
✅ Files skipped from review due to trivial changes (3)
- apps/api/openapi/main.jsonnet
- apps/workspace-engine/pkg/db/convert.go
- apps/api/openapi/schemas/variablesets.jsonnet
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters.go
- apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go
- apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go
- apps/workspace-engine/test/controllers/harness/mocks.go
Comment on lines
+268
to
+295
| "CreateVariableSet": { | ||
| "properties": { | ||
| "description": { | ||
| "type": "string" | ||
| }, | ||
| "name": { | ||
| "type": "string" | ||
| }, | ||
| "priority": { | ||
| "type": "integer" | ||
| }, | ||
| "selector": { | ||
| "description": "A CEL expression to select which release targets this variable set applies to", | ||
| "type": "string" | ||
| }, | ||
| "variables": { | ||
| "items": { | ||
| "$ref": "#/components/schemas/VariableSetVariable" | ||
| }, | ||
| "type": "array" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "name", | ||
| "selector", | ||
| "variables" | ||
| ], | ||
| "type": "object" |
Contributor
There was a problem hiding this comment.
Align the write schemas with the required response fields.
Lines 270-277 and 2444-2451 let callers omit description and priority, but Lines 2984-2986 mark them as always present on VariableSet. That leaves the contract ambiguous for generated clients: either the server is defaulting these fields and the defaults need to be documented here, or the input schemas are too permissive / the response schema is too strict.
Also applies to: 2442-2464, 2981-2989
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/openapi/openapi.json` around lines 268 - 295, The OpenAPI schemas
are inconsistent: the CreateVariableSet write schema (CreateVariableSet) allows
omitting description and priority while the VariableSet response schema requires
them; update the contract so clients aren’t confused by missing fields. Either
make description and priority optional in the VariableSet response schema
(VariableSet) or add defaulting/documentation and mark them required in
CreateVariableSet; specifically adjust the "required" arrays for
CreateVariableSet and VariableSet (and mirror changes where referenced around
the other occurrences you noted) and add clear default values/descriptions for
description and priority if the server populates them automatically.
apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go
Show resolved
Hide resolved
Comment on lines
+306
to
+325
| func resolveFromVariableSets( | ||
| ctx context.Context, | ||
| key string, | ||
| variableSets []oapi.VariableSetWithVariables, | ||
| resolver RelatedEntityResolver, | ||
| resourceID string, | ||
| entity *oapi.RelatableEntity, | ||
| ) *oapi.LiteralValue { | ||
| for _, vs := range variableSets { | ||
| for _, v := range vs.Variables { | ||
| if v.Key == key { | ||
| lv, err := ResolveValue(ctx, resolver, resourceID, entity, &v.Value) | ||
| if err == nil && lv != nil { | ||
| return lv | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the file structure and find the relevant files
cd apps/workspace-engine || exit 1
find . -name "resolve.go" -type f | head -5Repository: ctrlplanedev/ctrlplane
Length of output: 126
🏁 Script executed:
# Let's read the resolve.go file to understand the complete context
cat -n svc/controllers/desiredrelease/variableresolver/resolve.go | head -100Repository: ctrlplanedev/ctrlplane
Length of output: 156
🏁 Script executed:
# Also find the variable_sets.sql file
find . -name "variable_sets.sql" -type fRepository: ctrlplanedev/ctrlplane
Length of output: 122
🏁 Script executed:
# Search for filterVariableSets function
rg "func.*filterVariableSets" --type goRepository: ctrlplanedev/ctrlplane
Length of output: 171
🏁 Script executed:
# Read the resolve.go file from the correct path
cat -n apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.goRepository: ctrlplanedev/ctrlplane
Length of output: 14025
🏁 Script executed:
# Read the SQL file to check the query
cat -n apps/workspace-engine/pkg/db/queries/variable_sets.sqlRepository: ctrlplanedev/ctrlplane
Length of output: 634
🏁 Script executed:
# Search for any comments or documentation about priority or tie-breaking
rg -i "priority|tie.*break|same.*priority|deterministic" apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/ --type goRepository: ctrlplanedev/ctrlplane
Length of output: 4144
🏁 Script executed:
# Check if there's any validation that rejects same-priority variable sets
rg -i "priority.*collision|same.*priority.*reject|validation.*priority" apps/workspace-engine/ --type goRepository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Also check the oapi.VariableSetWithVariables structure to understand the fields
rg -A 10 "type VariableSetWithVariables" apps/workspace-engine/ --type goRepository: ctrlplanedev/ctrlplane
Length of output: 896
🏁 Script executed:
# Check the test file for same-priority scenarios
cat -n apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go | grep -A 30 "VariableSet_HighestPriorityWins"Repository: ctrlplanedev/ctrlplane
Length of output: 1133
🏁 Script executed:
# Look for any test that has same-priority scenarios
rg -A 20 "Priority.*Priority" apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go | head -80Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check the full VariableSetWithVariables structure
rg -A 20 "type VariableSetWithVariables struct" apps/workspace-engine/pkg/oapi/oapi.gen.goRepository: ctrlplanedev/ctrlplane
Length of output: 791
Add a secondary sort key to ensure deterministic ordering of equal-priority variable sets.
When two variable sets have the same priority and match the same variable key, resolveFromVariableSets returns whichever appears first in the input list. However, the SQL query (lines 1-8 of variable_sets.sql) and filterVariableSets (lines 346-348) sort only by Priority with no secondary order, leaving the order undefined for equal-priority sets. This causes nondeterministic resolution. Add a deterministic secondary sort key (e.g., by CreatedAt, Id, or Name) to both the SQL query and the sort in filterVariableSets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go`
around lines 306 - 325, The resolution is nondeterministic when variable sets
share the same Priority; update the ordering in both the SQL query
(variable_sets.sql) and in the Go-side sort used by filterVariableSets to add a
deterministic secondary key (e.g., CreatedAt DESC then Id ASC, or Name) so
equal-priority sets are consistently ordered; specifically, modify the SQL ORDER
BY to include the secondary column(s) after Priority and update
filterVariableSets to sort variableSets by the same fields (referencing the
VariableSet struct fields like Priority, CreatedAt, Id or Name) before
resolveFromVariableSets iterates.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/src/routes/v1/workspaces/variable-sets.ts (1)
32-38:⚠️ Potential issue | 🟠 MajorFinish the page sort with a unique tiebreaker.
priority DESC, name ASCis still unstable when two sets share the same priority and name, solimit/offsetcan still produce duplicates or gaps across pages. Add a final unique column such asid.📐 Suggested change
const sets = await db .select() .from(schema.variableSet) .where(eq(schema.variableSet.workspaceId, workspaceId)) - .orderBy(desc(schema.variableSet.priority), schema.variableSet.name) + .orderBy( + desc(schema.variableSet.priority), + schema.variableSet.name, + schema.variableSet.id, + ) .limit(limit) .offset(offset);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/workspaces/variable-sets.ts` around lines 32 - 38, The ordering for the variable set query is unstable for pagination; update the query that builds sets (the db.select() -> .from(schema.variableSet) chain) to include a final unique tiebreaker column by appending schema.variableSet.id to the .orderBy (e.g. .orderBy(desc(schema.variableSet.priority), schema.variableSet.name, schema.variableSet.id)) so ties on priority and name become deterministic and prevent duplicate/missing rows across pages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/workspaces/variable-sets.ts`:
- Around line 49-54: The handler is returning raw DB rows (using sets.map and
allVariables) which exposes internal columns; update the projection so you
construct the published API shapes instead of spreading DB rows: for each set in
sets build a VariableSet object (pick only the public fields declared in
VariableSet in openapi.ts—e.g., id/name/description/createdAt/updatedAt/etc.,
omit workspaceId and metadata) and for each variable in allVariables build a
VariableSetVariable object (pick only the public fields, omit internal id and
variableSetId), then assemble items and return res.status(200).json({ items,
total, limit, offset }); apply the same mapping logic to the other handler
blocks that use sets, allVariables, and the items response.
- Around line 21-29: The handlers read req.params.workspaceId (assigned to
workspaceId) and use it to query DB without authorization; add an explicit
workspace access check at the top of each handler that uses workspaceId (the
occurrences around the initial total count and the other blocks noted) by
calling the project's existing authorization helper (e.g., a function like
ensureWorkspaceMembership, checkWorkspaceAccess, or similar) with the current
user/context and workspaceId, and return 403 if the check fails; do this before
any DB queries or mutations that reference workspaceId so every branch (count,
list, get, create, update, delete) verifies membership/ownership first.
---
Duplicate comments:
In `@apps/api/src/routes/v1/workspaces/variable-sets.ts`:
- Around line 32-38: The ordering for the variable set query is unstable for
pagination; update the query that builds sets (the db.select() ->
.from(schema.variableSet) chain) to include a final unique tiebreaker column by
appending schema.variableSet.id to the .orderBy (e.g.
.orderBy(desc(schema.variableSet.priority), schema.variableSet.name,
schema.variableSet.id)) so ties on priority and name become deterministic and
prevent duplicate/missing rows across pages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9f033f2-fa78-4b60-8c69-6a7113e16f41
📒 Files selected for processing (8)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/variablesets.jsonnetapps/api/src/routes/v1/workspaces/variable-sets.tsapps/api/src/types/openapi.tsapps/web/app/api/openapi.tsapps/workspace-engine/pkg/db/queries/variable_sets.sqlapps/workspace-engine/pkg/db/variable_sets.sql.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go
✅ Files skipped from review due to trivial changes (3)
- apps/workspace-engine/pkg/db/queries/variable_sets.sql
- apps/api/openapi/schemas/variablesets.jsonnet
- apps/workspace-engine/pkg/db/variable_sets.sql.go
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/openapi/openapi.json
Comment on lines
+21
to
+29
| const { workspaceId } = req.params; | ||
| const limit = req.query.limit ?? 50; | ||
| const offset = req.query.offset ?? 0; | ||
|
|
||
| const total = await db | ||
| .select({ total: count() }) | ||
| .from(schema.variableSet) | ||
| .where(eq(schema.variableSet.workspaceId, workspaceId)) | ||
| .then(takeFirst) |
Contributor
There was a problem hiding this comment.
Authorize workspaceId before using it.
apps/api/src/routes/v1/workspaces/index.ts mounts this router directly under /:workspaceId/variable-sets, and every handler here trusts that path param as the authorization boundary. That lets a caller read or mutate another workspace’s variable sets by swapping the UUID in the URL unless you do an explicit membership/ownership check first.
Also applies to: 61-69, 91-100, 121-129, 163-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/workspaces/variable-sets.ts` around lines 21 - 29, The
handlers read req.params.workspaceId (assigned to workspaceId) and use it to
query DB without authorization; add an explicit workspace access check at the
top of each handler that uses workspaceId (the occurrences around the initial
total count and the other blocks noted) by calling the project's existing
authorization helper (e.g., a function like ensureWorkspaceMembership,
checkWorkspaceAccess, or similar) with the current user/context and workspaceId,
and return 403 if the check fails; do this before any DB queries or mutations
that reference workspaceId so every branch (count, list, get, create, update,
delete) verifies membership/ownership first.
Comment on lines
+49
to
+54
| const items = sets.map((s) => ({ | ||
| ...s, | ||
| variables: allVariables.filter((v) => v.variableSetId === s.id), | ||
| })); | ||
|
|
||
| res.status(200).json({ items, total, limit, offset }); |
Contributor
There was a problem hiding this comment.
Project DB rows into the published API shape.
These handlers currently return raw schema.variableSet / schema.variableSetVariable rows. That exposes workspaceId and metadata on sets plus id and variableSetId on variables, but apps/api/src/types/openapi.ts declares VariableSet / VariableSetVariable without those fields. This leaks internal columns and makes the implementation diverge from the generated SDK contract.
🧩 Suggested mapping
+const toVariableSet = ({
+ metadata: _metadata,
+ workspaceId: _workspaceId,
+ ...variableSet
+}: typeof schema.variableSet.$inferSelect) => variableSet;
+
+const toVariableSetVariable = ({
+ id: _id,
+ variableSetId: _variableSetId,
+ ...variable
+}: typeof schema.variableSetVariable.$inferSelect) => variable;
+
const items = sets.map((s) => ({
- ...s,
- variables: allVariables.filter((v) => v.variableSetId === s.id),
+ ...toVariableSet(s),
+ variables: allVariables
+ .filter((v) => v.variableSetId === s.id)
+ .map(toVariableSetVariable),
}));
- res.status(201).json(created);
+ res.status(201).json(toVariableSet(created));
- res.json({ ...vs, variables });
+ res.json({
+ ...toVariableSet(vs),
+ variables: variables.map(toVariableSetVariable),
+ });
- res.status(202).json(updated);
+ res.status(202).json(toVariableSet(updated));
- res.status(202).json(deleted);
+ res.status(202).json(toVariableSet(deleted));Also applies to: 64-83, 104-109, 119-155, 162-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/workspaces/variable-sets.ts` around lines 49 - 54, The
handler is returning raw DB rows (using sets.map and allVariables) which exposes
internal columns; update the projection so you construct the published API
shapes instead of spreading DB rows: for each set in sets build a VariableSet
object (pick only the public fields declared in VariableSet in openapi.ts—e.g.,
id/name/description/createdAt/updatedAt/etc., omit workspaceId and metadata) and
for each variable in allVariables build a VariableSetVariable object (pick only
the public fields, omit internal id and variableSetId), then assemble items and
return res.status(200).json({ items, total, limit, offset }); apply the same
mapping logic to the other handler blocks that use sets, allVariables, and the
items response.
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
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
Chores