Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a policy-summary reconciliation feature: new policysummary controller, enqueuing/events, scope parsers, reconciliation logic that evaluates policies per scope, evaluator helpers, DB schema for policy_rule_summary, and Postgres-backed Getter/Setter scaffolding. Changes
Sequence DiagramsequenceDiagram
participant Controller as Controller
participant Reconciler as Reconciler
participant Getter as Getter
participant Evaluator as EvaluatorFactory
participant Setter as Setter
Controller->>Reconciler: Reconcile(workspaceID, scopeType, scopeID)
Reconciler->>Reconciler: Parse scopeID
alt Environment
Reconciler->>Getter: GetEnvironment(envID)
Getter-->>Reconciler: Environment
Reconciler->>Getter: GetPoliciesForEnvironment(wsID, envID)
else EnvironmentVersion
Reconciler->>Getter: GetEnvironment(envID)
Getter-->>Reconciler: Environment
Reconciler->>Getter: GetVersion(versionID)
Getter-->>Reconciler: Version
Reconciler->>Getter: GetPoliciesForEnvironment(wsID, envID)
else DeploymentVersion
Reconciler->>Getter: GetDeployment(deploymentID)
Getter-->>Reconciler: Deployment
Reconciler->>Getter: GetVersion(versionID)
Getter-->>Reconciler: Version
Reconciler->>Getter: GetPoliciesForDeployment(wsID, deploymentID)
end
Getter-->>Reconciler: Policies
Reconciler->>Evaluator: EvaluatorsForScope(store, rule)
Evaluator-->>Reconciler: Evaluators[]
Reconciler->>Reconciler: Evaluate rules → collect RuleSummaryRows
Reconciler->>Setter: UpsertRuleSummaries(rows)
Setter-->>Reconciler: OK
Reconciler-->>Controller: ReconcileResult{NextReconcileAt}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
♻️ Duplicate comments (2)
apps/workspace-engine/svc/controllers/policysummary/reconcile.go (2)
131-134:⚠️ Potential issue | 🟠 MajorTODO returns nil; no evaluators will be executed for deployment-version scope.
Same issue:
EvaluatorsForDeploymentVersionSummaryexists but requires*store.Store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go` around lines 131 - 134, The callback passed to r.evaluateAndCollect currently returns nil so deployment-version scoped evaluators never run; replace the TODO by calling EvaluatorsForDeploymentVersionSummary with the required *store.Store instance (e.g., the controller's store or r.store) and returning its result. Locate the anonymous func(rule *oapi.PolicyRule) []evaluator.Evaluator in reconcile.go and change it to invoke evaluatorpkg.EvaluatorsForDeploymentVersionSummary(rule, yourStore) (or the correct package name) to produce and return the evaluators, ensuring you pass the existing store pointer used elsewhere in this controller.
87-91:⚠️ Potential issue | 🟠 MajorTODO returns nil; no evaluators will be executed for environment-version scope.
Same issue as
reconcileEnvironment:EvaluatorsForEnvironmentVersionSummaryexists but requires a*store.Storethat isn't available in the reconciler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go` around lines 87 - 91, The current lambda passed to evaluateAndCollect returns nil so no environment-version evaluators run; use EvaluatorsForEnvironmentVersionSummary to build evaluators but it needs a *store.Store which the reconciler currently lacks. Add a store dependency to the reconciler (e.g., a field on the reconciler struct and wire it via the reconciler constructor) or accept a store parameter where reconcile is invoked, then replace the nil-returning closure with a call that invokes EvaluatorsForEnvironmentVersionSummary(rule, r.store) (or the correct parameter order) to return the evaluators; mirror the same fix applied to reconcileEnvironment to ensure evaluators execute for the environment-version scope.
🧹 Nitpick comments (3)
apps/workspace-engine/svc/controllers/policysummary/controller.go (1)
59-62: Consider usinglog.Errorinstead oflog.Fatalbefore panic.Charmbracelet's
log.Fataldoes not callos.Exit, so thepanicis necessary here. However, usinglog.Fatalfollowed bypanicmay confuse readers who expectFatalto terminate. Usinglog.Errorwith the panic makes the control flow clearer.Suggested change
if pgxPool == nil { - log.Fatal("Failed to get pgx pool") + log.Error("Failed to get pgx pool") panic("failed to get pgx pool") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policysummary/controller.go` around lines 59 - 62, Replace the confusing log.Fatal followed by panic when checking pgxPool by logging with log.Error and then panicking; specifically, in the controller.go block that checks if pgxPool == nil (the pgxPool nil-check), change the call from log.Fatal("Failed to get pgx pool") to log.Error(...) to make it clear the panic is causing termination while still logging the failure before calling panic("failed to get pgx pool").apps/workspace-engine/svc/controllers/policysummary/reconcile.go (1)
174-174:uuid.MustParse(p.Id)will panic on invalid UUID.If policy IDs from the database are guaranteed to be valid UUIDs, this is fine. Otherwise, consider handling the error to avoid panicking the worker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go` at line 174, The code is using uuid.MustParse(p.Id) which will panic on invalid UUIDs; locate the construction that sets PolicyID (the PolicyID: uuid.MustParse(p.Id) assignment in reconcile.go) and replace MustParse with a safe parse (uuid.Parse) and handle the error—e.g., log the invalid p.Id and return or skip the affected policy rather than allowing a panic; ensure the surrounding function (reconcile handler) either returns an error or continues gracefully when uuid.Parse fails so the worker does not crash.apps/workspace-engine/svc/controllers/policysummary/getters_postgres.go (1)
16-39: All getter methods are stubs; reconciliation will fail at runtime.These stubs will cause all three reconcile paths (
reconcileEnvironment,reconcileEnvironmentVersion,reconcileDeploymentVersion) to fail immediately when they callGetEnvironment,GetDeployment, orGetVersion. Since the controller is already registered inmain.go, this will trigger retry loops.The stub structure and interface assertion are correct. Ensure implementations are added before this feature goes live, or disable controller registration until ready.
Would you like me to help generate the SQL query implementations for these getter methods?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policysummary/getters_postgres.go` around lines 16 - 39, The PostgresGetter methods are all unimplemented stubs causing reconciles to fail; implement GetEnvironment, GetDeployment, GetVersion, GetPoliciesForEnvironment, and GetPoliciesForDeployment on the PostgresGetter by querying the respective tables and mapping results to the oapi types: use SQL SELECT against environment -> populate oapi.Environment in GetEnvironment, deployment -> populate oapi.Deployment in GetDeployment, deployment_version -> populate oapi.DeploymentVersion in GetVersion, and select policies (filter by workspace_id and match selector semantics for the target environment or deployment) in GetPoliciesForEnvironment and GetPoliciesForDeployment; use the existing DB client on PostgresGetter (e.g., g.db / g.pool), handle sql.ErrNoRows and other errors properly, and return the populated objects or a clear error.
🤖 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/policysummary/controller.go`:
- Around line 80-83: The Controller is being instantiated in New() with
PostgresGetter and PostgresSetter which are unimplemented and will cause
reconciliation failures; update the New() function or registration so this
controller is not active until implementations exist: either (A) change New() to
return an error or nil controller when getter/setter are not implemented and
ensure callers in main only register if New() returns a valid controller, or (B)
add a feature flag (e.g., enablePolicySummaryController) checked in main before
registering the Controller and gate instantiation/registration behind that flag;
reference the Controller type, New() function, PostgresGetter and PostgresSetter
and the point in main where the controller is registered to implement the
conditional registration path.
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go`:
- Around line 45-48: The TODO in reconcile.go causes environment-scoped
evaluators to be nil because evaluateAndCollect is passed a factory that returns
nil; replace that with a call to the proper evaluator factory
(policy.EvaluatorsForEnvironmentSummary) and ensure the reconciler can supply
the required *store.Store: add a store field to the reconciler struct (or
otherwise inject a *store.Store), initialize it where the reconciler is
constructed, and then change the lambda passed to evaluateAndCollect to call
policy.EvaluatorsForEnvironmentSummary(r.store, rule) for each *oapi.PolicyRule;
alternatively, if you prefer not to add a store, refactor
EvaluatorsForEnvironmentSummary in evaluators.go to accept no store or an
interface that you can construct here and call that instead.
In `@apps/workspace-engine/svc/controllers/policysummary/setters_postgres.go`:
- Around line 12-20: The UpsertRuleSummaries stub currently returns a "not
implemented" error which causes reconciliation to fail; either implement the
real batch upsert or make a safe temporary change: inside
PostgresSetter.UpsertRuleSummaries replace the fmt.Errorf("not implemented")
with either (A) a full batch INSERT ... ON CONFLICT (rule_id, deployment_id,
environment_id, version_id) DO UPDATE statement that sets allowed, message, and
other fields for the provided rows and executes it against the DB, or (B) for a
temporary incremental merge, log a warning (including len(rows)) that upsert is
disabled and return nil to avoid reconciliation retries; ensure changes are
inside the PostgresSetter.UpsertRuleSummaries method referenced by the
controller (reconcile.go) so errors won't cause infinite retries.
---
Duplicate comments:
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go`:
- Around line 131-134: The callback passed to r.evaluateAndCollect currently
returns nil so deployment-version scoped evaluators never run; replace the TODO
by calling EvaluatorsForDeploymentVersionSummary with the required *store.Store
instance (e.g., the controller's store or r.store) and returning its result.
Locate the anonymous func(rule *oapi.PolicyRule) []evaluator.Evaluator in
reconcile.go and change it to invoke
evaluatorpkg.EvaluatorsForDeploymentVersionSummary(rule, yourStore) (or the
correct package name) to produce and return the evaluators, ensuring you pass
the existing store pointer used elsewhere in this controller.
- Around line 87-91: The current lambda passed to evaluateAndCollect returns nil
so no environment-version evaluators run; use
EvaluatorsForEnvironmentVersionSummary to build evaluators but it needs a
*store.Store which the reconciler currently lacks. Add a store dependency to the
reconciler (e.g., a field on the reconciler struct and wire it via the
reconciler constructor) or accept a store parameter where reconcile is invoked,
then replace the nil-returning closure with a call that invokes
EvaluatorsForEnvironmentVersionSummary(rule, r.store) (or the correct parameter
order) to return the evaluators; mirror the same fix applied to
reconcileEnvironment to ensure evaluators execute for the environment-version
scope.
---
Nitpick comments:
In `@apps/workspace-engine/svc/controllers/policysummary/controller.go`:
- Around line 59-62: Replace the confusing log.Fatal followed by panic when
checking pgxPool by logging with log.Error and then panicking; specifically, in
the controller.go block that checks if pgxPool == nil (the pgxPool nil-check),
change the call from log.Fatal("Failed to get pgx pool") to log.Error(...) to
make it clear the panic is causing termination while still logging the failure
before calling panic("failed to get pgx pool").
In `@apps/workspace-engine/svc/controllers/policysummary/getters_postgres.go`:
- Around line 16-39: The PostgresGetter methods are all unimplemented stubs
causing reconciles to fail; implement GetEnvironment, GetDeployment, GetVersion,
GetPoliciesForEnvironment, and GetPoliciesForDeployment on the PostgresGetter by
querying the respective tables and mapping results to the oapi types: use SQL
SELECT against environment -> populate oapi.Environment in GetEnvironment,
deployment -> populate oapi.Deployment in GetDeployment, deployment_version ->
populate oapi.DeploymentVersion in GetVersion, and select policies (filter by
workspace_id and match selector semantics for the target environment or
deployment) in GetPoliciesForEnvironment and GetPoliciesForDeployment; use the
existing DB client on PostgresGetter (e.g., g.db / g.pool), handle sql.ErrNoRows
and other errors properly, and return the populated objects or a clear error.
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go`:
- Line 174: The code is using uuid.MustParse(p.Id) which will panic on invalid
UUIDs; locate the construction that sets PolicyID (the PolicyID:
uuid.MustParse(p.Id) assignment in reconcile.go) and replace MustParse with a
safe parse (uuid.Parse) and handle the error—e.g., log the invalid p.Id and
return or skip the affected policy rather than allowing a panic; ensure the
surrounding function (reconcile handler) either returns an error or continues
gracefully when uuid.Parse fails so the worker does not crash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 031cacd0-3e15-4156-ae59-dc1b595e8040
📒 Files selected for processing (12)
apps/workspace-engine/main.goapps/workspace-engine/pkg/reconcile/events/policysummary.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluators.goapps/workspace-engine/svc/controllers/policysummary/controller.goapps/workspace-engine/svc/controllers/policysummary/getters.goapps/workspace-engine/svc/controllers/policysummary/getters_postgres.goapps/workspace-engine/svc/controllers/policysummary/reconcile.goapps/workspace-engine/svc/controllers/policysummary/scope.goapps/workspace-engine/svc/controllers/policysummary/setters.goapps/workspace-engine/svc/controllers/policysummary/setters_postgres.gopackages/db/src/schema/index.tspackages/db/src/schema/policy-rule-summary.ts
Comment on lines
+80
to
+83
| controller := &Controller{ | ||
| getter: &PostgresGetter{}, | ||
| setter: &PostgresSetter{}, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if PostgresGetter/PostgresSetter have real implementations or are all stubs
rg -n "not implemented" apps/workspace-engine/svc/controllers/policysummary/Repository: ctrlplanedev/ctrlplane
Length of output: 768
🏁 Script executed:
# Check if controller is registered in main.go and at what line
rg -n "policysummary" apps/workspace-engine/svc/main.go -A 3 -B 3Repository: ctrlplanedev/ctrlplane
Length of output: 139
🏁 Script executed:
# Look for retry logic and reconciliation behavior
rg -n "reconcil|retry|MaxRetry" apps/workspace-engine/svc/controllers/policysummary/ -A 2Repository: ctrlplanedev/ctrlplane
Length of output: 7989
🏁 Script executed:
# Find main.go in the repository
fd "main\.go" apps/workspace-engine/Repository: ctrlplanedev/ctrlplane
Length of output: 95
🏁 Script executed:
# Search for policysummary controller registration anywhere in the codebase
rg -n "policysummary.*[Cc]ontroller\|NewPolicySummaryController\|policysummary" apps/workspace-engine/ --type go | grep -i "register\|new\|init"Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check main.go for policysummary controller
cat -n apps/workspace-engine/main.go | grep -A 5 -B 5 "policysummary\|policysum"Repository: ctrlplanedev/ctrlplane
Length of output: 1162
🏁 Script executed:
# Also search broadly for NewPolicySummaryController or controller.New
rg -n "func New" apps/workspace-engine/svc/controllers/policysummary/Repository: ctrlplanedev/ctrlplane
Length of output: 329
🏁 Script executed:
# Read the New function to see if it instantiates the Controller with stub implementations
sed -n '58,83p' apps/workspace-engine/svc/controllers/policysummary/controller.goRepository: ctrlplanedev/ctrlplane
Length of output: 779
Controller with unimplemented getter/setter is registered and will cause reconciliation failures.
The New() function instantiates the Controller with PostgresGetter and PostgresSetter that return "not implemented" errors for all methods. This controller is actively registered in main.go line 71 and will cause any enqueued policy summary items to fail reconciliation, triggering retries up to the MaxRetryBackoff limit of 10 seconds.
Either defer registering this controller until implementations are complete, or add a feature flag to disable processing during development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/policysummary/controller.go` around
lines 80 - 83, The Controller is being instantiated in New() with PostgresGetter
and PostgresSetter which are unimplemented and will cause reconciliation
failures; update the New() function or registration so this controller is not
active until implementations exist: either (A) change New() to return an error
or nil controller when getter/setter are not implemented and ensure callers in
main only register if New() returns a valid controller, or (B) add a feature
flag (e.g., enablePolicySummaryController) checked in main before registering
the Controller and gate instantiation/registration behind that flag; reference
the Controller type, New() function, PostgresGetter and PostgresSetter and the
point in main where the controller is registered to implement the conditional
registration path.
Comment on lines
+45
to
+48
| rows, nextTime := r.evaluateAndCollect(ctx, policies, evalScope, func(rule *oapi.PolicyRule) []evaluator.Evaluator { | ||
| // TODO: return only environment-scoped summary evaluators (deployment window) | ||
| return nil | ||
| }) |
Contributor
There was a problem hiding this comment.
TODO returns nil; no evaluators will be executed for environment scope.
This TODO should likely call policy.EvaluatorsForEnvironmentSummary(store, rule) from evaluators.go, but there's no store available in this context. The reconciler needs access to a *store.Store to instantiate evaluators.
Consider adding the store to the reconciler struct, or refactor the evaluator factories to not require a store if they can operate without it in the summary context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go` around
lines 45 - 48, The TODO in reconcile.go causes environment-scoped evaluators to
be nil because evaluateAndCollect is passed a factory that returns nil; replace
that with a call to the proper evaluator factory
(policy.EvaluatorsForEnvironmentSummary) and ensure the reconciler can supply
the required *store.Store: add a store field to the reconciler struct (or
otherwise inject a *store.Store), initialize it where the reconciler is
constructed, and then change the lambda passed to evaluateAndCollect to call
policy.EvaluatorsForEnvironmentSummary(r.store, rule) for each *oapi.PolicyRule;
alternatively, if you prefer not to add a store, refactor
EvaluatorsForEnvironmentSummary in evaluators.go to accept no store or an
interface that you can construct here and call that instead.
Comment on lines
+12
to
+20
| func (s *PostgresSetter) UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error { | ||
| // TODO: batch upsert into policy_rule_summary table | ||
| // ON CONFLICT (rule_id, deployment_id, environment_id, version_id) DO UPDATE | ||
| // SET allowed = EXCLUDED.allowed, message = EXCLUDED.message, ... | ||
| if len(rows) == 0 { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("not implemented") | ||
| } |
Contributor
There was a problem hiding this comment.
Stub implementation will cause reconciliation to fail indefinitely.
The "not implemented" error will propagate to the controller and cause infinite retries for any policy summary event with rows to upsert. Per the context in reconcile.go, errors from UpsertRuleSummaries are returned as reconciliation failures.
If this PR is intended to be merged incrementally before the implementation is complete, consider either:
- Logging and returning
nilto allow graceful degradation (rows would be silently dropped) - Gating the policysummary controller registration behind a feature flag until the implementation is ready
Option 1: Graceful degradation with logging
func (s *PostgresSetter) UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error {
// TODO: batch upsert into policy_rule_summary table
// ON CONFLICT (rule_id, deployment_id, environment_id, version_id) DO UPDATE
// SET allowed = EXCLUDED.allowed, message = EXCLUDED.message, ...
if len(rows) == 0 {
return nil
}
- return fmt.Errorf("not implemented")
+ // TODO: implement batch upsert - currently a no-op
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *PostgresSetter) UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error { | |
| // TODO: batch upsert into policy_rule_summary table | |
| // ON CONFLICT (rule_id, deployment_id, environment_id, version_id) DO UPDATE | |
| // SET allowed = EXCLUDED.allowed, message = EXCLUDED.message, ... | |
| if len(rows) == 0 { | |
| return nil | |
| } | |
| return fmt.Errorf("not implemented") | |
| } | |
| func (s *PostgresSetter) UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error { | |
| // TODO: batch upsert into policy_rule_summary table | |
| // ON CONFLICT (rule_id, deployment_id, environment_id, version_id) DO UPDATE | |
| // SET allowed = EXCLUDED.allowed, message = EXCLUDED.message, ... | |
| if len(rows) == 0 { | |
| return nil | |
| } | |
| // TODO: implement batch upsert - currently a no-op | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/policysummary/setters_postgres.go`
around lines 12 - 20, The UpsertRuleSummaries stub currently returns a "not
implemented" error which causes reconciliation to fail; either implement the
real batch upsert or make a safe temporary change: inside
PostgresSetter.UpsertRuleSummaries replace the fmt.Errorf("not implemented")
with either (A) a full batch INSERT ... ON CONFLICT (rule_id, deployment_id,
environment_id, version_id) DO UPDATE statement that sets allowed, message, and
other fields for the provided rows and executes it against the DB, or (B) for a
temporary incremental merge, log a warning (including len(rows)) that upsert is
disabled and return nil to avoid reconciliation retries; ensure changes are
inside the PostgresSetter.UpsertRuleSummaries method referenced by the
controller (reconcile.go) so errors won't cause infinite retries.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/workspace-engine/svc/controllers/policysummary/reconcile.go (1)
45-48:⚠️ Potential issue | 🟠 MajorThese factories currently disable every summary path.
evaluateAndCollectonly emits rows from the evaluators returned here, but all three handlers returnnil, so every reconcile completes with no evaluations, no summary rows, and no next requeue time. This means the controller is merged with its core behavior still stubbed out.Also applies to: 87-91, 131-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go` around lines 45 - 48, The three inline factory callbacks passed to evaluateAndCollect are returning nil (disabling all summary paths); replace each stub with real evaluator factories that return the correct slice of evaluator.Evaluator for the rule’s scope instead of nil—implement the deployment-window/environment-scoped factory (used at the shown call), the cluster-scoped factory (lines ~87-91), and the namespace-scoped factory (lines ~131-134) so evaluateAndCollect can emit rows and nextTime; inside each factory, inspect the provided *oapi.PolicyRule and construct/return the appropriate summary evaluator(s) (e.g., deployment-window evaluator for environment-scoped rules, cluster-level summary evaluator for cluster-scoped rules, namespace-level summary evaluator for namespace-scoped rules) rather than returning nil.
🧹 Nitpick comments (1)
apps/workspace-engine/svc/controllers/policysummary/reconcile.go (1)
15-17: Add doc comments for the exported API.
ReconcileResultandReconcileare exported but undocumented, so they won't show useful package docs.As per coding guidelines, "document exported functions/types/methods".
Also applies to: 190-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go` around lines 15 - 17, Add package-level doc comments for the exported API: provide a concise comment above the ReconcileResult type describing its purpose and the meaning of NextReconcileAt, and add a comment above the exported Reconcile function (or method named Reconcile) that explains what the function does, its input parameters, return values and when NextReconcileAt is used; ensure comments follow Go doc comment conventions (start with the symbol name) so the documentation is generated correctly.
🤖 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/policysummary/reconcile.go`:
- Around line 36-38: The EvaluatorScope created in reconcile.go only sets
Environment, but the environment-summary evaluator requires both
ScopeEnvironment and ScopeVersion; update the evalScope construction in
reconcile.go to populate the Version (or corresponding version field/property)
on evaluator.EvaluatorScope so the scope satisfies ScopeEnvironment |
ScopeVersion (e.g., evalScope := evaluator.EvaluatorScope{ Environment: env,
Version: env.Version } or otherwise set the version/fields used by HasFields),
ensuring the summary evaluator is not skipped by HasFields.
- Around line 172-175: Replace uses of uuid.MustParse for external inputs with
safe parsing: call uuid.Parse (or equivalent) on both workspaceID and rule.Id
where RuleSummaryRow is constructed (the append in reconcile.go and the similar
site at the later occurrence around line 192), check the returned error, and
propagate a reconcile-friendly error (or skip the invalid record with a logged
warning) instead of panicking; update the RuleSummaryRow construction to use the
successfully parsed uuid values only after error handling so malformed UUIDs
produce a controlled error path.
In `@apps/workspace-engine/svc/controllers/policysummary/setters.go`:
- Around line 21-23: The Setter implementation is currently a stub that returns
“not implemented” for any non-empty batch, so UpsertRuleSummaries failures will
break reconciles; update the concrete implementation (the type that implements
Setter in setters_postgres.go) to correctly handle non-empty rows: implement
UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error to perform
the intended persistence/upsert (or at minimum accept and persist batches or
return nil for valid input) instead of always returning an error, ensuring it
matches the interface used by controller.go (the controller wiring expects a
working Setter) and follows the existing DB transaction/connection patterns in
that package.
---
Duplicate comments:
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go`:
- Around line 45-48: The three inline factory callbacks passed to
evaluateAndCollect are returning nil (disabling all summary paths); replace each
stub with real evaluator factories that return the correct slice of
evaluator.Evaluator for the rule’s scope instead of nil—implement the
deployment-window/environment-scoped factory (used at the shown call), the
cluster-scoped factory (lines ~87-91), and the namespace-scoped factory (lines
~131-134) so evaluateAndCollect can emit rows and nextTime; inside each factory,
inspect the provided *oapi.PolicyRule and construct/return the appropriate
summary evaluator(s) (e.g., deployment-window evaluator for environment-scoped
rules, cluster-level summary evaluator for cluster-scoped rules, namespace-level
summary evaluator for namespace-scoped rules) rather than returning nil.
---
Nitpick comments:
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go`:
- Around line 15-17: Add package-level doc comments for the exported API:
provide a concise comment above the ReconcileResult type describing its purpose
and the meaning of NextReconcileAt, and add a comment above the exported
Reconcile function (or method named Reconcile) that explains what the function
does, its input parameters, return values and when NextReconcileAt is used;
ensure comments follow Go doc comment conventions (start with the symbol name)
so the documentation is generated correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec93842a-bf73-457d-a2d7-569e86a6ca27
📒 Files selected for processing (3)
apps/workspace-engine/svc/controllers/policysummary/reconcile.goapps/workspace-engine/svc/controllers/policysummary/setters.gopackages/db/src/schema/policy-rule-summary.ts
Comment on lines
+36
to
+38
| evalScope := evaluator.EvaluatorScope{ | ||
| Environment: env, | ||
| } |
Contributor
There was a problem hiding this comment.
The environment scope does not satisfy the existing summary evaluator contract.
The current environment scope only sets Environment, but the environment-summary path in apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluators.go:43-51 builds the deployment-window summary evaluator, which currently requires ScopeEnvironment | ScopeVersion. HasFields on Line 167 will skip it even after Lines 45-48 are wired up, so this scope still won't produce summaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go` around
lines 36 - 38, The EvaluatorScope created in reconcile.go only sets Environment,
but the environment-summary evaluator requires both ScopeEnvironment and
ScopeVersion; update the evalScope construction in reconcile.go to populate the
Version (or corresponding version field/property) on evaluator.EvaluatorScope so
the scope satisfies ScopeEnvironment | ScopeVersion (e.g., evalScope :=
evaluator.EvaluatorScope{ Environment: env, Version: env.Version } or otherwise
set the version/fields used by HasFields), ensuring the summary evaluator is not
skipped by HasFields.
Comment on lines
+172
to
+175
| rows = append(rows, RuleSummaryRow{ | ||
| WorkspaceID: r.workspaceID, | ||
| RuleID: uuid.MustParse(rule.Id), | ||
| Evaluation: result, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does github.com/google/uuid.MustParse panic on invalid UUID input, and is uuid.Parse the non-panicking alternative intended for request or queue data?
💡 Result:
Yes.
uuid.MustParse(s)panics ifscan’t be parsed as a UUID (it callsParseand panics on a non-nil error). (chromium.googlesource.com)uuid.Parse(s)is the non-panicking alternative: it returns(UUID, error)and reports invalid input via the error. (chromium.googlesource.com)- The package docs explicitly describe
MustParseas a convenience for safe initialization of global variables / compiled UUID constants, not for untrusted runtime input like HTTP requests or queue messages—so for request/queue data,uuid.Parse(with error handling) is the intended choice. (pkg.go.dev)
Citations:
- 1: https://chromium.googlesource.com/external/github.com/google/uuid/%2B/ae25fc6a8e2fa9c3a72fc325117330736fe729f6/uuid.go
- 2: https://chromium.googlesource.com/external/github.com/google/uuid/%2B/ae25fc6a8e2fa9c3a72fc325117330736fe729f6/uuid.go
- 3: https://pkg.go.dev/github.com/google/uuid
🏁 Script executed:
cat -n apps/workspace-engine/svc/controllers/policysummary/reconcile.go | sed -n '1,50p'Repository: ctrlplanedev/ctrlplane
Length of output: 1706
🏁 Script executed:
cat -n apps/workspace-engine/svc/controllers/policysummary/reconcile.go | sed -n '160,200p'Repository: ctrlplanedev/ctrlplane
Length of output: 1381
Avoid MustParse on externally-sourced UUIDs.
Both workspaceID (function parameter) and rule.Id (from database result) are external inputs. A malformed UUID will panic the worker instead of returning a reconcile error, which is a poor failure mode for a background controller.
Safer parse flow
func Reconcile(ctx context.Context, workspaceID string, scopeType string, scopeID string, getter Getter, setter Setter) (*ReconcileResult, error) {
+ parsedWorkspaceID, err := uuid.Parse(workspaceID)
+ if err != nil {
+ return nil, fmt.Errorf("parse workspace id: %w", err)
+ }
+
r := &reconciler{
- workspaceID: uuid.MustParse(workspaceID),
+ workspaceID: parsedWorkspaceID,
getter: getter,
setter: setter,
}- rows = append(rows, RuleSummaryRow{
+ ruleID, err := uuid.Parse(rule.Id)
+ if err != nil {
+ continue
+ }
+
+ rows = append(rows, RuleSummaryRow{
WorkspaceID: r.workspaceID,
- RuleID: uuid.MustParse(rule.Id),
+ RuleID: ruleID,
Evaluation: result,
})Also applies to: line 192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go` around
lines 172 - 175, Replace uses of uuid.MustParse for external inputs with safe
parsing: call uuid.Parse (or equivalent) on both workspaceID and rule.Id where
RuleSummaryRow is constructed (the append in reconcile.go and the similar site
at the later occurrence around line 192), check the returned error, and
propagate a reconcile-friendly error (or skip the invalid record with a logged
warning) instead of panicking; update the RuleSummaryRow construction to use the
successfully parsed uuid values only after error handling so malformed UUIDs
produce a controlled error path.
Comment on lines
+21
to
+23
| type Setter interface { | ||
| UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error | ||
| } |
Contributor
There was a problem hiding this comment.
The new write path is still non-functional.
The only implementation shown for this interface, apps/workspace-engine/svc/controllers/policysummary/setters_postgres.go:1-20, still returns not implemented for any non-empty batch, and apps/workspace-engine/svc/controllers/policysummary/controller.go:54-95 wires that setter into production. Once the evaluator TODOs are filled, every successful reconcile will fail at persistence time instead of writing summaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/policysummary/setters.go` around lines
21 - 23, The Setter implementation is currently a stub that returns “not
implemented” for any non-empty batch, so UpsertRuleSummaries failures will break
reconciles; update the concrete implementation (the type that implements Setter
in setters_postgres.go) to correctly handle non-empty rows: implement
UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error to perform
the intended persistence/upsert (or at minimum accept and persist batches or
return nil for valid input) instead of always returning an error, ensuring it
matches the interface used by controller.go (the controller wiring expects a
working Setter) and follows the existing DB transaction/connection patterns in
that package.
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