refactor: only use simple CEL strings for selectors#845
Merged
adityachoudhari26 merged 5 commits intomainfrom Mar 12, 2026
Merged
refactor: only use simple CEL strings for selectors#845adityachoudhari26 merged 5 commits intomainfrom
adityachoudhari26 merged 5 commits intomainfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughReplaces the composite Selector types (JsonSelector, CelSelector, Selector) with plain CEL-expression strings across OpenAPI schemas, generated types, selector utilities, DB mappers, evaluators, TRPC/frontend callsites, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend
participant TRPC as TRPC Router
participant API as API Server
participant WE as Workspace-Engine
participant Selector as CEL Evaluator
participant DB as Database
FE->>TRPC: request resources list (selector: string)
TRPC->>API: forward selector string
API->>WE: query/compute with selector string
WE->>Selector: compile/validate CEL string
Selector-->>WE: matcher/function
WE->>DB: fetch candidate resources
DB-->>WE: resources
WE->>Selector: evaluate matcher per resource
WE-->>API: filtered results
API-->>TRPC: response
TRPC-->>FE: deliver results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 unit tests (beta)
📝 Coding Plan
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager_bench_test.go (4)
1283-1292:⚠️ Potential issue | 🟡 MinorSame issue: Missing
ResourceSelectorfor deployment variable values.Consistent with other benchmarks — these values will be skipped. If intentional, no action needed; otherwise, add selectors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager_bench_test.go` around lines 1283 - 1292, The DeploymentVariableValue instances created in the benchmark are missing the ResourceSelector field, so they will be skipped like in other benchmarks; update the block that constructs deploymentVarValue (the oapi.DeploymentVariableValue being Upserted via st.DeploymentVariableValues.Upsert) to populate the ResourceSelector (e.g., reference the intended resource or selector used elsewhere in tests) before calling Upsert — ensure the created value includes ResourceSelector alongside Id, DeploymentVariableId, Priority, and Value so it behaves consistently with other benchmarks.
1033-1042:⚠️ Potential issue | 🟡 MinorSame issue: Missing
ResourceSelectorfor deployment variable values.These reference variable values will be skipped in selector matching. Consider adding appropriate selectors or verify this is the intended test behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager_bench_test.go` around lines 1033 - 1042, The test is creating DeploymentVariableValue objects without setting ResourceSelector, so these reference values will be skipped during selector matching; update the code that builds deploymentVarValue (the DeploymentVariableValue struct created in the loop before calling st.DeploymentVariableValues.Upsert) to populate the ResourceSelector field with an appropriate oapi.ResourceSelector that matches the deployment under test (or explicitly set the intended selector values for the scenario), ensuring the selector covers the ResourceType/ResourceId (or other selector fields) used by your selector matching logic.
231-239:⚠️ Potential issue | 🟡 MinorTest may not exercise the intended code path.
This
DeploymentVariableValueis created without aResourceSelector. With the main code change at lines 199-202 ofvariablemanager.go, this value will be skipped intryResolveDeploymentVariableValueand won't be resolved.If this value should match the test resource, add a selector:
+ refSelector := "kind == 'service'" deploymentVarValue := &oapi.DeploymentVariableValue{ Id: refValueID, DeploymentVariableId: refVarID, Priority: 1, Value: *value, + ResourceSelector: &refSelector, }Alternatively, verify that the benchmark still produces meaningful results despite this value being skipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager_bench_test.go` around lines 231 - 239, The test is inserting a DeploymentVariableValue (created with refValueID and linked to refVarID) but lacks a ResourceSelector so tryResolveDeploymentVariableValue will skip it; update the test to set DeploymentVariableValue.ResourceSelector to match the test resource (e.g., the same resource type/name/labels used in the bench) before calling st.DeploymentVariableValues.Upsert, or else adjust the benchmark assertions to acknowledge this value is intentionally skipped.
716-723:⚠️ Potential issue | 🟡 MinorSame issue: Missing
ResourceSelectorfor deployment variable values.These values will be skipped due to the nil check in the main code. Apply the same fix as suggested for lines 231-239 if these values should participate in selector matching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager_bench_test.go` around lines 716 - 723, The DeploymentVariableValue created for valueID (variableID variable) is missing ResourceSelector, so it will be skipped by the nil check during selector matching; update the deploymentVarValue construction (the oapi.DeploymentVariableValue instance created before calling st.DeploymentVariableValues.Upsert) to set ResourceSelector to the same non-nil selector used for other test values (mirror the fix applied around the earlier block at lines 231-239), ensuring the ResourceSelector field is populated with the appropriate oapi.ResourceSelector object before calling Upsert.apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager.go (1)
199-206:⚠️ Potential issue | 🟠 MajorInconsistent behavior for nil
ResourceSelector.The current code skips values where
ResourceSelectoris nil, but inapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go(lines 249-251), nil selectors are explicitly included. Test data also createsDeploymentVariableValueinstances without selectors, indicating nil is a valid state. Either align this with theresolve.gopattern or clarify why the behavior should differ here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager.go` around lines 199 - 206, The loop in variablemanager.go currently skips entries where value.ResourceSelector == nil, causing inconsistent behavior with variableresolver.resolve.go which includes nil selectors; update the logic in the function containing this snippet to treat nil ResourceSelector as a match (i.e., include those values in sortedValues) or, if a different policy is intended, add a clear comment and unit tests; specifically adjust the branch around ResourceSelector, selector.Match(ctx, *value.ResourceSelector, resource) and sortedValues so that DeploymentVariableValue instances with nil selectors are appended just like in resolve.go.apps/web/app/routes/ws/resources.tsx (1)
57-57:⚠️ Potential issue | 🟠 MajorPotential CEL injection via unsanitized user input.
Similar to
ResourceFilter.tsx,searchDebouncedis interpolated into the CEL expression without escaping single quotes. This could allow users to manipulate the query logic.🛡️ Proposed fix to escape user input
- const cleanedCel = `resource.name.contains('${searchDebounced}') || resource.identifier.contains('${searchDebounced}')`; + const escapedSearch = searchDebounced.replace(/'/g, "\\'"); + const cleanedCel = `resource.name.contains('${escapedSearch}') || resource.identifier.contains('${escapedSearch}')`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/resources.tsx` at line 57, The CEL expression built into cleanedCel interpolates raw searchDebounced allowing single-quote injection; escape or sanitize searchDebounced before embedding it into the string so quotes and other CEL-special characters are neutralized (same approach used in ResourceFilter.tsx). Locate the cleanedCel assignment in resources.tsx and replace the direct interpolation with a sanitized/escapedSearchDebounced value (or use a helper like escapeCelString) so the final expression becomes safe, e.g., ensure any single quotes in searchDebounced are escaped and input is validated before constructing resource.name.contains(...) || resource.identifier.contains(...).apps/api/openapi/schemas/workflows.jsonnet (1)
1-3:⚠️ Potential issue | 🟡 MinorRemove duplicate import statement.
The
openapilibrary is imported twice (lines 1 and 3). Remove one of the duplicate imports.Proposed fix
local openapi = import '../lib/openapi.libsonnet'; -local openapi = import '../lib/openapi.libsonnet'; - {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/openapi/schemas/workflows.jsonnet` around lines 1 - 3, There are two identical import statements for the symbol openapi; remove the duplicate so only a single import of openapi (import '../lib/openapi.libsonnet') remains; locate the duplicated lines that declare local openapi and delete one, leaving a single local openapi = import '../lib/openapi.libsonnet' declaration.packages/workspace-engine-sdk/src/schema.ts (1)
94-97:⚠️ Potential issue | 🟠 MajorLegacy selector component types are still publicly exported.
CelSelector,JsonSelector, andSelectorremain in the SDK schema surface, which keeps the deprecated selector model discoverable despite the CEL-string migration. Please remove them from the source OpenAPI components and regenerate.Also applies to: 453-457, 817-817
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workspace-engine-sdk/src/schema.ts` around lines 94 - 97, Remove the legacy selector component types from the OpenAPI schema by deleting the CelSelector, JsonSelector, and Selector component definitions and any references to them in the generated schema (look for the symbols CelSelector, JsonSelector, Selector inside the components/definitions in schema.ts and any usages in operation/request/response schemas). After removing those entries, re-run the OpenAPI schema generation/regeneration step so the TypeScript output is refreshed and no longer exports those deprecated types; ensure no other generated types still reference them and update any dependent schemas to use the new CEL-based selector type.apps/api/src/routes/v1/workspaces/policies.ts (1)
75-81:⚠️ Potential issue | 🟠 MajorStop JSON-stringifying selector fields on insert/upsert.
The read path now returns selector DB values directly, but the write path still stringifies selector strings. That leaves persisted values quoted/encoded and breaks selector shape consistency in list/get responses.
💡 Proposed fix
if (rule.environmentProgression != null) await tx.insert(schema.policyRuleEnvironmentProgression).values({ id: ruleId, policyId, - dependsOnEnvironmentSelector: JSON.stringify( - rule.environmentProgression.dependsOnEnvironmentSelector, - ), + dependsOnEnvironmentSelector: + rule.environmentProgression.dependsOnEnvironmentSelector, maximumAgeHours: rule.environmentProgression.maximumAgeHours, minimumSoakTimeMinutes: rule.environmentProgression.minimumSockTimeMinutes, minimumSuccessPercentage: rule.environmentProgression.minimumSuccessPercentage, successStatuses: rule.environmentProgression.successStatuses, }); @@ if (rule.versionSelector != null) await tx.insert(schema.policyRuleVersionSelector).values({ id: ruleId, policyId, description: rule.versionSelector.description, - selector: JSON.stringify(rule.versionSelector.selector), + selector: rule.versionSelector.selector, });Also applies to: 124-130, 189-193, 256-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/workspaces/policies.ts` around lines 75 - 81, The selector fields are being JSON.stringify'ed before persisting (e.g., in the tx.insert call into schema.policyRuleEnvironmentProgression where dependsOnEnvironmentSelector is set via JSON.stringify), causing stored values to be quoted/encoded; remove the JSON.stringify wrappers and write the selector values directly (pass the raw object/array) so the DB stores the native JSON shape; apply the same change to the other insert/upsert sites in this file that stringify selector fields (the similar blocks that set dependsOnEnvironmentSelector / environmentSelector / targetEnvironmentSelector) to ensure consistent shape on read and write.
🧹 Nitpick comments (7)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go (1)
584-585: Consider centralizing the repeated CEL selector literal.You now use the same selector string in multiple tests. A file-level constant (or helper) would reduce typo risk and keep future selector updates single-point.
♻️ Suggested cleanup
+const stagingEnvSelector = "environment.name == 'staging'" ... - selector := "environment.name == 'staging'" + selector := stagingEnvSelector ... - DependsOnEnvironmentSelector: "environment.name == 'staging'", + DependsOnEnvironmentSelector: stagingEnvSelector,Also applies to: 647-648, 680-681, 716-717, 755-756, 808-809, 841-842, 1011-1012
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go` around lines 584 - 585, The test repeats the CEL selector string "environment.name == 'staging'" across multiple cases; extract it to a single file-level constant (e.g., stagingSelector) in gradualrollout_test.go and replace all local uses of the variable named selector with that constant (affecting occurrences around the selector assignments in the tests). Ensure the constant is declared near the top of the test file so all test functions (the selector variable usages at lines referenced) import the same value and update any comments or test helper references accordingly.apps/workspace-engine/pkg/db/convert_test.go (1)
168-168: Test name no longer matches covered input format.Line 168 now passes a plain CEL string, but the test is still named
...JSONCelFormat. Consider renaming to avoid misleading coverage signals.Suggested rename
-func TestToOapiPolicyWithRules_VersionSelectorJSONCelFormat(t *testing.T) { +func TestToOapiPolicyWithRules_VersionSelectorPlainCEL_AltCase(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/convert_test.go` at line 168, The test's name still uses the suffix "JSONCelFormat" but its input (the value passed at line with {"id": ruleID, "description": "test", "selector": celExpr}) is a plain CEL string, so rename the test case/function that contains "JSONCelFormat" to a name that reflects a plain CEL string input (e.g., "PlainCelFormat" or "CelStringFormat") to avoid confusing coverage signals; update any related subtest names or table-driven test entry keys that reference "JSONCelFormat" (search for the exact "JSONCelFormat" identifier) so test names accurately match the input format.apps/workspace-engine/pkg/workspace/store/relationship_indexes.go (1)
173-176: Update the function doc comment to match the new*stringcontract.The current comment still describes extracting CEL from a
Selector, but the function now receives raw CEL text directly.As per coding guidelines: “Write comments that explain why, document complex logic/algorithms, provide non-obvious context…”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/relationship_indexes.go` around lines 173 - 176, Update the doc comment for selectorToCel to reflect that it now accepts raw CEL text as a *string (not a Selector) and that it transforms that CEL expression by replacing the entity-type variable (e.g., "resource", "deployment", "environment") with the provided prefix ("from" or "to"), keeping mention of parameters entityType (oapi.RelatableEntityType) and prefix so callers understand the contract and behavior of selectorToCel.apps/workspace-engine/oapi/spec/schemas/policy.jsonnet (1)
147-147: Description may be misleading fordependsOnEnvironmentSelector.The description states "CEL expression to determine if the environment progression rule should be used", but based on the field name and the rule's purpose, this selector matches the dependent environment(s) that must have successful releases before progression, not whether the rule applies.
Consider clarifying:
📝 Suggested description update
- dependsOnEnvironmentSelector: { type: 'string', description: 'CEL expression to determine if the environment progression rule should be used' }, + dependsOnEnvironmentSelector: { type: 'string', description: 'CEL expression to match the environment(s) that must have a successful release before this environment can proceed' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/oapi/spec/schemas/policy.jsonnet` at line 147, Update the description for the dependsOnEnvironmentSelector schema property to clarify its purpose: it should state that this field is a CEL expression used to select the dependent environment(s) whose successful releases are required before progression, not a flag that controls whether the rule applies; locate the dependsOnEnvironmentSelector property in the policy schema and replace the current description with a concise phrase such as "CEL expression selecting dependent environment(s) whose successful releases are required before progression."apps/workspace-engine/pkg/workspace/store/diffcheck/environment_test.go (1)
176-187: Consider simplifying the selector change detection.With selectors now being simple strings, the prefix-matching logic for nested selector fields may be unnecessary. A direct key check should suffice.
Suggested simplification
changes := HasEnvironmentChanges(old, updated) assert.GreaterOrEqual(t, len(changes), 1, "Should detect resourceSelector change") - // Check if any selector-related field changed - hasResourceSelectorChange := false - for key := range changes { - if key == "resourceselector" || - len(key) > len("resourceselector") && - key[:len("resourceselector")] == "resourceselector" { - hasResourceSelectorChange = true - break - } - } - assert.True(t, hasResourceSelectorChange, "Should detect resourceSelector change") + assert.True(t, changes["resourceselector"], "Should detect resourceSelector change")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/diffcheck/environment_test.go` around lines 176 - 187, The test's selector-change detection loop (using hasResourceSelectorChange, changes and key) can be simplified: replace the prefix-matching block that checks for "resourceselector" and its prefixes with a single direct key equality check (e.g., if key == "resourceselector") since selectors are now simple strings; remove the len() and substring prefix logic and keep the assert.True on hasResourceSelectorChange unchanged.packages/trpc/src/routes/resources.ts (1)
167-182: Consider adding a default value toselectoror making it optional for clarity.The
selectorfield is defined as required (z.string()), but the filter logic on line 179 checksif (selector && selector !== "true"), suggesting it may receive empty or special values. Since"true"is the CEL expression for matching all resources (consistent with selector usage elsewhere in the codebase), consider either:
- Adding
.default("true")to the schema, or- Making it optional if callers may omit it
This would resolve the mismatch between the schema (strict) and implementation (defensive).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routes/resources.ts` around lines 167 - 182, The selector schema is declared as required (selector: z.string()) but runtime logic treats "true"/empty specially; update the schema for the input to match behavior by either changing selector: z.string() to selector: z.string().default("true") or to selector: z.string().optional(), and if making it optional ensure the destructuring in the query handler (inside the .query(async ({ input }) => { const { workspaceId, selector, kind, limit, offset } = input; ... })) supplies a fallback (e.g., selector ?? "true") so the filter construction (the IIFE that computes filter from selector and kind) remains correct.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go (1)
194-238: Dead code:evaluateJSONfunction is unreachable.This function is no longer called from anywhere in
Evaluate. Given the PR's migration to string-based CEL expressions, this JSON evaluation path appears to be vestigial code from the previousJsonSelectorsupport.🗑️ Consider removing the dead code
-// evaluateJSON evaluates a JSON-based selector. -func (e *Evaluator) evaluateJSON( - ctx context.Context, - scope evaluator.EvaluatorScope, - deployment *oapi.Deployment, - resource *oapi.Resource, - span trace.Span, -) *oapi.RuleEvaluation { - span.SetAttributes(attribute.String("selector.type", "json")) - - matched, err := selector.Match(ctx, e.rule.Selector, scope.Version) - if err != nil { - span.RecordError(err) - return results.NewDeniedResult( - fmt.Sprintf("Version selector: failed to evaluate JSON selector: %v", err), - ).WithDetail("error", err.Error()) - } - - if !matched { - description := "Version does not match selector" - if e.rule.Description != nil && *e.rule.Description != "" { - description = *e.rule.Description - } - - span.AddEvent("Version blocked by selector", - trace.WithAttributes( - attribute.Bool("selector.result", false), - )) - - return results.NewDeniedResult( - fmt.Sprintf("Version selector: %s", description), - ). - WithDetail("version_id", scope.Version.Id). - WithDetail("version_tag", scope.Version.Tag) - } - - span.AddEvent("Version allowed by selector", - trace.WithAttributes( - attribute.Bool("selector.result", true), - )) - - return results.NewAllowedResult("Version selector: version matches selector"). - WithDetail("version_id", scope.Version.Id). - WithDetail("version_tag", scope.Version.Tag) -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go` around lines 194 - 238, The evaluateJSON function is dead code after migrating to string-based CEL selectors; remove the evaluateJSON method and any related unused references (e.g., selector.Match usage, JSON selector handling, and e.rule.Selector JSON-path assumptions) and clean up imports that become unused; also search for symbols Evaluate, JsonSelector, and evaluateJSON to ensure nothing still references it and run tests/go vet to confirm no build errors remain.
🤖 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/web/app/routes/ws/jobs/_components/ResourceFilter.tsx`:
- Line 30: The selector string in ResourceFilter.tsx interpolates celDebounced
directly into the CEL expression (selector:
`resource.name.contains('${celDebounced}') ||
resource.identifier.contains('${celDebounced}')`), which allows CEL injection if
the input contains single quotes; sanitize celDebounced before building the
selector by escaping single quotes (e.g., replace all occurrences of ' with \')
or otherwise encoding the value into a safe CEL string literal, then use the
escaped/encoded value in the selector construction so user input cannot break
the CEL syntax or alter query logic.
In `@apps/workspace-engine/oapi/spec/paths/resource.jsonnet`:
- Around line 79-84: The schema marks the "filter" property as required but its
description claims a default of "true", which is contradictory; remove "filter"
from the required array in the object schema (the same object that contains the
properties.filter definition) so the server can apply the default, leaving the
existing description intact and ensuring the required array no longer contains
"filter".
In `@apps/workspace-engine/oapi/spec/paths/validate.jsonnet`:
- Around line 12-19: Update the JSON Schema for the
/v1/validate/resource-selector request so that resourceSelector is mandatory:
add a required array that includes "resourceSelector" to the schema object that
currently defines the properties (locate the object containing resourceSelector
in the validate/resource-selector request schema). Ensure the schema's required:
['resourceSelector'] is present alongside the existing type and properties so
empty bodies are rejected.
In `@apps/workspace-engine/pkg/selector/resources.go`:
- Around line 61-63: The FilterResources code conflates an omitted selector
(ResourceSelector == nil -> sel == "") with an explicit "false"; update
FilterResources (the branch that currently does: if sel == "" || sel == "false"
{ return []T{}, nil }) to only treat the literal "false" as match-none and treat
empty sel as match-all (i.e., remove the sel == "" check so empty string
defaults to true), so internal callers like GetResources,
GetResourcesByDeployment and GetResourcesByEnvironment that pass an empty sel
(from a nil ResourceSelector) will return all resources rather than none.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go`:
- Around line 178-180: The evaluator must fail fast when a rule's dependency
selector is blank: before calling selector.Match, check
e.rule.DependsOnEnvironmentSelector for an empty string and return a clear error
(e.g., "invalid rule: DependsOnEnvironmentSelector must be non-empty") instead
of proceeding; update the code around the selector.Match call in
environmentprogression.go to validate e.rule.DependsOnEnvironmentSelector and
return the error to surface bad/partially migrated policies.
In `@apps/workspace-engine/pkg/workspace/store/relationship_indexes.go`:
- Line 180: The current use of strings.ReplaceAll on sel (replaced :=
strings.ReplaceAll(*sel, string(entityType)+".", prefix+".")) will alter
occurrences inside quoted literals; update the logic that builds replaced (the
sel/entityType/prefix handling) to perform replacements only outside quoted
strings and trim surrounding whitespace — e.g., scan the sel string tracking
single/double-quote state (or use a simple tokenizer/regex that matches
identifiers like entityType followed by a dot with word boundaries) and only
substitute when not inside a quoted segment, preserving quoted content and
trimming whitespace where appropriate.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go`:
- Around line 101-107: The write-side mapping currently sets resourceSelector :=
pgtype.Text{String: "false", Valid: true} when d.ResourceSelector is nil/empty;
change this to mirror deploymentvariables/mapper.go by leaving the pgtype.Text
invalid in that case (i.e., set resourceSelector.Valid = false and do not set
String) so nil/empty ResourceSelector results in an invalid pgtype.Text; update
the logic around ResourceSelector in mapper.go (the creation of resourceSelector
from d.ResourceSelector) to implement this behavior.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go`:
- Line 61: The code dereferences e.ResourceSelector without a nil check; update
the environment-to-DB mapping (where ResourceSelector: *e.ResourceSelector is
set) to follow the deployments mapper pattern: if e.ResourceSelector is nil or
points to an empty string, set the DB ResourceSelector to the default non-null
value "false", otherwise use *e.ResourceSelector; ensure you reference and
change the assignment that sets ResourceSelector in the mapper handling
environments.
In `@packages/workspace-engine-sdk/src/schema.ts`:
- Around line 1205-1206: The type for queryResources.filter is marked required
but the JSDoc says it defaults to "true"; update the contract so they match by
making the field optional (change filter: string to filter?: string) in the
QueryResources interface/type and ensure any runtime constructors/validators
(e.g., functions that build or validate queryResources) apply the default "true"
when filter is undefined; alternatively remove the default text from the JSDoc
if you prefer to keep the field required—refer to the queryResources type
definition and any builder/validator code that references filter to keep
behavior consistent.
---
Outside diff comments:
In `@apps/api/openapi/schemas/workflows.jsonnet`:
- Around line 1-3: There are two identical import statements for the symbol
openapi; remove the duplicate so only a single import of openapi (import
'../lib/openapi.libsonnet') remains; locate the duplicated lines that declare
local openapi and delete one, leaving a single local openapi = import
'../lib/openapi.libsonnet' declaration.
In `@apps/api/src/routes/v1/workspaces/policies.ts`:
- Around line 75-81: The selector fields are being JSON.stringify'ed before
persisting (e.g., in the tx.insert call into
schema.policyRuleEnvironmentProgression where dependsOnEnvironmentSelector is
set via JSON.stringify), causing stored values to be quoted/encoded; remove the
JSON.stringify wrappers and write the selector values directly (pass the raw
object/array) so the DB stores the native JSON shape; apply the same change to
the other insert/upsert sites in this file that stringify selector fields (the
similar blocks that set dependsOnEnvironmentSelector / environmentSelector /
targetEnvironmentSelector) to ensure consistent shape on read and write.
In `@apps/web/app/routes/ws/resources.tsx`:
- Line 57: The CEL expression built into cleanedCel interpolates raw
searchDebounced allowing single-quote injection; escape or sanitize
searchDebounced before embedding it into the string so quotes and other
CEL-special characters are neutralized (same approach used in
ResourceFilter.tsx). Locate the cleanedCel assignment in resources.tsx and
replace the direct interpolation with a sanitized/escapedSearchDebounced value
(or use a helper like escapeCelString) so the final expression becomes safe,
e.g., ensure any single quotes in searchDebounced are escaped and input is
validated before constructing resource.name.contains(...) ||
resource.identifier.contains(...).
In
`@apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager_bench_test.go`:
- Around line 1283-1292: The DeploymentVariableValue instances created in the
benchmark are missing the ResourceSelector field, so they will be skipped like
in other benchmarks; update the block that constructs deploymentVarValue (the
oapi.DeploymentVariableValue being Upserted via
st.DeploymentVariableValues.Upsert) to populate the ResourceSelector (e.g.,
reference the intended resource or selector used elsewhere in tests) before
calling Upsert — ensure the created value includes ResourceSelector alongside
Id, DeploymentVariableId, Priority, and Value so it behaves consistently with
other benchmarks.
- Around line 1033-1042: The test is creating DeploymentVariableValue objects
without setting ResourceSelector, so these reference values will be skipped
during selector matching; update the code that builds deploymentVarValue (the
DeploymentVariableValue struct created in the loop before calling
st.DeploymentVariableValues.Upsert) to populate the ResourceSelector field with
an appropriate oapi.ResourceSelector that matches the deployment under test (or
explicitly set the intended selector values for the scenario), ensuring the
selector covers the ResourceType/ResourceId (or other selector fields) used by
your selector matching logic.
- Around line 231-239: The test is inserting a DeploymentVariableValue (created
with refValueID and linked to refVarID) but lacks a ResourceSelector so
tryResolveDeploymentVariableValue will skip it; update the test to set
DeploymentVariableValue.ResourceSelector to match the test resource (e.g., the
same resource type/name/labels used in the bench) before calling
st.DeploymentVariableValues.Upsert, or else adjust the benchmark assertions to
acknowledge this value is intentionally skipped.
- Around line 716-723: The DeploymentVariableValue created for valueID
(variableID variable) is missing ResourceSelector, so it will be skipped by the
nil check during selector matching; update the deploymentVarValue construction
(the oapi.DeploymentVariableValue instance created before calling
st.DeploymentVariableValues.Upsert) to set ResourceSelector to the same non-nil
selector used for other test values (mirror the fix applied around the earlier
block at lines 231-239), ensuring the ResourceSelector field is populated with
the appropriate oapi.ResourceSelector object before calling Upsert.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager.go`:
- Around line 199-206: The loop in variablemanager.go currently skips entries
where value.ResourceSelector == nil, causing inconsistent behavior with
variableresolver.resolve.go which includes nil selectors; update the logic in
the function containing this snippet to treat nil ResourceSelector as a match
(i.e., include those values in sortedValues) or, if a different policy is
intended, add a clear comment and unit tests; specifically adjust the branch
around ResourceSelector, selector.Match(ctx, *value.ResourceSelector, resource)
and sortedValues so that DeploymentVariableValue instances with nil selectors
are appended just like in resolve.go.
In `@packages/workspace-engine-sdk/src/schema.ts`:
- Around line 94-97: Remove the legacy selector component types from the OpenAPI
schema by deleting the CelSelector, JsonSelector, and Selector component
definitions and any references to them in the generated schema (look for the
symbols CelSelector, JsonSelector, Selector inside the components/definitions in
schema.ts and any usages in operation/request/response schemas). After removing
those entries, re-run the OpenAPI schema generation/regeneration step so the
TypeScript output is refreshed and no longer exports those deprecated types;
ensure no other generated types still reference them and update any dependent
schemas to use the new CEL-based selector type.
---
Nitpick comments:
In `@apps/workspace-engine/oapi/spec/schemas/policy.jsonnet`:
- Line 147: Update the description for the dependsOnEnvironmentSelector schema
property to clarify its purpose: it should state that this field is a CEL
expression used to select the dependent environment(s) whose successful releases
are required before progression, not a flag that controls whether the rule
applies; locate the dependsOnEnvironmentSelector property in the policy schema
and replace the current description with a concise phrase such as "CEL
expression selecting dependent environment(s) whose successful releases are
required before progression."
In `@apps/workspace-engine/pkg/db/convert_test.go`:
- Line 168: The test's name still uses the suffix "JSONCelFormat" but its input
(the value passed at line with {"id": ruleID, "description": "test", "selector":
celExpr}) is a plain CEL string, so rename the test case/function that contains
"JSONCelFormat" to a name that reflects a plain CEL string input (e.g.,
"PlainCelFormat" or "CelStringFormat") to avoid confusing coverage signals;
update any related subtest names or table-driven test entry keys that reference
"JSONCelFormat" (search for the exact "JSONCelFormat" identifier) so test names
accurately match the input format.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go`:
- Around line 584-585: The test repeats the CEL selector string
"environment.name == 'staging'" across multiple cases; extract it to a single
file-level constant (e.g., stagingSelector) in gradualrollout_test.go and
replace all local uses of the variable named selector with that constant
(affecting occurrences around the selector assignments in the tests). Ensure the
constant is declared near the top of the test file so all test functions (the
selector variable usages at lines referenced) import the same value and update
any comments or test helper references accordingly.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go`:
- Around line 194-238: The evaluateJSON function is dead code after migrating to
string-based CEL selectors; remove the evaluateJSON method and any related
unused references (e.g., selector.Match usage, JSON selector handling, and
e.rule.Selector JSON-path assumptions) and clean up imports that become unused;
also search for symbols Evaluate, JsonSelector, and evaluateJSON to ensure
nothing still references it and run tests/go vet to confirm no build errors
remain.
In `@apps/workspace-engine/pkg/workspace/store/diffcheck/environment_test.go`:
- Around line 176-187: The test's selector-change detection loop (using
hasResourceSelectorChange, changes and key) can be simplified: replace the
prefix-matching block that checks for "resourceselector" and its prefixes with a
single direct key equality check (e.g., if key == "resourceselector") since
selectors are now simple strings; remove the len() and substring prefix logic
and keep the assert.True on hasResourceSelectorChange unchanged.
In `@apps/workspace-engine/pkg/workspace/store/relationship_indexes.go`:
- Around line 173-176: Update the doc comment for selectorToCel to reflect that
it now accepts raw CEL text as a *string (not a Selector) and that it transforms
that CEL expression by replacing the entity-type variable (e.g., "resource",
"deployment", "environment") with the provided prefix ("from" or "to"), keeping
mention of parameters entityType (oapi.RelatableEntityType) and prefix so
callers understand the contract and behavior of selectorToCel.
In `@packages/trpc/src/routes/resources.ts`:
- Around line 167-182: The selector schema is declared as required (selector:
z.string()) but runtime logic treats "true"/empty specially; update the schema
for the input to match behavior by either changing selector: z.string() to
selector: z.string().default("true") or to selector: z.string().optional(), and
if making it optional ensure the destructuring in the query handler (inside the
.query(async ({ input }) => { const { workspaceId, selector, kind, limit, offset
} = input; ... })) supplies a fallback (e.g., selector ?? "true") so the filter
construction (the IIFE that computes filter from selector and kind) remains
correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8446606-6609-41bb-bb1d-f7808fe293e1
📒 Files selected for processing (63)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/core.jsonnetapps/api/openapi/schemas/policies.jsonnetapps/api/openapi/schemas/workflows.jsonnetapps/api/src/routes/v1/workspaces/policies.tsapps/api/src/routes/v1/workspaces/resources.tsapps/api/src/types/openapi.tsapps/web/app/api/openapi.tsapps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsxapps/web/app/routes/ws/environments/page.$environmentId.resources.tsxapps/web/app/routes/ws/jobs/_components/ResourceFilter.tsxapps/web/app/routes/ws/resources.tsxapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/paths/resource.jsonnetapps/workspace-engine/oapi/spec/paths/validate.jsonnetapps/workspace-engine/oapi/spec/schemas/core.jsonnetapps/workspace-engine/oapi/spec/schemas/deployments.jsonnetapps/workspace-engine/oapi/spec/schemas/environments.jsonnetapps/workspace-engine/oapi/spec/schemas/policy.jsonnetapps/workspace-engine/oapi/spec/schemas/relationship.jsonnetapps/workspace-engine/oapi/spec/schemas/workflows.jsonnetapps/workspace-engine/pkg/db/convert.goapps/workspace-engine/pkg/db/convert_test.goapps/workspace-engine/pkg/oapi/hash.goapps/workspace-engine/pkg/oapi/hash_test.goapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/selector/langs/jsonselector/unknown/unknown.goapps/workspace-engine/pkg/selector/match.goapps/workspace-engine/pkg/selector/match_test.goapps/workspace-engine/pkg/selector/resources.goapps/workspace-engine/pkg/selector/resources_test.goapps/workspace-engine/pkg/workspace/jobs/factory_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/mock_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector_test.goapps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager.goapps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager_bench_test.goapps/workspace-engine/pkg/workspace/releasemanager/variables/variablemanager_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/scheduler_test.goapps/workspace-engine/pkg/workspace/store/deployments.goapps/workspace-engine/pkg/workspace/store/diffcheck/deployment_test.goapps/workspace-engine/pkg/workspace/store/diffcheck/environment_test.goapps/workspace-engine/pkg/workspace/store/environments.goapps/workspace-engine/pkg/workspace/store/relationship_indexes.goapps/workspace-engine/pkg/workspace/store/release_targets_test.goapps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.goapps/workspace-engine/pkg/workspace/store/resources.goapps/workspace-engine/pkg/workspace/store/restore_test.goapps/workspace-engine/pkg/workspace/store/store.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.goapps/workspace-engine/svc/http/server/openapi/resources/server.goapps/workspace-engine/svc/http/server/openapi/validators/validators.goapps/workspace-engine/test/controllers/harness/pipeline_opts.gopackages/trpc/src/routes/resources.tspackages/workspace-engine-sdk/src/schema.ts
💤 Files with no reviewable changes (3)
- apps/api/openapi/schemas/core.jsonnet
- apps/workspace-engine/pkg/selector/langs/jsonselector/unknown/unknown.go
- apps/workspace-engine/oapi/spec/schemas/core.jsonnet
Comment on lines
61
to
63
| // If no selector is provided, return empty slice | ||
| if sel == nil { | ||
| if sel == "" || sel == "false" { | ||
| return []T{}, nil |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Selector filtering call sites:"
rg -n -C3 '\b(FilterResources|Filter)\s*\(' apps/workspace-engine
echo
echo "Blank-selector/default handling before those calls:"
rg -n -C3 'sel := ""|Filter\s*==\s*""|Filter\s*=\s*"true"|TrimSpace\(.*Filter|TrimSpace\(.*Selector|QueryResourcesJSONBody|QueryResources\(|ShouldBindJSON|BindJSON|json.NewDecoder' apps/workspace-engineRepository: ctrlplanedev/ctrlplane
Length of output: 34698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== HTTP Handler: QueryResources after ShouldBindJSON ==="
sed -n '17,50p' apps/workspace-engine/svc/http/server/openapi/resources/server.go
echo
echo "=== Filter function implementation ==="
sed -n '50,100p' apps/workspace-engine/pkg/selector/resources.go
echo
echo "=== CEL Evaluator handling of empty/blank selector ==="
rg -n -C5 'evaluateSelector|evalCEL|NewEnv|Parse.*selector' apps/workspace-engine/pkg/selector/ | head -60Repository: ctrlplanedev/ctrlplane
Length of output: 3710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Deployments context: nil ResourceSelector semantics ==="
sed -n '85,130p' apps/workspace-engine/pkg/workspace/store/deployments.go
echo
echo "=== Environments context: nil ResourceSelector semantics ==="
sed -n '75,110p' apps/workspace-engine/pkg/workspace/store/environments.goRepository: ctrlplanedev/ctrlplane
Length of output: 2260
Keep omitted selectors (nil ResourceSelector) distinct from explicit "false".
The HTTP handler correctly normalizes blank filter to "true" before calling GetResources, but internal callers like GetResourcesByDeployment and GetResourcesByEnvironment pass blank selectors directly. When a Deployment or Environment has ResourceSelector = nil (omitted), the current code sets sel := "" and passes it to FilterResources, which treats it as "false" and returns an empty slice. This silently changes the semantics: a missing selector should match all resources (defaulting to "true"), not none.
The blank-to-false conversion in Filter conflates two distinct cases. Either:
- Change
Filterto not treat""as an optimization, or - Ensure all callers normalize omitted selectors to
"true"before invokingFilter
Currently the code is inconsistent between the HTTP API (which gets it right) and internal store methods (which are broken).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/selector/resources.go` around lines 61 - 63, The
FilterResources code conflates an omitted selector (ResourceSelector == nil ->
sel == "") with an explicit "false"; update FilterResources (the branch that
currently does: if sel == "" || sel == "false" { return []T{}, nil }) to only
treat the literal "false" as match-none and treat empty sel as match-all (i.e.,
remove the sel == "" check so empty string defaults to true), so internal
callers like GetResources, GetResourcesByDeployment and
GetResourcesByEnvironment that pass an empty sel (from a nil ResourceSelector)
will return all resources rather than none.
Member
Author
There was a problem hiding this comment.
we want nil or empty to be false
Contributor
There was a problem hiding this comment.
@adityachoudhari26, understood! The behavior is intentional — an omitted or empty selector means "match nothing" (effectively false). I'll keep that in mind for future reviews.
✏️ Learnings added
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 845
File: apps/workspace-engine/pkg/selector/resources.go:61-63
Timestamp: 2026-03-12T22:10:06.625Z
Learning: In `apps/workspace-engine/pkg/selector/resources.go`, an empty string (`""`) selector is intentionally treated the same as `"false"` — both mean "match no resources". A nil or omitted ResourceSelector (converted to `""`) is designed to return an empty result, not match all resources. Do not flag this as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Include edge cases in tests (empty values, special characters, unicode) for condition types
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Test validation and matching logic separately for condition types
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 237
File: apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx:43-50
Timestamp: 2024-11-27T23:16:35.580Z
Learning: In `DeploymentResourceDrawer.tsx`, the `isOpen` variable already checks whether `deploymentId`, `environmentId`, and `resourceId` are non-null, so additional null checks in query `enabled` conditions are not necessary.
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/types.go : Add the new condition type to pkg/model/selector/types.go
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 818
File: apps/workspace-engine/pkg/db/queries/schema.sql:252-282
Timestamp: 2026-02-26T23:01:30.641Z
Learning: User adityachoudhari26 prefers to defer adding database indexes on foreign keys until there's evidence they're needed, considering such additions premature optimization at the initial schema design stage.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 685
File: apps/workspace-engine/pkg/db/common.go:47-57
Timestamp: 2025-10-16T20:59:56.459Z
Learning: In the ctrlplane workspace-engine db package, CEL selectors are intentionally not supported and are written as NULL to the database. This is documented with TODO comments for future implementation.
...g/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go
Show resolved
Hide resolved
| // Replace the entity type variable name with the from/to prefix. | ||
| // e.g. "resource.kind == 'vpc'" → "from.kind == 'vpc'" | ||
| replaced := strings.ReplaceAll(cs.Cel, string(entityType)+".", prefix+".") | ||
| replaced := strings.ReplaceAll(*sel, string(entityType)+".", prefix+".") |
Contributor
There was a problem hiding this comment.
ReplaceAll on raw CEL can corrupt string literals (Line 180).
This replaces every resource./deployment./environment. occurrence, including inside quoted literals, which can silently change rule meaning and produce incorrect matches.
💡 Safer replacement (skip quoted strings, trim whitespace)
func selectorToCel(sel *string, entityType oapi.RelatableEntityType, prefix string) string {
- if sel == nil || *sel == "" {
+ if sel == nil {
return ""
}
- replaced := strings.ReplaceAll(*sel, string(entityType)+".", prefix+".")
- return replaced
+ expr := strings.TrimSpace(*sel)
+ if expr == "" {
+ return ""
+ }
+ return replaceEntityPrefixOutsideStrings(expr, string(entityType), prefix)
}
+
+func replaceEntityPrefixOutsideStrings(expr, from, to string) string {
+ needle := from + "."
+ repl := to + "."
+
+ var b strings.Builder
+ inString := false
+ escaped := false
+
+ for i := 0; i < len(expr); {
+ ch := expr[i]
+ if inString {
+ b.WriteByte(ch)
+ if escaped {
+ escaped = false
+ } else if ch == '\\' {
+ escaped = true
+ } else if ch == '"' {
+ inString = false
+ }
+ i++
+ continue
+ }
+
+ if ch == '"' {
+ inString = true
+ b.WriteByte(ch)
+ i++
+ continue
+ }
+
+ if strings.HasPrefix(expr[i:], needle) && (i == 0 || !isIdentChar(expr[i-1])) {
+ b.WriteString(repl)
+ i += len(needle)
+ continue
+ }
+
+ b.WriteByte(ch)
+ i++
+ }
+ return b.String()
+}
+
+func isIdentChar(c byte) bool {
+ return c == '_' ||
+ (c >= 'a' && c <= 'z') ||
+ (c >= 'A' && c <= 'Z') ||
+ (c >= '0' && c <= '9')
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/relationship_indexes.go` at line
180, The current use of strings.ReplaceAll on sel (replaced :=
strings.ReplaceAll(*sel, string(entityType)+".", prefix+".")) will alter
occurrences inside quoted literals; update the logic that builds replaced (the
sel/entityType/prefix handling) to perform replacements only outside quoted
strings and trim surrounding whitespace — e.g., scan the sel string tracking
single/double-quote state (or use a simple tokenizer/regex that matches
identifiers like entityType followed by a dot with word boundaries) and only
substitute when not inside a quoted segment, preserving quoted content and
trimming whitespace where appropriate.
Member
Author
There was a problem hiding this comment.
it was like this in pre-refactor
Contributor
There was a problem hiding this comment.
@adityachoudhari26, you're right — the pre-refactor code already applied strings.ReplaceAll directly on the raw CEL string (cs.Cel). This PR preserves the same behavior; the potential string-literal corruption was a pre-existing concern, not something introduced here. I'll withdraw the issue for this PR.
Would you like me to open a follow-up issue to track improving the replacement logic (e.g., skipping quoted literals) as a separate task?
✏️ Learnings added
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 845
File: apps/workspace-engine/pkg/workspace/store/relationship_indexes.go:180-180
Timestamp: 2026-03-12T22:11:16.776Z
Learning: In `apps/workspace-engine/pkg/workspace/store/relationship_indexes.go`, the `selectorToCel` function uses `strings.ReplaceAll` to replace entity-type prefixes (e.g., "resource.", "deployment.") in raw CEL expressions. This behavior was present pre-refactor and is intentionally preserved. Do not flag it as a new issue in this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/types.go : Add the new condition type to pkg/model/selector/types.go
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Include edge cases in tests (empty values, special characters, unicode) for condition types
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Test validation and matching logic separately for condition types
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 845
File: apps/workspace-engine/pkg/selector/resources.go:61-63
Timestamp: 2026-03-12T22:10:06.625Z
Learning: In `apps/workspace-engine/pkg/selector/resources.go`, an empty string (`""`) selector is intentionally treated the same as `"false"` — both mean "match no resources". A nil or omitted ResourceSelector (converted to `""`) is designed to return an empty result, not match all resources. Do not flag this as a bug.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 818
File: apps/workspace-engine/pkg/db/queries/schema.sql:252-282
Timestamp: 2026-02-26T23:01:30.641Z
Learning: User adityachoudhari26 prefers to defer adding database indexes on foreign keys until there's evidence they're needed, considering such additions premature optimization at the initial schema design stage.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 685
File: apps/workspace-engine/pkg/db/common.go:47-57
Timestamp: 2025-10-16T20:59:56.459Z
Learning: In the ctrlplane workspace-engine db package, CEL selectors are intentionally not supported and are written as NULL to the database. This is documented with TODO comments for future implementation.
Comment on lines
+101
to
107
| var selStr string | ||
| if d.ResourceSelector == nil || *d.ResourceSelector == "" { | ||
| selStr = "false" | ||
| } else { | ||
| selStr = *d.ResourceSelector | ||
| } | ||
| resourceSelector := pgtype.Text{String: selStr, Valid: true} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other mappers handle nil/empty selectors
rg -n -A5 "ResourceSelector" apps/workspace-engine/pkg/workspace/store/repository/db/*/mapper.goRepository: ctrlplanedev/ctrlplane
Length of output: 7040
🏁 Script executed:
# Check how ResourceSelector is used/evaluated in the codebase
rg -n "false.*selector|selector.*false" apps/workspace-engine/ --type-list | head -20
rg -n "parse.*selector|selector.*parse" apps/workspace-engine/ --type-list | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 845
🏁 Script executed:
# Check if there's selector validation or evaluation logic
fd -e go -path "*/selector*" apps/workspace-engine/ | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 234
🏁 Script executed:
# Search for selector evaluation/parsing logic
rg -n "ResourceSelector" apps/workspace-engine/pkg/workspace/store/repository/db/ -A 3 | head -40Repository: ctrlplanedev/ctrlplane
Length of output: 3839
🏁 Script executed:
# Look for selector package or model to understand the format
fd selector apps/workspace-engine/ --type fRepository: ctrlplanedev/ctrlplane
Length of output: 610
🏁 Script executed:
# Check if there's any selector validation or format documentation
rg -n "false" apps/workspace-engine/pkg/model/selector/ 2>/dev/null | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check the resource selector tests to understand how selectors are evaluated
cat apps/workspace-engine/test/controllers/resource_selector_test.goRepository: ctrlplanedev/ctrlplane
Length of output: 28636
🏁 Script executed:
# Look at deploymentselector to understand how deployment selectors work
cat apps/workspace-engine/pkg/reconcile/events/deploymentselector.goRepository: ctrlplanedev/ctrlplane
Length of output: 1287
Inconsistency confirmed: the "false" default differs from how other mappers handle nil/empty selectors.
The write side defaults nil/empty to "false", but the read side (lines 47-49) only checks the Valid flag and does not handle "false" specially. In contrast, deploymentvariables/mapper.go leaves pgtype.Text in an invalid state for nil input, and the read side explicitly checks Valid && String != "" before assigning. Align with the deploymentvariables approach by using an invalid pgtype.Text when ResourceSelector is nil or empty, rather than defaulting to the string "false".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go`
around lines 101 - 107, The write-side mapping currently sets resourceSelector
:= pgtype.Text{String: "false", Valid: true} when d.ResourceSelector is
nil/empty; change this to mirror deploymentvariables/mapper.go by leaving the
pgtype.Text invalid in that case (i.e., set resourceSelector.Valid = false and
do not set String) so nil/empty ResourceSelector results in an invalid
pgtype.Text; update the logic around ResourceSelector in mapper.go (the creation
of resourceSelector from d.ResourceSelector) to implement this behavior.
Member
Author
There was a problem hiding this comment.
same commetn as above
Contributor
There was a problem hiding this comment.
@adityachoudhari26, understood — the "false" default for nil/empty ResourceSelector in deployments/mapper.go is intentional by design, consistent with the selector engine treating both "" and "false" as "match no resources." My review comment was incorrect. I'll extend the existing learning to cover this file as well.
✏️ Learnings added
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 845
File: apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go:101-107
Timestamp: 2026-03-12T22:11:31.674Z
Learning: In `apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go`, defaulting a nil or empty `ResourceSelector` to the string `"false"` (as a valid `pgtype.Text`) when writing to the database is intentional. The selector engine treats both `""` and `"false"` as "match no resources", so this is by design — do not flag it as an inconsistency or suggest using an invalid `pgtype.Text` instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 845
File: apps/workspace-engine/pkg/selector/resources.go:61-63
Timestamp: 2026-03-12T22:10:06.625Z
Learning: In `apps/workspace-engine/pkg/selector/resources.go`, an empty string (`""`) selector is intentionally treated the same as `"false"` — both mean "match no resources". A nil or omitted ResourceSelector (converted to `""`) is designed to return an empty result, not match all resources. Do not flag this as a bug.
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/types.go : Add the new condition type to pkg/model/selector/types.go
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Include edge cases in tests (empty values, special characters, unicode) for condition types
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Test validation and matching logic separately for condition types
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 673
File: apps/workspace-engine/pkg/db/deployment_variables.go:51-65
Timestamp: 2025-10-07T16:44:54.938Z
Learning: In `apps/workspace-engine/pkg/db/deployment_variables.go`, the `default_value_id` field is intentionally scanned from the database but not assigned to the `DefaultValueId` field in the proto struct. This is a temporary decision and the field assignment is deferred for later implementation.
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Write comprehensive, data-driven tests for new condition types
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Use table-driven tests for all condition types
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 818
File: apps/workspace-engine/pkg/db/queries/schema.sql:252-282
Timestamp: 2026-02-26T23:01:30.641Z
Learning: User adityachoudhari26 prefers to defer adding database indexes on foreign keys until there's evidence they're needed, considering such additions premature optimization at the initial schema design stage.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 685
File: apps/workspace-engine/pkg/db/common.go:47-57
Timestamp: 2025-10-16T20:59:56.459Z
Learning: In the ctrlplane workspace-engine db package, CEL selectors are intentionally not supported and are written as NULL to the database. This is documented with TODO comments for future implementation.
apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go (1)
3-12:⚠️ Potential issue | 🟡 MinorFix import formatting to resolve golangci-lint failure.
The pipeline reports a
gciformatting violation at line 9. Rungci writeor your configured formatter to fix the import grouping/ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go` around lines 3 - 12, The import block in mapper.go is misformatted causing a gci lint failure; reorder/group the imports for standard library, third-party, and internal packages (e.g., keep "encoding/json" and "fmt" together, then "github.com/..." entries like "github.com/google/uuid" and "github.com/jackc/pgx/v5/pgtype", and finally local imports "workspace-engine/pkg/db" and "workspace-engine/pkg/oapi"), or simply run your configured formatter (e.g., gci write) to automatically fix the grouping/ordering so the import block compiles cleanly.
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go (1)
61-61:⚠️ Potential issue | 🟠 MajorNil pointer dereference when
ResourceSelectoris nil.
e.ResourceSelectoris*stringand can be nil, but this line dereferences it directly. Follow the pattern fromdeployments/mapper.go(lines 101-107) to handle nil/empty cases.🛡️ Proposed fix
+ var resourceSelector string + if e.ResourceSelector == nil || *e.ResourceSelector == "" { + resourceSelector = "false" + } else { + resourceSelector = *e.ResourceSelector + } + return db.UpsertEnvironmentParams{ ID: id, Name: e.Name, Description: description, - ResourceSelector: *e.ResourceSelector, + ResourceSelector: resourceSelector, Metadata: metadata, WorkspaceID: uuid.Nil, // set by caller CreatedAt: createdAt, }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go` at line 61, The code dereferences e.ResourceSelector directly (ResourceSelector: *e.ResourceSelector) which can be nil; change the mapping to mirror deployments/mapper.go by checking e.ResourceSelector for nil/empty and only dereferencing when non-nil—otherwise assign the zero/empty value expected by the target type (e.g., empty string or nil as used in deployments/mapper.go). Update the mapping in mapper.go to use a conditional branch around e.ResourceSelector so you don't perform *e.ResourceSelector when it's nil.
🧹 Nitpick comments (5)
apps/workspace-engine/pkg/db/convert_test.go (1)
152-184: Test name no longer matches what is being validated.At Line 168 this test now uses a plain CEL string, so
TestToOapiPolicyWithRules_VersionSelectorJSONCelFormatno longer exercises a JSON-CEL format path. Please rename/remove this case (or restore a true JSON-format fixture if that behavior is still expected).Minimal rename diff
-func TestToOapiPolicyWithRules_VersionSelectorJSONCelFormat(t *testing.T) { +func TestToOapiPolicyWithRules_VersionSelectorCELString(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/convert_test.go` around lines 152 - 184, The test named TestToOapiPolicyWithRules_VersionSelectorJSONCelFormat no longer uses a JSON-CEL fixture (it passes a plain CEL string in VersionSelectorRules), so rename the test to reflect the actual behavior (e.g., TestToOapiPolicyWithRules_VersionSelectorPlainCel) or replace the VersionSelectorRules payload with a true JSON-CEL formatted fixture; locate the test function by its current name and the use of ToOapiPolicyWithRules and celExpr to update the name or restore the JSON structure accordingly so the test name and assertion align.apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go (1)
1512-1517: UsingInEpsilonwith epsilon=0 is misleading.
assert.InEpsilonwith an epsilon of0provides no tolerance and is functionally equivalent to an exact equality check. This is confusing becauseInEpsilonimplies tolerance-based comparison.If exact equality is intended, prefer
assert.Equalfor clarity. If approximate floating-point comparison is needed, use a meaningful epsilon value (e.g.,1e-9).♻️ Suggested fix: revert to Equal for exact comparisons
- assert.InEpsilon(t, float64(123), cfg["installationId"], 0, "agent-level key should survive") + assert.Equal(t, float64(123), cfg["installationId"], "agent-level key should survive") assert.Equal(t, "myorg", cfg["owner"], "agent-level key should survive") assert.Equal(t, "myrepo", cfg["repo"], "agent-level key should survive") - assert.InEpsilon(t, float64(456), cfg["workflowId"], 0, "deployment-level key should be merged in") + assert.Equal(t, float64(456), cfg["workflowId"], "deployment-level key should be merged in") assert.Equal(t, "v1.2.3", cfg["image"], "version-level key should be merged in") - assert.InEpsilon(t, float64(90), cfg["timeout"], 0, "version-level should win over deployment and agent") + assert.Equal(t, float64(90), cfg["timeout"], "version-level should win over deployment and agent")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go` around lines 1512 - 1517, The test uses assert.InEpsilon(..., 0) which is misleading; change the two assert.InEpsilon calls comparing integer-like values (assert.InEpsilon(t, float64(123), cfg["installationId"], 0, ...) and assert.InEpsilon(t, float64(456), cfg["workflowId"], 0, ...)) to assert.Equal (or assert.EqualValues) for exact comparison against cfg["installationId"] and cfg["workflowId"]; keep the assert.InEpsilon usage only if you need a true tolerance and then supply a meaningful epsilon (e.g., 1e-9) for numeric comparisons like timeout, otherwise replace the timeout check assert.InEpsilon(t, float64(90), cfg["timeout"], 0, ...) with assert.Equal as well.apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go (1)
3-11: Fix import formatting to match gci requirements.Similar to the deployments mapper, this file likely has the same import formatting issue. Run your formatter to ensure consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go` around lines 3 - 11, The imports in mapper.go are not grouped/ordered per gci style; reorder and reformat the import block to follow gci (standard library first: "fmt", then third-party libs like "github.com/google/uuid" and "github.com/jackc/pgx/v5/pgtype", then internal modules "workspace-engine/pkg/db" and "workspace-engine/pkg/oapi"), remove extra blank lines, and run the project formatter (gofmt/gci) so the import block in mapper.go matches the deployments mapper formatting.apps/workspace-engine/pkg/selector/resources_test.go (2)
723-819: Consider adding an error case for invalid CEL syntax.The edge cases test covers empty lists, no matches, all matches, and boolean literals (
true/false), but there's no test case withwantErr: truefor malformed CEL expressions (e.g.,"resource.name.invalidMethod()"or syntax errors like"resource.name == "). Adding such a case would improve confidence that the error path is exercised.Based on learnings: "Include edge cases in tests (empty values, special characters, unicode) for condition types."
💡 Example test case to add
{ name: "invalid CEL syntax returns error", selector: "resource.name.invalidMethod()", resources: []*oapi.Resource{{Id: "1", Name: "test"}}, expectedCount: 0, wantErr: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/selector/resources_test.go` around lines 723 - 819, Add a test case to TestFilterResources_EmptyAndEdgeCases that supplies a malformed/unsupported CEL selector (e.g. "resource.name.invalidMethod()" or a syntax error) and sets wantErr: true so the FilterResources(ctx, selector, resources) error path is exercised; update the tests slice in resources_test.go to include an entry named like "invalid CEL syntax returns error" with a simple resource (e.g., &oapi.Resource{Id:"1", Name:"test"}) and expectedCount 0; run the test to ensure FilterResources (the function under test) returns an error for the invalid selector.
626-663: Consider adding tests forConfigfield access or renaming the test.The test function is named
TestFilterResources_ConfigFieldConditionsand definesconfig1/config2maps that are assigned to resources, but the selectors only testresource.versionandresource.identifierfields—not theConfigmap itself.If CEL expressions can access
resource.config['replicas'], consider adding a test case for that. Otherwise, renaming toTestFilterResources_VersionAndIdentifierConditionswould better reflect the actual coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/selector/resources_test.go` around lines 626 - 663, The test function TestFilterResources_ConfigFieldConditions defines config1/config2 but never asserts on them—either add a test case that exercises resource.config access or rename the test to reflect it checks only version/identifier; to fix, add a new test entry in the tests slice that uses selector like resource.config['replicas']=='3' (or another appropriate CEL expression) and expects the resource with config1 (Id "1") to match, referencing the existing config1/config2 and the TestFilterResources_ConfigFieldConditions name so the added case is colocated; alternatively, if you prefer not to test config access, rename the function to TestFilterResources_VersionAndIdentifierConditions and update the test name strings to match.
🤖 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/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go`:
- Around line 82-86: The error path that currently returns
results.NewDeniedResult(fmt.Sprintf("Version selector: failed to parse selector:
%v", celSelector)) should special-case an empty selector value: if celSelector
== "" (the required Selector field is missing/empty) return a denied result with
a clear message like "Version selector: selector is required but was empty" (and
keep the .WithDetail("selector", celSelector)); otherwise preserve the existing
parse-failure message. Update the logic around the parse/validation in
versionselector.go (the code that handles celSelector and calls
results.NewDeniedResult) to perform this empty-check before treating it as a
generic parse error.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.go`:
- Around line 105-107: Normalize empty resource selectors by writing SQL NULL
instead of an empty string: when mapping e.ResourceSelector into pgtype.Text (in
mapper.go), check if the pointer is nil or points to an empty string and set
pgtype.Text.Valid = false in that case; only set Valid = true and assign String
when the pointer is non-nil and the string is non-empty. This will make the
write behavior consistent with ValueToOapi's treatment of empty DB selectors as
nil.
---
Outside diff comments:
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.go`:
- Around line 3-12: The import block in mapper.go is misformatted causing a gci
lint failure; reorder/group the imports for standard library, third-party, and
internal packages (e.g., keep "encoding/json" and "fmt" together, then
"github.com/..." entries like "github.com/google/uuid" and
"github.com/jackc/pgx/v5/pgtype", and finally local imports
"workspace-engine/pkg/db" and "workspace-engine/pkg/oapi"), or simply run your
configured formatter (e.g., gci write) to automatically fix the
grouping/ordering so the import block compiles cleanly.
---
Duplicate comments:
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go`:
- Line 61: The code dereferences e.ResourceSelector directly (ResourceSelector:
*e.ResourceSelector) which can be nil; change the mapping to mirror
deployments/mapper.go by checking e.ResourceSelector for nil/empty and only
dereferencing when non-nil—otherwise assign the zero/empty value expected by the
target type (e.g., empty string or nil as used in deployments/mapper.go). Update
the mapping in mapper.go to use a conditional branch around e.ResourceSelector
so you don't perform *e.ResourceSelector when it's nil.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/db/convert_test.go`:
- Around line 152-184: The test named
TestToOapiPolicyWithRules_VersionSelectorJSONCelFormat no longer uses a JSON-CEL
fixture (it passes a plain CEL string in VersionSelectorRules), so rename the
test to reflect the actual behavior (e.g.,
TestToOapiPolicyWithRules_VersionSelectorPlainCel) or replace the
VersionSelectorRules payload with a true JSON-CEL formatted fixture; locate the
test function by its current name and the use of ToOapiPolicyWithRules and
celExpr to update the name or restore the JSON structure accordingly so the test
name and assertion align.
In `@apps/workspace-engine/pkg/selector/resources_test.go`:
- Around line 723-819: Add a test case to TestFilterResources_EmptyAndEdgeCases
that supplies a malformed/unsupported CEL selector (e.g.
"resource.name.invalidMethod()" or a syntax error) and sets wantErr: true so the
FilterResources(ctx, selector, resources) error path is exercised; update the
tests slice in resources_test.go to include an entry named like "invalid CEL
syntax returns error" with a simple resource (e.g., &oapi.Resource{Id:"1",
Name:"test"}) and expectedCount 0; run the test to ensure FilterResources (the
function under test) returns an error for the invalid selector.
- Around line 626-663: The test function
TestFilterResources_ConfigFieldConditions defines config1/config2 but never
asserts on them—either add a test case that exercises resource.config access or
rename the test to reflect it checks only version/identifier; to fix, add a new
test entry in the tests slice that uses selector like
resource.config['replicas']=='3' (or another appropriate CEL expression) and
expects the resource with config1 (Id "1") to match, referencing the existing
config1/config2 and the TestFilterResources_ConfigFieldConditions name so the
added case is colocated; alternatively, if you prefer not to test config access,
rename the function to TestFilterResources_VersionAndIdentifierConditions and
update the test name strings to match.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go`:
- Around line 3-11: The imports in mapper.go are not grouped/ordered per gci
style; reorder and reformat the import block to follow gci (standard library
first: "fmt", then third-party libs like "github.com/google/uuid" and
"github.com/jackc/pgx/v5/pgtype", then internal modules
"workspace-engine/pkg/db" and "workspace-engine/pkg/oapi"), remove extra blank
lines, and run the project formatter (gofmt/gci) so the import block in
mapper.go matches the deployments mapper formatting.
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go`:
- Around line 1512-1517: The test uses assert.InEpsilon(..., 0) which is
misleading; change the two assert.InEpsilon calls comparing integer-like values
(assert.InEpsilon(t, float64(123), cfg["installationId"], 0, ...) and
assert.InEpsilon(t, float64(456), cfg["workflowId"], 0, ...)) to assert.Equal
(or assert.EqualValues) for exact comparison against cfg["installationId"] and
cfg["workflowId"]; keep the assert.InEpsilon usage only if you need a true
tolerance and then supply a meaningful epsilon (e.g., 1e-9) for numeric
comparisons like timeout, otherwise replace the timeout check
assert.InEpsilon(t, float64(90), cfg["timeout"], 0, ...) with assert.Equal as
well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 506573cc-fc04-49bf-b8cf-7282a77394b1
📒 Files selected for processing (8)
apps/workspace-engine/pkg/db/convert_test.goapps/workspace-engine/pkg/oapi/merge.goapps/workspace-engine/pkg/selector/resources_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.goapps/workspace-engine/pkg/workspace/store/repository/db/deployments/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go
...pace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go
Outdated
Show resolved
Hide resolved
apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.go
Outdated
Show resolved
Hide resolved
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go (1)
77-86:⚠️ Potential issue | 🟡 MinorMisleading error message for empty selector.
The message "failed to parse selector" is inaccurate when the selector is simply empty. Since
Selectoris a required field in the schema, an empty string indicates missing input rather than a parse failure. Consider updating the message for clarity.♻️ Proposed fix
// Failed to parse selector return results.NewDeniedResult( - fmt.Sprintf("Version selector: failed to parse selector: %v", celSelector), + "Version selector: selector is required but was empty", ). WithDetail("selector", celSelector),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go` around lines 77 - 86, The error message for empty selector is misleading: instead of saying "failed to parse selector" when e.rule.Selector is an empty string, update the returned denied result in the block handling celSelector == "" to indicate a missing or empty selector (e.g., "Version selector: selector is empty or missing") and keep the WithDetail("selector", celSelector) call; adjust the message where e.rule.Selector is checked just before calling evaluateCEL to reflect this change so evaluateCEL is only called for non-empty selectors.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go (1)
165-167: Consider moving validation before the database call.The empty selector check happens after
GetAllEnvironmentshas already been called. Moving this validation to the top of the function would fail fast before incurring the database lookup cost.♻️ Suggested improvement
func (e *EnvironmentProgressionEvaluator) findDependencyEnvironments( ctx context.Context, environment *oapi.Environment, ) ([]*oapi.Environment, error) { + if e.rule.DependsOnEnvironmentSelector == "" { + return nil, fmt.Errorf("invalid rule: DependsOnEnvironmentSelector must be non-empty") + } + var matchedEnvs []*oapi.Environment // Iterate through all environments envItems, err := e.getters.GetAllEnvironments(ctx, environment.WorkspaceId) if err != nil { return nil, fmt.Errorf("failed to get all environments: %w", err) } - if e.rule.DependsOnEnvironmentSelector == "" { - return nil, fmt.Errorf("invalid rule: DependsOnEnvironmentSelector must be non-empty") - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go` around lines 165 - 167, Move the validation of e.rule.DependsOnEnvironmentSelector to the start of the function so we fail fast before calling GetAllEnvironments; specifically, check that e.rule.DependsOnEnvironmentSelector is non-empty at the top of the function (the same check currently returning fmt.Errorf("invalid rule: DependsOnEnvironmentSelector must be non-empty")), and only call GetAllEnvironments after this validation passes to avoid unnecessary database lookups.
🤖 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/svc/http/server/openapi/resources/server.go`:
- Around line 32-33: The handler currently assigns cel = *body.Filter without
validating the CEL expression, causing malformed filters to surface as 500s;
validate the CEL filter right after the assignment of cel (when body.Filter !=
nil && *body.Filter != "") by invoking the CEL parser/evaluator validation used
elsewhere (or a helper like validateCEL or the CEL library) and if validation
fails return an HTTP 400 response with a clear client-facing error instead of
letting the error propagate to downstream evaluation; locate the check using
body.Filter and variable cel in server.go and add the validation+400-return
there so malformed client input is rejected with 400 rather than producing a
500.
---
Duplicate comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.go`:
- Around line 77-86: The error message for empty selector is misleading: instead
of saying "failed to parse selector" when e.rule.Selector is an empty string,
update the returned denied result in the block handling celSelector == "" to
indicate a missing or empty selector (e.g., "Version selector: selector is empty
or missing") and keep the WithDetail("selector", celSelector) call; adjust the
message where e.rule.Selector is checked just before calling evaluateCEL to
reflect this change so evaluateCEL is only called for non-empty selectors.
---
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go`:
- Around line 165-167: Move the validation of
e.rule.DependsOnEnvironmentSelector to the start of the function so we fail fast
before calling GetAllEnvironments; specifically, check that
e.rule.DependsOnEnvironmentSelector is non-empty at the top of the function (the
same check currently returning fmt.Errorf("invalid rule:
DependsOnEnvironmentSelector must be non-empty")), and only call
GetAllEnvironments after this validation passes to avoid unnecessary database
lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45b14c1c-5ff4-4e38-96d2-52ffdd4a078d
📒 Files selected for processing (11)
apps/web/app/routes/ws/jobs/_components/ResourceFilter.tsxapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/paths/resource.jsonnetapps/workspace-engine/oapi/spec/paths/validate.jsonnetapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/versionselector.goapps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.goapps/workspace-engine/svc/http/server/openapi/resources/server.goapps/workspace-engine/svc/http/server/openapi/validators/validators.gopackages/workspace-engine-sdk/src/schema.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/workspace-engine/pkg/workspace/store/repository/db/environments/mapper.go
- apps/workspace-engine/oapi/spec/paths/resource.jsonnet
- packages/workspace-engine-sdk/src/schema.ts
- apps/web/app/routes/ws/jobs/_components/ResourceFilter.tsx
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
1 participant
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