refactor: update DeploymentDependencyEvaluator to use getters for dat…#835
Open
refactor: update DeploymentDependencyEvaluator to use getters for dat…#835
Conversation
…a access and rename evaluator constructor
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 28. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors evaluator data access to use Getters abstractions (and some direct store usage), removes multiple adapter/loader layers, restructures variable resolution into a resolveContext with preloaded candidates and utilities, and adds a conditional legacy-entity migration during Store.Restore. Changes
Sequence Diagram(s)sequenceDiagram
participant Resolver as VariableResolver.Resolve
participant Getter as Getter / store
participant RC as resolveContext
participant Eval as eval.EvaluateRules
participant Candidates as Candidates (preloaded)
Resolver->>Getter: GetAllEntities(workspaceID)
Getter-->>Resolver: candidates
Resolver->>RC: buildResolveContext(entity, rules, candidates)
RC->>Eval: EvaluateRules(entity, candidates, rules)
Eval-->>RC: matches
RC-->>Resolver: related entity data (via candidate index)
Resolver->>Getter: optional property lookup (getMapProperty)
Getter-->>Resolver: property values
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
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: 1
🧹 Nitpick comments (3)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.go (1)
30-49: Document the exported constructors.
NewEvaluatorFromStoreand the renamedNewEvaluatorare exported entry points now, but neither has a doc comment.As per coding guidelines "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".✍️ Minimal fix
+// NewEvaluatorFromStore builds a deployment-dependency evaluator backed by a store. func NewEvaluatorFromStore(store *store.Store, dependencyRule *oapi.PolicyRule) evaluator.Evaluator { @@ +// NewEvaluator builds a deployment-dependency evaluator from the provided getters. func NewEvaluator(getters Getters, dependencyRule *oapi.PolicyRule) evaluator.Evaluator {🤖 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/deploymentdependency/deployment_dependency.go` around lines 30 - 49, Add doc comments for the two exported constructors NewEvaluatorFromStore and NewEvaluator: explain purpose (create a DeploymentDependency evaluator), describe parameters (store *store.Store for NewEvaluatorFromStore; getters Getters and dependencyRule *oapi.PolicyRule for NewEvaluator), note return value semantics (returns an evaluator.Evaluator or nil when inputs are nil or when dependencyRule.DeploymentDependency is nil), and mention that NewEvaluator wraps the evaluator with memoization and populates the DeploymentDependencyEvaluator fields (getters, ruleId, rule). Place the comment immediately above each function to document usage and nil-return behavior.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/getters.go (1)
9-13: Add a doc comment for the new exportedGettersinterface.This new exported type is undocumented right now, which makes the package API harder to navigate.
As per coding guidelines "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".✍️ Minimal fix
+// Getters provides the read-only data access needed by DeploymentDependencyEvaluator. type Getters interface {🤖 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/deploymentdependency/getters.go` around lines 9 - 13, Add a descriptive doc comment above the exported Getters interface that explains its purpose (provides read-only accessors used by the deployment dependency evaluator), the semantics of each method (GetDeployments returns the current deployment map, GetReleaseTargetsForResource returns release targets for a given resourceID in the provided context, GetLatestCompletedJobForReleaseTarget returns the most recent completed Job for a ReleaseTarget), and any concurrency/ownership expectations or nil-return guarantees callers must handle; place this comment immediately above the Getters declaration so it documents the exported type and its methods.apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go (1)
37-37: Avoid shipping a bare commented-out evaluator call.If gradual rollout is intentionally disabled here, please replace the commented-out call with a short rationale/TODO or remove it.
As per coding guidelines "Do not add comments that simply restate what the code does" and "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go` at line 37, The commented-out evaluator call gradualrollout.NewEvaluator(getter, rule) should not be left bare; either remove the commented line completely or replace it with a short explanatory comment/TODO that documents why gradual rollout is disabled and what needs to be done to re-enable it (e.g., "TODO: enable gradual rollout after X is implemented" or "disabled due to Y"). Locate the commented call in the evaluator registration block (the Gradual Rollout evaluator reference) and apply the change so the codebase contains intent/context instead of a bare commented-out call.
🤖 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/controllers/desiredrelease/policyeval/policyeval.go`:
- Around line 22-27: The new policyeval.Getter embeds multiple evaluator
interfaces that policyevalAdapter doesn't implement and evaluator constructors
(e.g., approval.NewEvaluator in ruleEvaluators) are being passed the raw getter,
causing signature/missing-method compile errors; fix by restoring the adapter
wrappers: in ruleEvaluators wrap the controller-facing getter with the existing
adapter (policyevalAdapter from policyeval/adapters.go) before passing to each
evaluator (e.g., create adapter := policyeval.NewAdapter(getter) or call the
existing adapter constructor and pass adapter to approval.NewEvaluator,
environmentprogression.NewEvaluator, deploymentwindow.NewEvaluator,
deploymentdependency.NewEvaluator, versioncooldown.NewEvaluator,
deployableversions.NewEvaluator) so the evaluators receive the expected method
signatures, or alternatively implement all missing methods on policyevalAdapter
with the exact evaluator interface signatures if you prefer not to use the
adapter.
---
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.go`:
- Around line 30-49: Add doc comments for the two exported constructors
NewEvaluatorFromStore and NewEvaluator: explain purpose (create a
DeploymentDependency evaluator), describe parameters (store *store.Store for
NewEvaluatorFromStore; getters Getters and dependencyRule *oapi.PolicyRule for
NewEvaluator), note return value semantics (returns an evaluator.Evaluator or
nil when inputs are nil or when dependencyRule.DeploymentDependency is nil), and
mention that NewEvaluator wraps the evaluator with memoization and populates the
DeploymentDependencyEvaluator fields (getters, ruleId, rule). Place the comment
immediately above each function to document usage and nil-return behavior.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/getters.go`:
- Around line 9-13: Add a descriptive doc comment above the exported Getters
interface that explains its purpose (provides read-only accessors used by the
deployment dependency evaluator), the semantics of each method (GetDeployments
returns the current deployment map, GetReleaseTargetsForResource returns release
targets for a given resourceID in the provided context,
GetLatestCompletedJobForReleaseTarget returns the most recent completed Job for
a ReleaseTarget), and any concurrency/ownership expectations or nil-return
guarantees callers must handle; place this comment immediately above the Getters
declaration so it documents the exported type and its methods.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go`:
- Line 37: The commented-out evaluator call gradualrollout.NewEvaluator(getter,
rule) should not be left bare; either remove the commented line completely or
replace it with a short explanatory comment/TODO that documents why gradual
rollout is disabled and what needs to be done to re-enable it (e.g., "TODO:
enable gradual rollout after X is implemented" or "disabled due to Y"). Locate
the commented call in the evaluator registration block (the Gradual Rollout
evaluator reference) and apply the change so the codebase contains
intent/context instead of a bare commented-out call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40e7af72-c51e-4127-9520-b4c8eb8efdd5
📒 Files selected for processing (6)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluators.goapps/workspace-engine/pkg/workspace/releasemanager/policy/policymanager.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go
Comment on lines
+22
to
+27
| approval.Getters | ||
| environmentprogression.Getters | ||
| deploymentwindow.Getters | ||
| deploymentdependency.Getters | ||
| versioncooldown.Getters | ||
| deployableversions.Getters |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== policyeval.Getter ==="
sed -n '20,45p' apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go
echo
echo "=== Existing desiredrelease/policyeval adapters ==="
sed -n '1,220p' apps/workspace-engine/svc/controllers/desiredrelease/policyeval/adapters.go
echo
echo "=== Concrete adapter passed into policyeval ==="
sed -n '1,140p' apps/workspace-engine/svc/controllers/desiredrelease/adapters.go
echo
echo "=== Evaluator getter contracts ==="
rg -n -A6 -B1 'type Getters interface' apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluatorRepository: ctrlplanedev/ctrlplane
Length of output: 13735
policyeval.Getter now embeds evaluator interfaces that policyevalAdapter does not fully implement.
The new policyeval.Getter embeds approval.Getters, environmentprogression.Getters, deploymentwindow.Getters, deploymentdependency.Getters, versioncooldown.Getters, and deployableversions.Getters. The ruleEvaluators function passes getter directly to evaluator constructors (e.g., approval.NewEvaluator(getter, rule)), but the concrete policyevalAdapter only implements 4 methods and lacks many required by these embedded interfaces (e.g., GetEnvironments, GetDeployments, GetReleaseTargetsForResource, etc.). Additionally, the signatures of methods that do exist differ—policyevalAdapter has ctx and error returns, while evaluator interfaces expect neither. The adapter layer in policyeval/adapters.go bridges these differences but is no longer invoked. This will not compile unless you either implement all missing methods on policyevalAdapter with the correct signatures, reintroduce the adapter wrappers in ruleEvaluators, or restructure Getter to remain controller-facing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go`
around lines 22 - 27, The new policyeval.Getter embeds multiple evaluator
interfaces that policyevalAdapter doesn't implement and evaluator constructors
(e.g., approval.NewEvaluator in ruleEvaluators) are being passed the raw getter,
causing signature/missing-method compile errors; fix by restoring the adapter
wrappers: in ruleEvaluators wrap the controller-facing getter with the existing
adapter (policyevalAdapter from policyeval/adapters.go) before passing to each
evaluator (e.g., create adapter := policyeval.NewAdapter(getter) or call the
existing adapter constructor and pass adapter to approval.NewEvaluator,
environmentprogression.NewEvaluator, deploymentwindow.NewEvaluator,
deploymentdependency.NewEvaluator, versioncooldown.NewEvaluator,
deployableversions.NewEvaluator) so the evaluators receive the expected method
signatures, or alternatively implement all missing methods on policyevalAdapter
with the exact evaluator interface signatures if you prefer not to use the
adapter.
…egrating direct candidate access
…tion process by integrating direct entity access
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/relationships/eval/evaluate.go (1)
118-124:⚠️ Potential issue | 🔴 CriticalUpdate the remaining
EvaluateRulescaller before merging.This signature change leaves
apps/workspace-engine/svc/controllers/relationshipeval/controller.goon the old loader-based invocation, so the PR will not compile until that controller preloads candidates and passes the slice here instead. While you touch this, please also refresh the doc comment since it still describes loader-based evaluation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/relationships/eval/evaluate.go` around lines 118 - 124, The controller that calls EvaluateRules still passes a loader rather than a preloaded slice, so update that caller in the relationshipeval controller to preload candidates into a []EntityData and invoke EvaluateRules(ctx, entity, candidates, rules); also update the EvaluateRules doc comment to remove the loader-based description and reflect the new slice-based API. Locate references to EvaluateRules and the controller's evaluation handler and change the call-site signature accordingly, ensuring types match ([]EntityData) and imports remain correct.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environment_progression_action.go (1)
26-31: Document the exported constructor.
NewEnvironmentProgressionActionis a new exported entry point, but it does not have a doc comment yet.✍️ Suggested fix
+// NewEnvironmentProgressionAction creates the action that re-reconciles +// downstream release targets when an environment progression threshold is crossed. func NewEnvironmentProgressionAction(store *store.Store, reconcileFn ReconcileFn) *EnvironmentProgressionAction {As per coding guidelines "document exported functions/types/methods".
🤖 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/environment_progression_action.go` around lines 26 - 31, Add a descriptive doc comment above the exported constructor NewEnvironmentProgressionAction explaining its purpose and behavior: state that it creates and returns a new *EnvironmentProgressionAction initialized with the provided *store.Store and ReconcileFn, describe any important side-effects or expectations of the passed-in reconcileFn and store (e.g., non-nil requirements), and mention the returned type EnvironmentProgressionAction; keep the comment concise and follow project doc style for exported functions.
🤖 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/controllers/desiredrelease/variableresolver/utils.go`:
- Around line 11-43: getPropertyReflection currently rejects non-struct entities
causing map-backed inputs (e.g., refs[0].Raw maps) to fail; modify
getPropertyReflection to handle map kinds before the struct check: if
reflect.Value.Kind() is Map, use
reflect.Value.MapIndex(reflect.ValueOf(fieldName)) to fetch the value (also try
string-key case-insensitive lookup if MapIndex is invalid), return convertValue
when this is the last path element, or recurse into getPropertyReflection with
the map value.Interface() and propertyPath[1:]; keep existing struct-field logic
(FieldByName/FieldByNameFunc) unchanged and ensure pointer dereferencing
(v.Elem()) is handled as before.
---
Outside diff comments:
In `@apps/workspace-engine/pkg/workspace/relationships/eval/evaluate.go`:
- Around line 118-124: The controller that calls EvaluateRules still passes a
loader rather than a preloaded slice, so update that caller in the
relationshipeval controller to preload candidates into a []EntityData and invoke
EvaluateRules(ctx, entity, candidates, rules); also update the EvaluateRules doc
comment to remove the loader-based description and reflect the new slice-based
API. Locate references to EvaluateRules and the controller's evaluation handler
and change the call-site signature accordingly, ensuring types match
([]EntityData) and imports remain correct.
---
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environment_progression_action.go`:
- Around line 26-31: Add a descriptive doc comment above the exported
constructor NewEnvironmentProgressionAction explaining its purpose and behavior:
state that it creates and returns a new *EnvironmentProgressionAction
initialized with the provided *store.Store and ReconcileFn, describe any
important side-effects or expectations of the passed-in reconcileFn and store
(e.g., non-nil requirements), and mention the returned type
EnvironmentProgressionAction; keep the comment concise and follow project doc
style for exported functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 841df09a-f912-49e4-960a-1282cb1c3109
📒 Files selected for processing (16)
apps/workspace-engine/pkg/workspace/relationships/eval/evaluate.goapps/workspace-engine/pkg/workspace/relationships/eval/evaluate_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/approval.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environment_progression_action.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.goapps/workspace-engine/pkg/workspace/workspace.goapps/workspace-engine/svc/controllers/desiredrelease/adapters.goapps/workspace-engine/svc/controllers/desiredrelease/getters.goapps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/adapters.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/related_resolver.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/utils.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/value.go
💤 Files with no reviewable changes (3)
- apps/workspace-engine/svc/controllers/desiredrelease/adapters.go
- apps/workspace-engine/svc/controllers/desiredrelease/policyeval/adapters.go
- apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.go
Comment on lines
+11
to
+43
| func getPropertyReflection(entity any, propertyPath []string) (*oapi.LiteralValue, error) { | ||
| if len(propertyPath) == 0 { | ||
| return convertValue(entity) | ||
| } | ||
|
|
||
| v := reflect.ValueOf(entity) | ||
| if v.Kind() == reflect.Ptr { | ||
| v = v.Elem() | ||
| } | ||
|
|
||
| if v.Kind() != reflect.Struct { | ||
| return nil, fmt.Errorf("entity is not a struct") | ||
| } | ||
|
|
||
| // Get the first property | ||
| fieldName := propertyPath[0] | ||
| field := v.FieldByName(fieldName) | ||
| if !field.IsValid() { | ||
| // Try case-insensitive match | ||
| field = v.FieldByNameFunc(func(name string) bool { | ||
| return strings.EqualFold(name, fieldName) | ||
| }) | ||
| if !field.IsValid() { | ||
| return nil, fmt.Errorf("field %s not found", fieldName) | ||
| } | ||
| } | ||
|
|
||
| // If this is the last element, return the field value | ||
| if len(propertyPath) == 1 { | ||
| return convertValue(field.Interface()) | ||
| } | ||
|
|
||
| return getPropertyReflection(field.Interface(), propertyPath[1:]) |
Contributor
There was a problem hiding this comment.
Support map-backed entities before reflecting on struct fields.
ResolveValue now passes refs[0].Raw, and both apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go and apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/related_resolver.go populate Raw as maps. Because Line 21 rejects any non-struct input, reference paths like metadata.region will now fail with entity is not a struct.
🛠️ Suggested fix
func getPropertyReflection(entity any, propertyPath []string) (*oapi.LiteralValue, error) {
if len(propertyPath) == 0 {
return convertValue(entity)
}
+
+ switch v := entity.(type) {
+ case map[string]any:
+ next, ok := v[propertyPath[0]]
+ if !ok {
+ return nil, fmt.Errorf("field %s not found", propertyPath[0])
+ }
+ return getPropertyReflection(next, propertyPath[1:])
+ case map[string]string:
+ next, ok := v[propertyPath[0]]
+ if !ok {
+ return nil, fmt.Errorf("field %s not found", propertyPath[0])
+ }
+ return getPropertyReflection(next, propertyPath[1:])
+ }
v := reflect.ValueOf(entity)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func getPropertyReflection(entity any, propertyPath []string) (*oapi.LiteralValue, error) { | |
| if len(propertyPath) == 0 { | |
| return convertValue(entity) | |
| } | |
| v := reflect.ValueOf(entity) | |
| if v.Kind() == reflect.Ptr { | |
| v = v.Elem() | |
| } | |
| if v.Kind() != reflect.Struct { | |
| return nil, fmt.Errorf("entity is not a struct") | |
| } | |
| // Get the first property | |
| fieldName := propertyPath[0] | |
| field := v.FieldByName(fieldName) | |
| if !field.IsValid() { | |
| // Try case-insensitive match | |
| field = v.FieldByNameFunc(func(name string) bool { | |
| return strings.EqualFold(name, fieldName) | |
| }) | |
| if !field.IsValid() { | |
| return nil, fmt.Errorf("field %s not found", fieldName) | |
| } | |
| } | |
| // If this is the last element, return the field value | |
| if len(propertyPath) == 1 { | |
| return convertValue(field.Interface()) | |
| } | |
| return getPropertyReflection(field.Interface(), propertyPath[1:]) | |
| func getPropertyReflection(entity any, propertyPath []string) (*oapi.LiteralValue, error) { | |
| if len(propertyPath) == 0 { | |
| return convertValue(entity) | |
| } | |
| switch v := entity.(type) { | |
| case map[string]any: | |
| next, ok := v[propertyPath[0]] | |
| if !ok { | |
| return nil, fmt.Errorf("field %s not found", propertyPath[0]) | |
| } | |
| return getPropertyReflection(next, propertyPath[1:]) | |
| case map[string]string: | |
| next, ok := v[propertyPath[0]] | |
| if !ok { | |
| return nil, fmt.Errorf("field %s not found", propertyPath[0]) | |
| } | |
| return getPropertyReflection(next, propertyPath[1:]) | |
| } | |
| v := reflect.ValueOf(entity) | |
| if v.Kind() == reflect.Ptr { | |
| v = v.Elem() | |
| } | |
| if v.Kind() != reflect.Struct { | |
| return nil, fmt.Errorf("entity is not a struct") | |
| } | |
| // Get the first property | |
| fieldName := propertyPath[0] | |
| field := v.FieldByName(fieldName) | |
| if !field.IsValid() { | |
| // Try case-insensitive match | |
| field = v.FieldByNameFunc(func(name string) bool { | |
| return strings.EqualFold(name, fieldName) | |
| }) | |
| if !field.IsValid() { | |
| return nil, fmt.Errorf("field %s not found", fieldName) | |
| } | |
| } | |
| // If this is the last element, return the field value | |
| if len(propertyPath) == 1 { | |
| return convertValue(field.Interface()) | |
| } | |
| return getPropertyReflection(field.Interface(), propertyPath[1:]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/utils.go`
around lines 11 - 43, getPropertyReflection currently rejects non-struct
entities causing map-backed inputs (e.g., refs[0].Raw maps) to fail; modify
getPropertyReflection to handle map kinds before the struct check: if
reflect.Value.Kind() is Map, use
reflect.Value.MapIndex(reflect.ValueOf(fieldName)) to fetch the value (also try
string-key case-insensitive lookup if MapIndex is invalid), return convertValue
when this is the last path element, or recurse into getPropertyReflection with
the map value.Interface() and propertyPath[1:]; keep existing struct-field logic
(FieldByName/FieldByNameFunc) unchanged and ensure pointer dereferencing
(v.Elem()) is handled as before.
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/svc/controllers/desiredrelease/variableresolver/resolve_test.go (1)
19-37:⚠️ Potential issue | 🔴 CriticalUpdate
mockGetterto satisfy the newGetterinterface.
Resolvenow requiresGetAllEntities, but this test double still implements the removedLoadCandidatesmethod. EveryResolve(..., getter, scope)call in this file will fail to compile until the mock is updated.🧪 Minimal fix
type mockGetter struct { deploymentVars []oapi.DeploymentVariableWithValues resourceVars map[string]oapi.ResourceVariable rules []eval.Rule candidates map[string][]eval.EntityData } @@ -func (m *mockGetter) LoadCandidates(_ context.Context, _ uuid.UUID, entityType string) ([]eval.EntityData, error) { - return m.candidates[entityType], nil +func (m *mockGetter) GetAllEntities(_ context.Context, _ uuid.UUID) ([]eval.EntityData, error) { + total := 0 + for _, candidates := range m.candidates { + total += len(candidates) + } + all := make([]eval.EntityData, 0, total) + for _, candidates := range m.candidates { + all = append(all, candidates...) + } + return all, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go` around lines 19 - 37, The mockGetter no longer satisfies the Getter interface because Resolve now calls GetAllEntities; add a method on mockGetter with the signature GetAllEntities(ctx context.Context, id uuid.UUID) (map[string][]eval.EntityData, error) that returns m.candidates, nil (you can keep or remove LoadCandidates but implement GetAllEntities so mockGetter satisfies Getter and tests compile). Ensure the method uses the same package types (context.Context, uuid.UUID, eval.EntityData) and mirrors how the real Getter populates candidates.
🤖 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/controllers/desiredrelease/variableresolver/resolve.go`:
- Around line 80-84: The code eagerly calls buildResolveContext at the start of
Resolve (rc, err := buildResolveContext(...)) which materializes workspace
entities even when no reference lookup is needed; change Resolve to defer
building the resolve context until the first time a reference variable is
encountered by: remove the initial buildResolveContext call, add a
lazily-initialized helper (e.g. getResolveContext or ensureResolveContext) that
calls buildResolveContext the first time a non-literal/default variable requires
a lookup, cache its result in a local rc variable, and propagate errors via
span.RecordError and the existing error return path so behavior is unchanged
when a context is actually required (references: buildResolveContext, Resolve,
span.RecordError).
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/value.go`:
- Around line 22-61: The resolveValue function currently collapses all failures
into a nil return which makes errors indistinguishable from “not found”; change
resolveValue(ctx, lookup, value) to return (*oapi.LiteralValue, error) and
propagate real errors instead of returning nil — call value.GetType() and
value.AsLiteralValue() and on any error return (nil, err) after recording the
span (span.RecordError / span.SetStatus) instead of nil, and make
resolveReference return (lv, err) propagate that error similarly; only return
(nil, nil) when the value truly means “not found” (e.g., type == "sensitive" or
explicit absence), and update all callers to handle the (value, error) tuple so
callers can fall through on (nil, nil) but fail-fast on non-nil errors.
---
Outside diff comments:
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.go`:
- Around line 19-37: The mockGetter no longer satisfies the Getter interface
because Resolve now calls GetAllEntities; add a method on mockGetter with the
signature GetAllEntities(ctx context.Context, id uuid.UUID)
(map[string][]eval.EntityData, error) that returns m.candidates, nil (you can
keep or remove LoadCandidates but implement GetAllEntities so mockGetter
satisfies Getter and tests compile). Ensure the method uses the same package
types (context.Context, uuid.UUID, eval.EntityData) and mirrors how the real
Getter populates candidates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b31cf21-73b2-4910-bc99-d2d3e2cc9363
📒 Files selected for processing (6)
apps/workspace-engine/svc/controllers/desiredrelease/reconcile.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_context.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve_test.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/utils.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/value.go
Comment on lines
+80
to
84
| rc, err := buildResolveContext(ctx, getter, scope.Resource) | ||
| if err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "get relationship rules failed") | ||
| return nil, fmt.Errorf("get relationship rules: %w", err) | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
Defer buildResolveContext until a reference value is actually encountered.
This now loads relationship rules and all workspace entities for every Resolve call, even when every variable is literal/default and no related lookup is needed. On the reconcile hot path, that turns a cheap variable pass into two extra store calls plus a whole-workspace materialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go`
around lines 80 - 84, The code eagerly calls buildResolveContext at the start of
Resolve (rc, err := buildResolveContext(...)) which materializes workspace
entities even when no reference lookup is needed; change Resolve to defer
building the resolve context until the first time a reference variable is
encountered by: remove the initial buildResolveContext call, add a
lazily-initialized helper (e.g. getResolveContext or ensureResolveContext) that
calls buildResolveContext the first time a non-literal/default variable requires
a lookup, cache its result in a local rc variable, and propagate errors via
span.RecordError and the existing error return path so behavior is unchanged
when a context is actually required (references: buildResolveContext, Resolve,
span.RecordError).
Comment on lines
+22
to
61
| func resolveValue( | ||
| ctx context.Context, | ||
| resolver RelatedEntityResolver, | ||
| resourceID string, | ||
| entity *oapi.RelatableEntity, | ||
| lookup relatedLookup, | ||
| value *oapi.Value, | ||
| ) (*oapi.LiteralValue, error) { | ||
| _, span := tracer.Start(ctx, "variableresolver.ResolveValue") | ||
| ) *oapi.LiteralValue { | ||
| _, span := tracer.Start(ctx, "variableresolver.resolveValue") | ||
| defer span.End() | ||
|
|
||
| valueType, err := value.GetType() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("determine value type: %w", err) | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "get value type failed") | ||
| log.Error("get value type failed", "error", err) | ||
| return nil | ||
| } | ||
|
|
||
| switch valueType { | ||
| case "literal": | ||
| return resolveLiteral(value) | ||
| lv, err := value.AsLiteralValue() | ||
| if err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "extract literal value failed") | ||
| log.Error("extract literal value failed", "error", err) | ||
| return nil | ||
| } | ||
| return &lv | ||
| case "reference": | ||
| return resolveReference(ctx, resolver, value, entity) | ||
| lv, err := resolveReference(ctx, lookup, value) | ||
| if err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, "resolve reference failed") | ||
| log.Error("resolve reference failed", "error", err) | ||
| return nil | ||
| } | ||
| return lv | ||
| case "sensitive": | ||
| return nil, fmt.Errorf("sensitive values are not resolved by the variable resolver") | ||
| return nil | ||
| default: | ||
| return nil, fmt.Errorf("unsupported value type: %s", valueType) | ||
| } | ||
| } | ||
|
|
||
| func resolveLiteral(value *oapi.Value) (*oapi.LiteralValue, error) { | ||
| lv, err := value.AsLiteralValue() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("extract literal value: %w", err) | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t collapse real resolution failures into nil.
Resolve treats nil here as “fall through to the next source”. With this contract, a bad reference path, malformed literal, unsupported type, and sensitive value all look identical to “reference not found”, so misconfigured variables can silently deploy with a lower-priority/default value instead of failing fast. This is also the root cause of the stale two-value resolveValue calls in the tests.
🛠️ Suggested direction
func resolveValue(
ctx context.Context,
lookup relatedLookup,
value *oapi.Value,
-) *oapi.LiteralValue {
+) (*oapi.LiteralValue, error) {
_, span := tracer.Start(ctx, "variableresolver.resolveValue")
defer span.End()
@@
valueType, err := value.GetType()
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, "get value type failed")
log.Error("get value type failed", "error", err)
- return nil
+ return nil, err
}
@@
case "literal":
lv, err := value.AsLiteralValue()
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, "extract literal value failed")
log.Error("extract literal value failed", "error", err)
- return nil
+ return nil, err
}
- return &lv
+ return &lv, nil
case "reference":
- lv, err := resolveReference(ctx, lookup, value)
- if err != nil {
- span.RecordError(err)
- span.SetStatus(codes.Error, "resolve reference failed")
- log.Error("resolve reference failed", "error", err)
- return nil
- }
- return lv
+ return resolveReference(ctx, lookup, value)
case "sensitive":
- return nil
+ return nil, fmt.Errorf("sensitive values cannot be resolved")
default:
- return nil
+ return nil, fmt.Errorf("unsupported value type %q", valueType)
}
}Callers can still treat (nil, nil) as “reference not found”, but they should stop falling through on real errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func resolveValue( | |
| ctx context.Context, | |
| resolver RelatedEntityResolver, | |
| resourceID string, | |
| entity *oapi.RelatableEntity, | |
| lookup relatedLookup, | |
| value *oapi.Value, | |
| ) (*oapi.LiteralValue, error) { | |
| _, span := tracer.Start(ctx, "variableresolver.ResolveValue") | |
| ) *oapi.LiteralValue { | |
| _, span := tracer.Start(ctx, "variableresolver.resolveValue") | |
| defer span.End() | |
| valueType, err := value.GetType() | |
| if err != nil { | |
| return nil, fmt.Errorf("determine value type: %w", err) | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "get value type failed") | |
| log.Error("get value type failed", "error", err) | |
| return nil | |
| } | |
| switch valueType { | |
| case "literal": | |
| return resolveLiteral(value) | |
| lv, err := value.AsLiteralValue() | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "extract literal value failed") | |
| log.Error("extract literal value failed", "error", err) | |
| return nil | |
| } | |
| return &lv | |
| case "reference": | |
| return resolveReference(ctx, resolver, value, entity) | |
| lv, err := resolveReference(ctx, lookup, value) | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "resolve reference failed") | |
| log.Error("resolve reference failed", "error", err) | |
| return nil | |
| } | |
| return lv | |
| case "sensitive": | |
| return nil, fmt.Errorf("sensitive values are not resolved by the variable resolver") | |
| return nil | |
| default: | |
| return nil, fmt.Errorf("unsupported value type: %s", valueType) | |
| } | |
| } | |
| func resolveLiteral(value *oapi.Value) (*oapi.LiteralValue, error) { | |
| lv, err := value.AsLiteralValue() | |
| if err != nil { | |
| return nil, fmt.Errorf("extract literal value: %w", err) | |
| return nil | |
| } | |
| func resolveValue( | |
| ctx context.Context, | |
| lookup relatedLookup, | |
| value *oapi.Value, | |
| ) (*oapi.LiteralValue, error) { | |
| _, span := tracer.Start(ctx, "variableresolver.resolveValue") | |
| defer span.End() | |
| valueType, err := value.GetType() | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "get value type failed") | |
| log.Error("get value type failed", "error", err) | |
| return nil, err | |
| } | |
| switch valueType { | |
| case "literal": | |
| lv, err := value.AsLiteralValue() | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, "extract literal value failed") | |
| log.Error("extract literal value failed", "error", err) | |
| return nil, err | |
| } | |
| return &lv, nil | |
| case "reference": | |
| return resolveReference(ctx, lookup, value) | |
| case "sensitive": | |
| return nil, fmt.Errorf("sensitive values cannot be resolved") | |
| default: | |
| return nil, fmt.Errorf("unsupported value type %q", valueType) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/value.go`
around lines 22 - 61, The resolveValue function currently collapses all failures
into a nil return which makes errors indistinguishable from “not found”; change
resolveValue(ctx, lookup, value) to return (*oapi.LiteralValue, error) and
propagate real errors instead of returning nil — call value.GetType() and
value.AsLiteralValue() and on any error return (nil, err) after recording the
span (span.RecordError / span.SetStatus) instead of nil, and make
resolveReference return (lv, err) propagate that error similarly; only return
(nil, nil) when the value truly means “not found” (e.g., type == "sensitive" or
explicit absence), and update all callers to handle the (value, error) tuple so
callers can fall through on (nil, nil) but fail-fast on non-nil errors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/store/store.go (1)
667-689: Consider combining job migrations into a single loop to avoid inconsistent intermediate state.Jobs are iterated twice: first to persist them (lines 667-672), then again to normalize
ReleaseIdvalues (lines 677-689). This writes jobs with potentially invalid (non-UUID)ReleaseIdto the database, then immediately overwrites them with the corrected value. This creates unnecessary DB writes for affected jobs and a brief window of inconsistent data.♻️ Suggested refactor: combine into single loop
if setStatus != nil { setStatus("Migrating legacy jobs") } for _, job := range s.repo.Jobs.Items() { + // Normalize non-UUID ReleaseId before persisting + if job.ReleaseId != "" { + if _, err := uuid.Parse(job.ReleaseId); err != nil { + job.ReleaseId = uuid.NewSHA1(uuid.NameSpaceOID, []byte(job.ReleaseId)).String() + } + } if err := s.Jobs.repo.Set(job); err != nil { log.Warn("Failed to migrate legacy job", "job_id", job.Id, "error", err) } } - - if setStatus != nil { - setStatus("Migrating legacy job release IDs") - } - for _, job := range s.repo.Jobs.Items() { - if job.ReleaseId == "" { - continue - } - if _, err := uuid.Parse(job.ReleaseId); err == nil { - continue - } - job.ReleaseId = uuid.NewSHA1(uuid.NameSpaceOID, []byte(job.ReleaseId)).String() - if err := s.Jobs.repo.Set(job); err != nil { - log.Warn("Failed to migrate legacy job release ID", - "job_id", job.Id, "error", err) - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/store.go` around lines 667 - 689, The code performs two passes over jobs causing redundant writes and transient bad ReleaseId state; refactor to a single loop that normalizes job.ReleaseId (use uuid.Parse and uuid.NewSHA1 when needed) before calling s.Jobs.repo.Set(job), keep the existing log.Warn messages on Set errors, and invoke setStatus("Migrating legacy job release IDs") once before the combined loop so each job is validated/normalized and then persisted in one operation.
🤖 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/store/store.go`:
- Around line 644-649: Replace direct field access to the in-memory repos with
the provided accessor methods for consistency: change usages of
s.repo.RelationshipRules.Items() to s.repo.RelationshipRulesRepo().Items() and
change s.repo.Jobs.Items() to s.repo.JobsRepo().Items(); update the code blocks
that iterate over legacy relationship rules (where rr is used) and job
migrations to call the RelationshipRulesRepo() and JobsRepo() methods
respectively so they match the existing pattern used by Systems(), Policies(),
ResourceVariables(), and Releases().
---
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/store/store.go`:
- Around line 667-689: The code performs two passes over jobs causing redundant
writes and transient bad ReleaseId state; refactor to a single loop that
normalizes job.ReleaseId (use uuid.Parse and uuid.NewSHA1 when needed) before
calling s.Jobs.repo.Set(job), keep the existing log.Warn messages on Set errors,
and invoke setStatus("Migrating legacy job release IDs") once before the
combined loop so each job is validated/normalized and then persisted in one
operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02f50c44-1e1b-4a72-8d10-5262fbc81de0
📒 Files selected for processing (5)
apps/workspace-engine/pkg/config/env.goapps/workspace-engine/pkg/workspace/store/restore_test.goapps/workspace-engine/pkg/workspace/store/store.goapps/workspace-engine/svc/http/server/openapi/deployments/server.gopackages/trpc/src/routes/reconcile.ts
💤 Files with no reviewable changes (1)
- apps/workspace-engine/pkg/workspace/store/restore_test.go
✅ Files skipped from review due to trivial changes (1)
- packages/trpc/src/routes/reconcile.ts
Comment on lines
+644
to
+649
| for _, rr := range s.repo.RelationshipRules.Items() { | ||
| if err := s.Relationships.repo.Set(rr); err != nil { | ||
| log.Warn("Failed to migrate legacy relationship rule", | ||
| "id", rr.Id, "name", rr.Name, "error", err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cd /tmp || exit 1
# Find the memory repo implementation file
echo "=== Locating memory repo files ==="
find /repo -name "*memory*" -type f -path "*/repo/*" | head -20
echo ""
echo "=== Checking InMemory struct definition ==="
rg -n "type InMemory struct" --type go -A 20
echo ""
echo "=== Checking how RelationshipRules is defined ==="
rg -n "RelationshipRules" --type go | head -30
echo ""
echo "=== Checking how Systems is defined/accessed ==="
rg -n "Systems\(\)" --type go | head -20
echo ""
echo "=== Checking how Policies is defined/accessed ==="
rg -n "Policies\(\)" --type go | head -20
echo ""
echo "=== Checking how Jobs is defined/accessed ==="
rg -n "\.Jobs" --type go | head -30Repository: ctrlplanedev/ctrlplane
Length of output: 1147
🏁 Script executed:
#!/bin/bash
echo "=== Finding Go files in workspace engine ==="
find . -name "*.go" -path "*/workspace/*" -type f | grep -i memory | head -10
echo ""
echo "=== Looking for store.go to understand the context ==="
find . -name "store.go" -path "*/workspace/*" | head -5
echo ""
echo "=== Searching for InMemory struct definition ==="
rg "type InMemory struct" -A 30 --type go
echo ""
echo "=== Checking RelationshipRules accessor pattern ==="
rg "RelationshipRules" -B 2 -A 2 --type go
echo ""
echo "=== Checking Systems accessor pattern ==="
rg "\.Systems\(" --type go | head -20
echo ""
echo "=== Checking Jobs accessor ==="
rg "\.Jobs" --type go | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 50378
🏁 Script executed:
#!/bin/bash
echo "=== Context around line 644-649 in store.go ==="
sed -n '630,660p' ./apps/workspace-engine/pkg/workspace/store/store.go
echo ""
echo "=== Checking what s.repo is in the Store struct ==="
grep -A 20 "type Store struct" ./apps/workspace-engine/pkg/workspace/store/store.go | head -30
echo ""
echo "=== Checking how Jobs and RelationshipRules are accessed elsewhere ==="
rg "repo\.Jobs\(" --type go -C 2 | head -40
echo ""
echo "=== Checking cmap.ConcurrentMap Items method ==="
fd "cmap" --type f -o | head -5
rg "type ConcurrentMap" --type go -A 5 | head -20
echo ""
echo "=== Verify Systems and Policies accessor patterns in actual usage ==="
rg "s\.repo\.Systems\(\)" --type go -C 2 | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 3675
🏁 Script executed:
#!/bin/bash
echo "=== Check how other repos define their methods in InMemory ==="
rg "func \(s \*InMemory\)" --type go | grep -E "(Systems|Policies|Jobs|ResourceVariables|Releases|RelationshipRules)" | head -20
echo ""
echo "=== Look specifically for Jobs and RelationshipRules method definitions ==="
rg "func \(s \*InMemory\) (Jobs|RelationshipRules)" --type go -A 2
echo ""
echo "=== Check line 667 context for Jobs usage ==="
sed -n '660,675p' ./apps/workspace-engine/pkg/workspace/store/store.go
echo ""
echo "=== Search for s.repo.Jobs usage pattern ==="
rg "s\.repo\.Jobs" --type go -C 2
</parameter>Repository: ctrlplanedev/ctrlplane
Length of output: 3202
Use consistent accessor pattern for RelationshipRules and Jobs repositories.
Line 644 accesses s.repo.RelationshipRules.Items() and line 667 accesses s.repo.Jobs.Items() as direct fields, while other repositories in the same code use method calls: s.repo.Systems().Items(), s.repo.Policies().Items(), s.repo.ResourceVariables().Items(), s.repo.Releases().Items().
InMemory provides wrapper methods RelationshipRulesRepo() and JobsRepo() that follow the same pattern as the other repositories. Update these lines to use the method accessors for consistency:
- Change
s.repo.RelationshipRules.Items()tos.repo.RelationshipRulesRepo().Items() - Change
s.repo.Jobs.Items()tos.repo.JobsRepo().Items()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/store.go` around lines 644 - 649,
Replace direct field access to the in-memory repos with the provided accessor
methods for consistency: change usages of s.repo.RelationshipRules.Items() to
s.repo.RelationshipRulesRepo().Items() and change s.repo.Jobs.Items() to
s.repo.JobsRepo().Items(); update the code blocks that iterate over legacy
relationship rules (where rr is used) and job migrations to call the
RelationshipRulesRepo() and JobsRepo() methods respectively so they match the
existing pattern used by Systems(), Policies(), ResourceVariables(), and
Releases().
…ne evaluator construction
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.go (1)
43-45: Consider returning the interface type for consistency.The constructor returns
*storeGetters(concrete type), whileapproval.NewStoreGettersreturnsGetters(interface type). For consistency across the codebase and to allow easier mocking in tests, consider returning the interface.♻️ Proposed change
-func NewStoreGetters(store *store.Store) *storeGetters { +func NewStoreGetters(store *store.Store) Getters { return &storeGetters{store: store} }🤖 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/getters.go` around lines 43 - 45, The constructor NewStoreGetters currently returns the concrete type *storeGetters; change its signature to return the Getters interface instead (i.e., func NewStoreGetters(store *store.Store) Getters) and keep returning &storeGetters{store: store} so callers receive the interface; this matches approval.NewStoreGetters, improves consistency and testability for mocking of storeGetters.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/store.go (2)
19-25: Consider returning the interface type for consistency.
NewStoreGettersreturns*storeGetters(concrete type), whileapproval.NewStoreGettersreturnsGetters(interface type). For API consistency across the evaluator packages, consider returning the interface.♻️ Proposed change
-func NewStoreGetters(store *store.Store) *storeGetters { +func NewStoreGetters(store *store.Store) Getters { return &storeGetters{ store: store, approvalGetters: approval.NewStoreGetters(store), environmentProgressionGetters: environmentprogression.NewStoreGetters(store), } }🤖 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/store.go` around lines 19 - 25, Change the constructor NewStoreGetters to return the Getters interface instead of the concrete *storeGetters so it matches approval.NewStoreGetters; ensure storeGetters implements the Getters interface and update NewStoreGetters signature to return Getters, returning the same &storeGetters{...} value (callers will receive the interface type). Verify any callers of NewStoreGetters compile with the interface return and adjust imports/usages if they relied on the concrete type.
36-39:HasCurrentReleaseconflates missing data with potential errors.Using
err == nilto determine if a current release exists treats all errors as "no release." Perrelease_targets.go:58-102,GetCurrentReleasereturns genericfmt.Errorffor both business logic conditions ("no successful job found") and potential infrastructure issues. While the current store implementation may not surface infrastructure errors, this pattern could mask failures if error handling changes upstream.Consider documenting this assumption or adding a comment explaining that all errors indicate "no current release."
🤖 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/store.go` around lines 36 - 39, HasCurrentRelease currently treats any error from s.store.ReleaseTargets.GetCurrentRelease as "no release", which can conflate business-not-found vs infrastructure errors; update the HasCurrentRelease implementation (storeGetters.HasCurrentRelease) to either return an error or at minimum add a clear comment above the function documenting the assumption that all errors from GetCurrentRelease are considered "no current release" (referencing GetCurrentRelease in release_targets.go) and why that's acceptable with the current store implementation, so future maintainers won't mistake this behavior for a bug if upstream error handling changes.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (1)
69-84: Consider extracting duplicated timeGetter initialization.The timeGetter setup logic (lines 70-75) is duplicated from
NewEvaluatorFromStore(lines 53-58). Consider extracting this to a helper function to maintain DRY.♻️ Proposed helper extraction
+func getTimeGetter() func() time.Time { + if testTimeGetterFactory != nil { + return testTimeGetterFactory + } + return time.Now +} + func NewEvaluatorFromStore(store *store.Store, rolloutRule *oapi.PolicyRule) evaluator.Evaluator { if rolloutRule == nil || rolloutRule.GradualRollout == nil || store == nil { return nil } - timeGetter := func() time.Time { - return time.Now() - } - if testTimeGetterFactory != nil { - timeGetter = testTimeGetterFactory - } - return evaluator.WithMemoization(&GradualRolloutEvaluator{ getters: NewStoreGetters(store), ruleId: rolloutRule.Id, rule: rolloutRule.GradualRollout, hashingFn: fnvHashingFn, - timeGetter: timeGetter, + timeGetter: getTimeGetter(), }) } func NewEvaluator(getters Getters, rolloutRule *oapi.PolicyRule) *GradualRolloutEvaluator { - timeGetter := func() time.Time { - return time.Now() - } - if testTimeGetterFactory != nil { - timeGetter = testTimeGetterFactory - } - return &GradualRolloutEvaluator{ getters: getters, ruleId: rolloutRule.Id, rule: rolloutRule.GradualRollout, hashingFn: fnvHashingFn, - timeGetter: timeGetter, + timeGetter: getTimeGetter(), } }🤖 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.go` around lines 69 - 84, Extract the duplicated time getter initialization into a small helper function and use it from both NewEvaluator and NewEvaluatorFromStore: move the logic that sets timeGetter to either testTimeGetterFactory (when non-nil) or time.Now into a new function (e.g., makeTimeGetter or timeGetterFromFactory) and replace the inline block in NewEvaluator and NewEvaluatorFromStore with a call to that helper; reference testTimeGetterFactory, NewEvaluator, NewEvaluatorFromStore, and the timeGetter field to ensure correct wiring.
🤖 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/approval/approval.go`:
- Around line 44-46: The interface Getters has a minor formatting issue in the
GetApprovalRecords signature: there is no space after the comma between the ctx
parameter and versionID; update the function signature in the Getters interface
(GetApprovalRecords(ctx context.Context, versionID, environmentID string)
([]*oapi.UserApprovalRecord, error)) to add the missing space after the comma so
it reads "ctx context.Context, versionID".
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/store.go`:
- Around line 19-21: The current storeGetters.GetApprovalRecords always returns
nil error because store.UserApprovalRecords.GetApprovalRecords swallows repo
errors; update the repository and store layers to propagate errors instead of
swallowing them: change user_approval_records.go GetApprovalRecords to return
([]*oapi.UserApprovalRecord, error) and bubble that error through
store.UserApprovalRecords.GetApprovalRecords and storeGetters.GetApprovalRecords
(so storeGetters.GetApprovalRecords returns the underlying error), then adjust
callers such as the approval.go branch handling (lines around approval
processing) to use the propagated error; alternatively, if you intend no errors
ever, remove the error from the Getters interface and all
implementations/callers—but prefer propagating the error for correct upstream
handling.
---
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.go`:
- Around line 43-45: The constructor NewStoreGetters currently returns the
concrete type *storeGetters; change its signature to return the Getters
interface instead (i.e., func NewStoreGetters(store *store.Store) Getters) and
keep returning &storeGetters{store: store} so callers receive the interface;
this matches approval.NewStoreGetters, improves consistency and testability for
mocking of storeGetters.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go`:
- Around line 69-84: Extract the duplicated time getter initialization into a
small helper function and use it from both NewEvaluator and
NewEvaluatorFromStore: move the logic that sets timeGetter to either
testTimeGetterFactory (when non-nil) or time.Now into a new function (e.g.,
makeTimeGetter or timeGetterFromFactory) and replace the inline block in
NewEvaluator and NewEvaluatorFromStore with a call to that helper; reference
testTimeGetterFactory, NewEvaluator, NewEvaluatorFromStore, and the timeGetter
field to ensure correct wiring.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/store.go`:
- Around line 19-25: Change the constructor NewStoreGetters to return the
Getters interface instead of the concrete *storeGetters so it matches
approval.NewStoreGetters; ensure storeGetters implements the Getters interface
and update NewStoreGetters signature to return Getters, returning the same
&storeGetters{...} value (callers will receive the interface type). Verify any
callers of NewStoreGetters compile with the interface return and adjust
imports/usages if they relied on the concrete type.
- Around line 36-39: HasCurrentRelease currently treats any error from
s.store.ReleaseTargets.GetCurrentRelease as "no release", which can conflate
business-not-found vs infrastructure errors; update the HasCurrentRelease
implementation (storeGetters.HasCurrentRelease) to either return an error or at
minimum add a clear comment above the function documenting the assumption that
all errors from GetCurrentRelease are considered "no current release"
(referencing GetCurrentRelease in release_targets.go) and why that's acceptable
with the current store implementation, so future maintainers won't mistake this
behavior for a bug if upstream error handling changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f151236-088d-4911-9815-aebb558651de
📒 Files selected for processing (8)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/approval.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/store.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/environment_summary.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/store.go
Comment on lines
44
to
46
| type Getters interface { | ||
| GetApprovalRecords(versionID, environmentID string) []*oapi.UserApprovalRecord | ||
| GetApprovalRecords(ctx context.Context,versionID, environmentID string) ([]*oapi.UserApprovalRecord, error) | ||
| } |
Contributor
There was a problem hiding this comment.
Minor formatting: missing space after comma.
There's a missing space after the comma between ctx context.Context, and versionID.
🔧 Proposed fix
type Getters interface {
- GetApprovalRecords(ctx context.Context,versionID, environmentID string) ([]*oapi.UserApprovalRecord, error)
+ GetApprovalRecords(ctx context.Context, versionID, environmentID string) ([]*oapi.UserApprovalRecord, error)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type Getters interface { | |
| GetApprovalRecords(versionID, environmentID string) []*oapi.UserApprovalRecord | |
| GetApprovalRecords(ctx context.Context,versionID, environmentID string) ([]*oapi.UserApprovalRecord, error) | |
| } | |
| type Getters interface { | |
| GetApprovalRecords(ctx context.Context, versionID, environmentID string) ([]*oapi.UserApprovalRecord, error) | |
| } |
🤖 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/approval/approval.go`
around lines 44 - 46, The interface Getters has a minor formatting issue in the
GetApprovalRecords signature: there is no space after the comma between the ctx
parameter and versionID; update the function signature in the Getters interface
(GetApprovalRecords(ctx context.Context, versionID, environmentID string)
([]*oapi.UserApprovalRecord, error)) to add the missing space after the comma so
it reads "ctx context.Context, versionID".
Comment on lines
+19
to
+21
| func (s *storeGetters) GetApprovalRecords(ctx context.Context, versionID, environmentID string) ([]*oapi.UserApprovalRecord, error) { | ||
| return s.store.UserApprovalRecords.GetApprovalRecords(versionID, environmentID), nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the store method signature and error handling
ast-grep --pattern 'func ($_ *UserApprovalRecords) GetApprovalRecords($_, $_) $_ {
$$$
}'Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Find the approval package directory structure
find . -type f -name "store.go" -path "*/approval/*" | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Also look for user_approval_records.go and approval.go
find . -type f -name "user_approval_records.go" -path "*/approval/*"
find . -type f -name "approval.go" -path "*/approval/*"Repository: ctrlplanedev/ctrlplane
Length of output: 156
🏁 Script executed:
#!/bin/bash
# List files in the approval directory to understand structure
find . -type d -name "approval" -path "*/workspace/*"Repository: ctrlplanedev/ctrlplane
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Read the store.go file to see the GetApprovalRecords implementation
cat -n ./apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/store.goRepository: ctrlplanedev/ctrlplane
Length of output: 722
🏁 Script executed:
#!/bin/bash
# Read approval.go to check the error handling at lines 116-123
cat -n ./apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/approval.goRepository: ctrlplanedev/ctrlplane
Length of output: 6993
🏁 Script executed:
#!/bin/bash
# Search for user_approval_records.go file
find . -type f -name "user_approval_records.go"Repository: ctrlplanedev/ctrlplane
Length of output: 221
🏁 Script executed:
#!/bin/bash
# Read the user_approval_records.go file from the store package
cat -n ./apps/workspace-engine/pkg/workspace/store/user_approval_records.goRepository: ctrlplanedev/ctrlplane
Length of output: 3443
🏁 Script executed:
#!/bin/bash
# Check if there are other implementations of the Getters interface
rg "type.*struct" -A 5 -B 5 apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/ | grep -A 10 "Getters"Repository: ctrlplanedev/ctrlplane
Length of output: 2189
🏁 Script executed:
#!/bin/bash
# Search for all implementations of Getters interface
rg "func.*GetApprovalRecords" apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/approval/Repository: ctrlplanedev/ctrlplane
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Check the repository interface to see if GetApprovedByVersionAndEnvironment returns an error
rg "GetApprovedByVersionAndEnvironment" --type go -A 3 -B 3 | head -40Repository: ctrlplanedev/ctrlplane
Length of output: 4197
Error is always nil, making upstream error handling unreachable.
The GetApprovalRecords method in store.go always returns nil as the error because the underlying store.UserApprovalRecords.GetApprovalRecords method (at user_approval_records.go:76-91) catches errors from the repository layer, logs them, and returns only a slice. This makes the error handling branch in approval.go (lines 115-123) unreachable dead code, and any database errors are silently swallowed.
Consider either:
- Propagating errors from the underlying repository through the store layer to the interface, or
- Removing the error return type from the
Gettersinterface if errors will never be returned.
🤖 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/approval/store.go`
around lines 19 - 21, The current storeGetters.GetApprovalRecords always returns
nil error because store.UserApprovalRecords.GetApprovalRecords swallows repo
errors; update the repository and store layers to propagate errors instead of
swallowing them: change user_approval_records.go GetApprovalRecords to return
([]*oapi.UserApprovalRecord, error) and bubble that error through
store.UserApprovalRecords.GetApprovalRecords and storeGetters.GetApprovalRecords
(so storeGetters.GetApprovalRecords returns the underlying error), then adjust
callers such as the approval.go branch handling (lines around approval
processing) to use the propagated error; alternatively, if you intend no errors
ever, remove the error from the Getters interface and all
implementations/callers—but prefer propagating the error for correct upstream
handling.
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
2 participants
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.
…a access and rename evaluator constructor
Summary by CodeRabbit
New Features
Bug Fixes
Refactor