feat: implement summary reconciler#836
Merged
adityachoudhari26 merged 13 commits intomainfrom Mar 7, 2026
Merged
Conversation
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:
📝 WalkthroughWalkthroughAdds a new policy-summary reconciliation feature: controller, reconcilers, evaluators, DB schema + queries, Postgres getter/setter implementations, TypeScript enqueue endpoints, and tests to evaluate and persist policy rule summaries scoped by environment:version. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TRPC as tRPC
participant Queue as ReconcileQueue
participant Controller as PolicySummaryController
participant Getter as PostgresGetter
participant Evaluator as RuleEvaluators
participant Setter as PostgresSetter
participant DB as Database
Client->>TRPC: triggerPolicySummary(workspaceId, envId, versionId)
TRPC->>Queue: enqueuePolicySummary(workspaceId, envId:versionId)
Queue->>DB: INSERT reconcile work item
Note over Controller: Polling/worker loop
Controller->>Queue: Dequeue policy-summary item
Controller->>Controller: ParseScope(envId:versionId)
Controller->>Getter: GetVersion / GetPoliciesForEnvironment
Getter->>DB: SELECT deployment_version / policies
DB-->>Getter: rows
Getter-->>Controller: DeploymentVersion, Policies
loop for each policy.rule
Controller->>Evaluator: RuleEvaluators(getter, rule)
Evaluator-->>Controller: []Evaluator
Controller->>Evaluator: Execute evaluator(s)
Evaluator-->>Controller: RuleEvaluation results + nextEvaluationAt
Controller->>Controller: Accumulate RuleSummaryRow(s)
end
Controller->>Setter: UpsertRuleSummaries(rows)
Setter->>DB: UPSERT policy_rule_summary (batch)
DB-->>Setter: OK
Setter-->>Controller: nil
Controller->>Queue: Mark item complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
apps/workspace-engine/svc/controllers/policysummary/reconcile.go
Outdated
Show resolved
Hide resolved
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
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/deploymentresourceselectoreval/controller.go (1)
88-94:⚠️ Potential issue | 🟠 MajorDon't derive summary invalidation from only the post-update target set.
releaseTargetsis loaded afterSetComputedDeploymentResources, so this path only sees the new state. If the selector removes the last target from an environment—or all targets entirely—those environments never reachenqueuePolicySummaries, and the oldpolicy_rule_summaryrows stay stale. You need to include previously attached environments as well, or enqueue a deployment-scoped recompute unconditionally.Also applies to: 118-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentresourceselectoreval/controller.go` around lines 88 - 94, The current logic only calls enqueuePolicySummaries when the new releaseTargets (loaded after SetComputedDeploymentResources) is non-empty, which misses cases where targets were removed and leaves stale policy_rule_summary rows; update the controller logic around SetComputedDeploymentResources so that you either (a) compute the union of previously attached environments and the new releaseTargets and pass that to enqueuePolicySummaries (use the prior target set fetched before SetComputedDeploymentResources), or (b) always call enqueuePolicySummaries (or an unconditional deployment-scoped recompute) for the deployment.WorkspaceID regardless of releaseTargets length; reference enqueueReleaseTargets, enqueuePolicySummaries, SetComputedDeploymentResources, releaseTargets and deployment.WorkspaceID to locate where to add this behavior.
♻️ Duplicate comments (1)
apps/workspace-engine/svc/controllers/policysummary/reconcile.go (1)
159-163:⚠️ Potential issue | 🟠 MajorAvoid
uuid.MustParseon runtime input.Both
workspaceIDandrule.Idare coming from runtime data. A malformed UUID here panics the worker instead of returning a normal reconcile error.Does github.com/google/uuid.MustParse panic on invalid UUID strings, and is uuid.Parse the non-panicking alternative intended for runtime input?Also applies to: 177-180
🧹 Nitpick comments (1)
apps/workspace-engine/svc/controllers/policysummary/getters_postgres_test.go (1)
24-26: Potential race condition withos.Setenvin tests.Setting environment variables with
os.Setenvduring test execution can cause race conditions when tests run in parallel (go test -parallel). Environment variables are process-global state.Consider using
t.Setenvinstead, which is test-scoped and handles cleanup automatically:if os.Getenv("POSTGRES_URL") == "" { - os.Setenv("POSTGRES_URL", defaultDBURL) + t.Setenv("POSTGRES_URL", defaultDBURL) }🤖 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_test.go` around lines 24 - 26, The test currently calls os.Setenv to set POSTGRES_URL which is process-global and can cause race conditions; replace the os.Setenv usage with t.Setenv (test-scoped) so the environment change is isolated and automatically restored—update the test that contains the snippet to use the testing.T instance (t) and call t.Setenv("POSTGRES_URL", defaultDBURL) when os.Getenv("POSTGRES_URL") == "" (or simply always call t.Setenv to be safe), referencing the existing symbols os.Setenv, t.Setenv, POSTGRES_URL and defaultDBURL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/db/queries/policy_rule_summary.sql`:
- Around line 9-24: The insert currently writes zero-UUID placeholders into
nullable FK columns and uses a raw NULL-including conflict target (rule_id,
deployment_id, environment_id, version_id) which breaks FK checks and causes
duplicate rows for partial scopes; fix by normalizing zero UUID placeholders to
true NULLs in the VALUES (use NULLIF($2, '00000000-0000-0000-0000-000000000000')
etc. for deployment_id/environment_id/version_id) and stop using the raw
nullable column list as the ON CONFLICT target—instead use ON CONFLICT ON
CONSTRAINT <null_safe_unique_constraint> (create a unique index/constraint that
uses COALESCE(column, '00000000-0000-0000-0000-000000000000') so the uniqueness
key is NULL-safe) and keep the DO UPDATE clause as-is so upserts update the
correct row for partial scopes.
In `@apps/workspace-engine/pkg/events/handler/policies/policies.go`:
- Around line 144-145: The code currently calls
enqueuePolicySummariesForTargets(ctx, ws, affectedTargets) using affectedTargets
computed after upserting the policy so environments removed by the update are
missed; change the update path to compute both the pre-update target set and
post-update target set (e.g., oldAffectedTargets and newAffectedTargets), take
the union of those sets, and pass that union into
enqueuePolicySummariesForTargets so both previously affected and newly affected
environments are re-enqueued; update any variable names and comments in the
upsert/update logic to reflect oldAffectedTargets/newAffectedTargets and use a
deterministic set/unique list before enqueuing.
In `@apps/workspace-engine/svc/controllers/policysummary/controller.go`:
- Around line 26-29: Add Go doc comments for the exported API: write brief,
descriptive comments above the Controller type, NewController function, New
function, and the Process method describing their purpose, inputs/outputs and
behavior (e.g., "Controller orchestrates policy summary operations",
"NewController returns a Controller initialized with given Getter and Setter",
"New is a convenience constructor...","Process processes a policy summary and
returns ..."). Ensure each comment starts with the exact exported name
(Controller, NewController, New, Process) so they are picked up by godoc and
satisfy the coding guideline.
In `@apps/workspace-engine/svc/controllers/policysummary/getters_postgres.go`:
- Around line 54-67: GetPoliciesForDeployment currently ignores the deploymentID
and returns all workspace policies; update the function to filter by deployment
by calling the appropriate query (e.g., replace
g.queries.ListPoliciesByWorkspaceID with a query like
g.queries.ListPoliciesByDeploymentID or extend the params to include
DeploymentID) and pass deploymentID (uuid.UUID) in the
db.ListPoliciesByDeploymentIDParams (or add DeploymentID: pgtype.UUID{...}) so
only policies for the given deployment are returned; keep the return conversion
using db.ToOapiPolicy for each row.
- Around line 39-52: GetPoliciesForEnvironment and GetPoliciesForDeployment
declare environmentID/deploymentID but currently ignore them and call
ListPoliciesByWorkspaceID; fix by either (A) applying post-query filtering
inside GetPoliciesForEnvironment/GetPoliciesForDeployment after calling
ListPoliciesByWorkspaceID: convert rows to oapi.Policy (using db.ToOapiPolicy),
inspect the policy fields (e.g. explicit EnvironmentID/DeploymentID fields or
target selectors/annotations) and only append matching policies, or (B) if the
policy rows have no environment/deployment data, update the DB query
(ListPoliciesByWorkspaceID) to accept environment/deployment filters and
propagate those params, or (C) if filtering is not possible, rename/remove the
unused parameters and the methods to reflect workspace-scoped behavior; pick one
approach and make consistent changes to the signatures and callers for
GetPoliciesForEnvironment, GetPoliciesForDeployment and
ListPoliciesByWorkspaceID.
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go`:
- Around line 52-54: The current Upsert-only writes via
r.setter.UpsertRuleSummaries(ctx, rows) leave stale summary rows when rules or
scope matches disappear; modify the reconcile logic to remove summaries for the
affected rule+scope combinations that are NOT present in the current evaluation
batch: collect the unique (rule_id, scope_id) keys from rows, then within the
same DB transaction call a new/delete helper (e.g.
r.setter.DeleteRuleSummariesNotIn or similar) to delete any existing summaries
for those rule+scope pairs whose summary IDs are not in the current rows
before/after the upsert; implement the delete helper to accept rule+scope pairs
and the set of current summary IDs and ensure the operation is done
transactionally around UpsertRuleSummaries so no stale rows remain (apply same
change to the other Upsert calls referenced).
- Around line 16-18: Add missing Go doc comments for the exported reconcile
surface: document the ReconcileResult type and the Reconcile function/method so
they explain purpose, semantics and key fields/return values; specifically add a
comment above "type ReconcileResult struct" describing what NextReconcileAt
means and when it is set, and add a comment above the exported "Reconcile"
function/method (and any other exported items in that file around lines 177-209)
that explains what the function does, its inputs, outputs, error behavior and
any side effects. Use standard godoc style (single sentence summary, then
details) and ensure comments are complete for exported API consumers.
In `@apps/workspace-engine/svc/controllers/policysummary/setters_postgres.go`:
- Around line 15-21: Add standard Go doc comments for the exported type
PostgresSetter and its constructor NewPostgresSetter: write a one-line comment
above type PostgresSetter describing its purpose (e.g., "PostgresSetter provides
methods to set policy summaries using Postgres queries.") and a one-line comment
above NewPostgresSetter describing what it returns and the role of the queries
parameter (e.g., "NewPostgresSetter creates a PostgresSetter that uses the given
db.Queries to perform DB operations."). Ensure comments are complete sentences
and placed immediately above the declarations for PostgresSetter and
NewPostgresSetter.
In `@apps/workspace-engine/svc/controllers/policysummary/setters.go`:
- Around line 20-22: Add a package-style doc comment for the exported Setter
interface describing its purpose and behavior; place a short comment above the
Setter declaration that explains what this interface represents and what
UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error does
(e.g., that it upserts rule summary rows into the policy summary store and any
concurrency/transaction expectations).
In
`@apps/workspace-engine/svc/controllers/policysummary/summaryeval/getter_postgres.go`:
- Around line 11-39: PostgresGetter lacks the GetAllReleaseTargets method
required by the Getter interface; add a delegating method on PostgresGetter
named GetAllReleaseTargets that forwards the call to the underlying getter that
implements it (e.g. call g.versioncooldown.GetAllReleaseTargets(...) or
g.gradualRolloutGetter.GetAllReleaseTargets(...) depending on which component
owns that API) so the type assertion var _ Getter = (*PostgresGetter)(nil)
compiles.
In
`@apps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval.go`:
- Around line 12-29: Add Go doc comments for the three exported evaluator
factory functions to satisfy the documentation guideline: insert concise
package-level comments immediately above EnvironmentRuleEvaluators,
EnvironmentVersionRuleEvaluators, and DeploymentVersionRuleEvaluators that
describe what each function returns and the purpose/role of the evaluators they
construct (e.g., which policy rules they evaluate and any key parameters like
the Getter). Ensure the comment starts with the function name (e.g.,
"EnvironmentRuleEvaluators ...") and briefly notes the types returned (slice of
evaluator.Evaluator) and the intent of each factory.
In `@packages/db/drizzle/meta/_journal.json`:
- Around line 1138-1147: The generated JSON journal block ending with the entry
{ "idx": 162, "version": "7", "when": 1772850815650, "tag": "0162_same_kronos",
"breakpoints": true } is not formatted per the repo Prettier config; reformat
this JSON using Prettier with the `@ctrlplane/prettier-config` (apply to
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}) so the file matches CI expectations and
the JSON spacing/indentation matches the rest of the repo.
In `@packages/db/src/schema/policy-rule-summary.ts`:
- Around line 1-68: The file fails the repo Prettier check; run Prettier (using
the repo config `@ctrlplane/prettier-config`) against this file and commit the
formatted output so CI passes; specifically format the exported
policyRuleSummary pgTable declaration and the policyRuleSummaryRelations
relations block (and any imports) using the project's Prettier setup (e.g.,
npm/yarn script like "prettier --write" or your editor integration) so spacing,
trailing commas, and line breaks match the configured rules before merging.
---
Outside diff comments:
In
`@apps/workspace-engine/svc/controllers/deploymentresourceselectoreval/controller.go`:
- Around line 88-94: The current logic only calls enqueuePolicySummaries when
the new releaseTargets (loaded after SetComputedDeploymentResources) is
non-empty, which misses cases where targets were removed and leaves stale
policy_rule_summary rows; update the controller logic around
SetComputedDeploymentResources so that you either (a) compute the union of
previously attached environments and the new releaseTargets and pass that to
enqueuePolicySummaries (use the prior target set fetched before
SetComputedDeploymentResources), or (b) always call enqueuePolicySummaries (or
an unconditional deployment-scoped recompute) for the deployment.WorkspaceID
regardless of releaseTargets length; reference enqueueReleaseTargets,
enqueuePolicySummaries, SetComputedDeploymentResources, releaseTargets and
deployment.WorkspaceID to locate where to add this behavior.
---
Nitpick comments:
In
`@apps/workspace-engine/svc/controllers/policysummary/getters_postgres_test.go`:
- Around line 24-26: The test currently calls os.Setenv to set POSTGRES_URL
which is process-global and can cause race conditions; replace the os.Setenv
usage with t.Setenv (test-scoped) so the environment change is isolated and
automatically restored—update the test that contains the snippet to use the
testing.T instance (t) and call t.Setenv("POSTGRES_URL", defaultDBURL) when
os.Getenv("POSTGRES_URL") == "" (or simply always call t.Setenv to be safe),
referencing the existing symbols os.Setenv, t.Setenv, POSTGRES_URL and
defaultDBURL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8eb53299-6700-4564-9468-e355d3b9a3cc
📒 Files selected for processing (28)
apps/workspace-engine/main.goapps/workspace-engine/pkg/db/batch.goapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/policy_rule_summary.sql.goapps/workspace-engine/pkg/db/queries/policy_rule_summary.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/sqlc.yamlapps/workspace-engine/pkg/events/handler/deploymentversion/deploymentversion.goapps/workspace-engine/pkg/events/handler/environment/environment.goapps/workspace-engine/pkg/events/handler/jobs/jobs.goapps/workspace-engine/pkg/events/handler/policies/policies.goapps/workspace-engine/pkg/events/handler/userapprovalrecords/userapprovalrecords.goapps/workspace-engine/svc/controllers/deploymentresourceselectoreval/controller.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/getters_postgres_test.goapps/workspace-engine/svc/controllers/policysummary/reconcile.goapps/workspace-engine/svc/controllers/policysummary/setters.goapps/workspace-engine/svc/controllers/policysummary/setters_postgres.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/getter.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/getter_postgres.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval_test.gopackages/db/drizzle/0162_same_kronos.sqlpackages/db/drizzle/meta/0162_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/policy-rule-summary.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspace-engine/svc/controllers/policysummary/getters.go
Comment on lines
+9
to
+24
| VALUES ( | ||
| gen_random_uuid(), $1, | ||
| $2, $3, $4, | ||
| $5, $6, $7, | ||
| $8, $9, | ||
| $10, $11, NOW() | ||
| ) | ||
| ON CONFLICT (rule_id, deployment_id, environment_id, version_id) DO UPDATE | ||
| SET allowed = EXCLUDED.allowed, | ||
| action_required = EXCLUDED.action_required, | ||
| action_type = EXCLUDED.action_type, | ||
| message = EXCLUDED.message, | ||
| details = EXCLUDED.details, | ||
| satisfied_at = EXCLUDED.satisfied_at, | ||
| next_evaluation_at = EXCLUDED.next_evaluation_at, | ||
| evaluated_at = NOW(); |
Contributor
There was a problem hiding this comment.
This scope key cannot upsert correctly for partial scopes.
The writer currently substitutes missing deployment_id / environment_id / version_id values with the zero UUID, and Lines 9-15 insert them verbatim into nullable FK columns. That makes environment-only, environment-version, and deployment-version writes fail FK checks immediately. If you normalize those placeholders to real NULLs, the conflict target on Lines 16-24 still is not a stable upsert key for nullable scope columns, so re-reconciles will insert duplicates instead of updating. This needs a NULL-safe key end-to-end here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/db/queries/policy_rule_summary.sql` around lines 9
- 24, The insert currently writes zero-UUID placeholders into nullable FK
columns and uses a raw NULL-including conflict target (rule_id, deployment_id,
environment_id, version_id) which breaks FK checks and causes duplicate rows for
partial scopes; fix by normalizing zero UUID placeholders to true NULLs in the
VALUES (use NULLIF($2, '00000000-0000-0000-0000-000000000000') etc. for
deployment_id/environment_id/version_id) and stop using the raw nullable column
list as the ON CONFLICT target—instead use ON CONFLICT ON CONSTRAINT
<null_safe_unique_constraint> (create a unique index/constraint that uses
COALESCE(column, '00000000-0000-0000-0000-000000000000') so the uniqueness key
is NULL-safe) and keep the DO UPDATE clause as-is so upserts update the correct
row for partial scopes.
Comment on lines
+144
to
+145
| enqueuePolicySummariesForTargets(ctx, ws, affectedTargets) | ||
|
|
Contributor
There was a problem hiding this comment.
Include previously affected environments when re-enqueueing summaries on policy updates.
Here affectedTargets was computed after the policy was upserted, so it only contains targets still matched by the new definition. If the update removes the policy from an environment, that environment never gets re-enqueued and its stored summaries remain stale. Build the enqueue set from the union of old and new affected targets for the update path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/events/handler/policies/policies.go` around lines
144 - 145, The code currently calls enqueuePolicySummariesForTargets(ctx, ws,
affectedTargets) using affectedTargets computed after upserting the policy so
environments removed by the update are missed; change the update path to compute
both the pre-update target set and post-update target set (e.g.,
oldAffectedTargets and newAffectedTargets), take the union of those sets, and
pass that union into enqueuePolicySummariesForTargets so both previously
affected and newly affected environments are re-enqueued; update any variable
names and comments in the upsert/update logic to reflect
oldAffectedTargets/newAffectedTargets and use a deterministic set/unique list
before enqueuing.
Comment on lines
+26
to
+29
| type Controller struct { | ||
| getter Getter | ||
| setter Setter | ||
| } |
Contributor
There was a problem hiding this comment.
Document the exported controller API.
Controller, Process, NewController, and New are all exported without doc comments.
As per coding guidelines "document exported functions/types/methods".
Also applies to: 31-57, 59-97
🤖 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 26 - 29, Add Go doc comments for the exported API: write brief,
descriptive comments above the Controller type, NewController function, New
function, and the Process method describing their purpose, inputs/outputs and
behavior (e.g., "Controller orchestrates policy summary operations",
"NewController returns a Controller initialized with given Getter and Setter",
"New is a convenience constructor...","Process processes a policy summary and
returns ..."). Ensure each comment starts with the exact exported name
(Controller, NewController, New, Process) so they are picked up by godoc and
satisfy the coding guideline.
Comment on lines
+39
to
+52
| func (g *PostgresGetter) GetPoliciesForEnvironment(ctx context.Context, workspaceID, environmentID uuid.UUID) ([]*oapi.Policy, error) { | ||
| rows, err := g.queries.ListPoliciesByWorkspaceID(ctx, db.ListPoliciesByWorkspaceIDParams{ | ||
| WorkspaceID: workspaceID, | ||
| Limit: pgtype.Int4{Int32: 5000, Valid: true}, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list policies for workspace %s: %w", workspaceID, err) | ||
| } | ||
| policies := make([]*oapi.Policy, 0, len(rows)) | ||
| for _, row := range rows { | ||
| policies = append(policies, db.ToOapiPolicy(row)) | ||
| } | ||
| return policies, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a query that actually filters by environment_id
rg -n "environment_id" apps/workspace-engine/pkg/db/queries/policies.sql
# Check if there are any other implementations that filter by environment
ast-grep --pattern 'GetPoliciesForEnvironment($$$) {
$$$
}'Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Find SQL query files related to policies
fd -t f "*.sql" apps/workspace-engine/ | grep -i polic
# Check the actual file structure
find apps/workspace-engine -type f -name "*.go" | grep -i getter | head -5Repository: ctrlplanedev/ctrlplane
Length of output: 849
🏁 Script executed:
# Look for ListPoliciesByWorkspaceID definition
rg -n "ListPoliciesByWorkspaceID" apps/workspace-engine/
# Check the getters_postgres.go file itself to understand the context
head -100 apps/workspace-engine/svc/controllers/policysummary/getters_postgres.goRepository: ctrlplanedev/ctrlplane
Length of output: 3466
🏁 Script executed:
# Search for GetPoliciesForEnvironment interface definition
rg -n "GetPoliciesForEnvironment" apps/workspace-engine/ -B 2 -A 2Repository: ctrlplanedev/ctrlplane
Length of output: 4090
🏁 Script executed:
# Look at the SQL query definition
cat apps/workspace-engine/pkg/db/queries/policies.sql | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 961
🏁 Script executed:
# Check the test setup to see what data is being created
head -200 apps/workspace-engine/svc/controllers/policysummary/getters_postgres_test.go | tail -100Repository: ctrlplanedev/ctrlplane
Length of output: 2989
🏁 Script executed:
# Look at the full test to understand what it expects
sed -n '181,210p' apps/workspace-engine/svc/controllers/policysummary/getters_postgres_test.goRepository: ctrlplanedev/ctrlplane
Length of output: 1064
🏁 Script executed:
# Check if there's any post-filtering logic elsewhere
rg -n "environmentID\|environment_id" apps/workspace-engine/svc/controllers/policysummary/Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check if there's supposed to be environment filtering in the policy table schema
rg -n "CREATE TABLE policy" apps/workspace-engine/pkg/db/migrations/ -A 15
# Check if there are any other query methods that might support environment filtering
rg -n "SELECT.*policy" apps/workspace-engine/pkg/db/queries/policies.sqlRepository: ctrlplanedev/ctrlplane
Length of output: 822
🏁 Script executed:
# Check what other similar getters do - look for pattern with similar parameters
grep -r "GetPoliciesFor" apps/workspace-engine/svc/controllers/ -A 5 | head -40Repository: ctrlplanedev/ctrlplane
Length of output: 4105
environmentID and deploymentID parameters are declared but never used.
Both GetPoliciesForEnvironment and GetPoliciesForDeployment accept these parameters but ignore them completely. Both methods call ListPoliciesByWorkspaceID, returning all policies for the workspace regardless of the environment or deployment passed in. This violates the method contract—callers expect filtered results but receive workspace-scoped results instead.
The underlying query ListPoliciesByWorkspaceID only supports workspace-level filtering; the policy table has no environment_id or deployment_id column. Either implement the filtering (post-query via selector logic or schema change), rename the methods to accurately reflect workspace-scoped results, or remove the unused parameters.
🤖 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 39 - 52, GetPoliciesForEnvironment and GetPoliciesForDeployment
declare environmentID/deploymentID but currently ignore them and call
ListPoliciesByWorkspaceID; fix by either (A) applying post-query filtering
inside GetPoliciesForEnvironment/GetPoliciesForDeployment after calling
ListPoliciesByWorkspaceID: convert rows to oapi.Policy (using db.ToOapiPolicy),
inspect the policy fields (e.g. explicit EnvironmentID/DeploymentID fields or
target selectors/annotations) and only append matching policies, or (B) if the
policy rows have no environment/deployment data, update the DB query
(ListPoliciesByWorkspaceID) to accept environment/deployment filters and
propagate those params, or (C) if filtering is not possible, rename/remove the
unused parameters and the methods to reflect workspace-scoped behavior; pick one
approach and make consistent changes to the signatures and callers for
GetPoliciesForEnvironment, GetPoliciesForDeployment and
ListPoliciesByWorkspaceID.
Comment on lines
+54
to
+67
| func (g *PostgresGetter) GetPoliciesForDeployment(ctx context.Context, workspaceID, deploymentID uuid.UUID) ([]*oapi.Policy, error) { | ||
| rows, err := g.queries.ListPoliciesByWorkspaceID(ctx, db.ListPoliciesByWorkspaceIDParams{ | ||
| WorkspaceID: workspaceID, | ||
| Limit: pgtype.Int4{Int32: 5000, Valid: true}, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list policies for workspace %s: %w", workspaceID, err) | ||
| } | ||
| policies := make([]*oapi.Policy, 0, len(rows)) | ||
| for _, row := range rows { | ||
| policies = append(policies, db.ToOapiPolicy(row)) | ||
| } | ||
| return policies, nil | ||
| } |
Contributor
There was a problem hiding this comment.
deploymentID parameter is declared but never used.
Same issue as GetPoliciesForEnvironment above. The deploymentID parameter is accepted but ignored—the method returns all workspace policies without filtering by deployment. This implementation is also identical to GetPoliciesForEnvironment, suggesting both need proper filtering logic.
🤖 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 54 - 67, GetPoliciesForDeployment currently ignores the
deploymentID and returns all workspace policies; update the function to filter
by deployment by calling the appropriate query (e.g., replace
g.queries.ListPoliciesByWorkspaceID with a query like
g.queries.ListPoliciesByDeploymentID or extend the params to include
DeploymentID) and pass deploymentID (uuid.UUID) in the
db.ListPoliciesByDeploymentIDParams (or add DeploymentID: pgtype.UUID{...}) so
only policies for the given deployment are returned; keep the return conversion
using db.ToOapiPolicy for each row.
Comment on lines
+20
to
+22
| type Setter interface { | ||
| UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error | ||
| } |
Contributor
There was a problem hiding this comment.
Add a doc comment for Setter.
Setter is exported, so this file is adding public API without the package-style comment required for the new controller surface.
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/svc/controllers/policysummary/setters.go` around lines
20 - 22, Add a package-style doc comment for the exported Setter interface
describing its purpose and behavior; place a short comment above the Setter
declaration that explains what this interface represents and what
UpsertRuleSummaries(ctx context.Context, rows []RuleSummaryRow) error does
(e.g., that it upserts rule summary rows into the policy summary store and any
concurrency/transaction expectations).
apps/workspace-engine/svc/controllers/policysummary/summaryeval/getter_postgres.go
Show resolved
Hide resolved
Comment on lines
+12
to
+29
| func EnvironmentRuleEvaluators(rule *oapi.PolicyRule) []evaluator.Evaluator { | ||
| return evaluator.CollectEvaluators( | ||
| deploymentwindow.NewSummaryEvaluator(rule), | ||
| ) | ||
| } | ||
|
|
||
| func EnvironmentVersionRuleEvaluators(getter Getter, rule *oapi.PolicyRule) []evaluator.Evaluator { | ||
| return evaluator.CollectEvaluators( | ||
| approval.NewEvaluator(getter, rule), | ||
| environmentprogression.NewEvaluator(getter, rule), | ||
| ) | ||
| } | ||
|
|
||
| func DeploymentVersionRuleEvaluators(getter Getter, rule *oapi.PolicyRule) []evaluator.Evaluator { | ||
| return evaluator.CollectEvaluators( | ||
| versioncooldown.NewSummaryEvaluator(getter, rule), | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Document the exported evaluator factories.
EnvironmentRuleEvaluators, EnvironmentVersionRuleEvaluators, and DeploymentVersionRuleEvaluators are all exported without doc comments.
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/svc/controllers/policysummary/summaryeval/summaryeval.go`
around lines 12 - 29, Add Go doc comments for the three exported evaluator
factory functions to satisfy the documentation guideline: insert concise
package-level comments immediately above EnvironmentRuleEvaluators,
EnvironmentVersionRuleEvaluators, and DeploymentVersionRuleEvaluators that
describe what each function returns and the purpose/role of the evaluators they
construct (e.g., which policy rules they evaluate and any key parameters like
the Getter). Ensure the comment starts with the function name (e.g.,
"EnvironmentRuleEvaluators ...") and briefly notes the types returned (slice of
evaluator.Evaluator) and the intent of each factory.
Comment on lines
+1138
to
+1147
| }, | ||
| { | ||
| "idx": 162, | ||
| "version": "7", | ||
| "when": 1772850815650, | ||
| "tag": "0162_same_kronos", | ||
| "breakpoints": true | ||
| } | ||
| ] | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
Run Prettier on this generated journal file.
CI is already failing on this change because the JSON formatting here does not match the repo's Prettier config.
As per coding guidelines, **/*.{js,jsx,ts,tsx,json,md,yml,yaml}: Formatting: Prettier is used with @ctrlplane/prettier-config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/meta/_journal.json` around lines 1138 - 1147, The
generated JSON journal block ending with the entry { "idx": 162, "version": "7",
"when": 1772850815650, "tag": "0162_same_kronos", "breakpoints": true } is not
formatted per the repo Prettier config; reformat this JSON using Prettier with
the `@ctrlplane/prettier-config` (apply to **/*.{js,jsx,ts,tsx,json,md,yml,yaml})
so the file matches CI expectations and the JSON spacing/indentation matches the
rest of the repo.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/workspace-engine/svc/controllers/policysummary/reconcile.go (1)
92-93:⚠️ Potential issue | 🟠 MajorAvoid
MustParseon externally-sourced UUIDs.
workspaceIDoriginates from the work queue andrule.Id(line 64) comes from database results. While these should normally be valid, malformed UUIDs will panic the worker instead of returning a recoverable error.Safer parse flow
func Reconcile(ctx context.Context, workspaceID string, scopeID string, getter Getter, setter Setter) (*ReconcileResult, error) { scope, err := ParseScope(scopeID) if err != nil { return nil, err } + 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, }Also applies to line 64 for
rule.Id.🤖 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 92 - 93, Replace uuid.MustParse usage with safe parsing and error handling: instead of uuid.MustParse(workspaceID) in the reconciler construction and for rule.Id, call uuid.Parse (or uuid.ParseStrict) and check the returned error, returning or bubbling a descriptive error from the caller (e.g., the function that constructs the reconciler or the reconcile entrypoint) rather than panicking; update the code paths that build the reconciler (symbol: reconciler and its constructor/initialization site where workspaceID is set) and wherever rule.Id is parsed to validate and handle malformed UUIDs gracefully, logging context (workspaceID or rule.Id) on parse failure and stopping or re-queuing the work item appropriately.
🧹 Nitpick comments (3)
apps/workspace-engine/svc/controllers/policysummary/setters_postgres.go (1)
65-72: Consider collecting all batch errors instead of only the first.The current error handling captures only the first error and silently ignores subsequent failures. While this may be acceptable for debugging, it could mask multiple independent failures in a batch.
♻️ Alternative: collect all errors
results := s.queries.UpsertPolicyRuleSummary(ctx, params) - var batchErr error + var errs []error results.Exec(func(i int, err error) { - if err != nil && batchErr == nil { - batchErr = fmt.Errorf("upsert rule summary %d: %w", i, err) + if err != nil { + errs = append(errs, fmt.Errorf("upsert rule summary %d: %w", i, err)) } }) - return batchErr + if len(errs) > 0 { + return errors.Join(errs...) + } + return nilThis would require adding
"errors"to the imports.🤖 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 65 - 72, The batch handler for UpsertPolicyRuleSummary currently stores only the first error via batchErr inside results.Exec; change this to collect all errors by building an errors.Join-able slice: import "errors", append each non-nil error inside the Exec callback (e.g., collect into a []error named errs), and after Exec return errors.Join(errs) (or nil if empty) instead of returning only the first error; keep the existing fmt-wrapping for each item (e.g., fmt.Errorf("upsert rule summary %d: %w", i, err)) when appending to the slice.apps/workspace-engine/svc/controllers/policysummary/scope.go (1)
10-28: Add doc comments for exportedScopetype andParseScopefunction.The exported
Scopestruct andParseScopefunction are missing documentation comments. As per coding guidelines, exported types and functions should be documented.📝 Proposed doc comments
+// Scope identifies a policy summary evaluation target by environment and version. type Scope struct { EnvironmentID uuid.UUID VersionID uuid.UUID } +// ParseScope parses a colon-separated scope ID string into its component UUIDs. +// Expected format: "environmentID:versionID" func ParseScope(scopeID string) (*Scope, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/policysummary/scope.go` around lines 10 - 28, Add Go doc comments for the exported Scope type and ParseScope function: prepend a comment for Scope that describes its purpose (represents the policy summary scope holding EnvironmentID and VersionID) and a brief description of each exported field; prepend a comment for ParseScope that explains it parses a scopeID of the form "envUUID:versionUUID", describes the input parameter and returned *Scope and error, and notes the error cases (invalid format or UUID parse failures). Use standard Go doc style (start with "Scope ..." and "ParseScope ...") so godoc picks them up and keep the comments concise.apps/workspace-engine/pkg/reconcile/events/policysummary.go (1)
27-28: Consider extractingScopeTypeto a constant.The string
"environment-version"is duplicated in both enqueue functions. Extracting it to a package-level constant (e.g.,PolicySummaryScopeType) would reduce duplication and make it easier to keep in sync with any consumer expectations.const PolicySummaryKind = "policy-summary" +const PolicySummaryScopeType = "environment-version" ... func EnqueuePolicySummary(...) error { return queue.Enqueue(ctx, reconcile.EnqueueParams{ ... - ScopeType: "environment-version", + ScopeType: PolicySummaryScopeType, ... }) }Also applies to: 42-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/reconcile/events/policysummary.go` around lines 27 - 28, The literal "environment-version" is duplicated for ScopeType in the enqueue functions; extract it to a package-level constant (e.g., PolicySummaryScopeType) and replace the string literal usages in both enqueue functions so ScopeType: PolicySummaryScopeType is used instead of the hard-coded string; update any comments/tests that reference the literal if present and keep the constant name exported if other packages consume it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/db/drizzle/0162_bizarre_jackal.sql`:
- Around line 1-19: The policy_rule_summary.rule_id has no FK and the cleanup
function DeletePolicyRuleSummariesByRuleID is defined but never called; update
the deletion flow in deleteAllRulesWithQueries to invoke
DeletePolicyRuleSummariesByRuleID for each rule ID being removed (or
alternatively add a foreign key constraint on policy_rule_summary.rule_id
pointing to the policy rule table) so summary rows are removed when rules are
deleted; locate deleteAllRulesWithQueries and call
DeletePolicyRuleSummariesByRuleID for each rule ID prior to or as part of the
rule deletion transaction to avoid orphaned policy_rule_summary rows.
---
Duplicate comments:
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go`:
- Around line 92-93: Replace uuid.MustParse usage with safe parsing and error
handling: instead of uuid.MustParse(workspaceID) in the reconciler construction
and for rule.Id, call uuid.Parse (or uuid.ParseStrict) and check the returned
error, returning or bubbling a descriptive error from the caller (e.g., the
function that constructs the reconciler or the reconcile entrypoint) rather than
panicking; update the code paths that build the reconciler (symbol: reconciler
and its constructor/initialization site where workspaceID is set) and wherever
rule.Id is parsed to validate and handle malformed UUIDs gracefully, logging
context (workspaceID or rule.Id) on parse failure and stopping or re-queuing the
work item appropriately.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/reconcile/events/policysummary.go`:
- Around line 27-28: The literal "environment-version" is duplicated for
ScopeType in the enqueue functions; extract it to a package-level constant
(e.g., PolicySummaryScopeType) and replace the string literal usages in both
enqueue functions so ScopeType: PolicySummaryScopeType is used instead of the
hard-coded string; update any comments/tests that reference the literal if
present and keep the constant name exported if other packages consume it.
In `@apps/workspace-engine/svc/controllers/policysummary/scope.go`:
- Around line 10-28: Add Go doc comments for the exported Scope type and
ParseScope function: prepend a comment for Scope that describes its purpose
(represents the policy summary scope holding EnvironmentID and VersionID) and a
brief description of each exported field; prepend a comment for ParseScope that
explains it parses a scopeID of the form "envUUID:versionUUID", describes the
input parameter and returned *Scope and error, and notes the error cases
(invalid format or UUID parse failures). Use standard Go doc style (start with
"Scope ..." and "ParseScope ...") so godoc picks them up and keep the comments
concise.
In `@apps/workspace-engine/svc/controllers/policysummary/setters_postgres.go`:
- Around line 65-72: The batch handler for UpsertPolicyRuleSummary currently
stores only the first error via batchErr inside results.Exec; change this to
collect all errors by building an errors.Join-able slice: import "errors",
append each non-nil error inside the Exec callback (e.g., collect into a []error
named errs), and after Exec return errors.Join(errs) (or nil if empty) instead
of returning only the first error; keep the existing fmt-wrapping for each item
(e.g., fmt.Errorf("upsert rule summary %d: %w", i, err)) when appending to the
slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8daf0e8c-f70b-475f-bb6a-868e03713d7b
📒 Files selected for processing (21)
apps/workspace-engine/pkg/db/batch.goapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/policy_rule_summary.sql.goapps/workspace-engine/pkg/db/queries/policy_rule_summary.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/events/handler/jobs/jobs.goapps/workspace-engine/pkg/events/handler/policies/policies.goapps/workspace-engine/pkg/reconcile/events/policysummary.goapps/workspace-engine/svc/controllers/policysummary/controller.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.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval_test.gopackages/db/drizzle/0162_bizarre_jackal.sqlpackages/db/drizzle/meta/0162_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/reconcilers/index.tspackages/db/src/schema/policy-rule-summary.tspackages/trpc/src/routes/reconcile.ts
💤 Files with no reviewable changes (1)
- apps/workspace-engine/pkg/events/handler/jobs/jobs.go
✅ Files skipped from review due to trivial changes (1)
- apps/workspace-engine/pkg/events/handler/policies/policies.go
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval_test.go
- apps/workspace-engine/pkg/db/models.go
- packages/db/drizzle/meta/_journal.json
- packages/db/src/schema/policy-rule-summary.ts
- apps/workspace-engine/pkg/db/batch.go
- apps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval.go
- apps/workspace-engine/pkg/db/queries/schema.sql
Comment on lines
+1
to
+19
| CREATE TABLE "policy_rule_summary" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | ||
| "rule_id" uuid NOT NULL, | ||
| "environment_id" uuid NOT NULL, | ||
| "version_id" uuid NOT NULL, | ||
| "allowed" boolean NOT NULL, | ||
| "action_required" boolean DEFAULT false NOT NULL, | ||
| "action_type" text, | ||
| "message" text NOT NULL, | ||
| "details" jsonb DEFAULT '{}' NOT NULL, | ||
| "satisfied_at" timestamp with time zone, | ||
| "next_evaluation_at" timestamp with time zone, | ||
| "evaluated_at" timestamp with time zone DEFAULT now() NOT NULL | ||
| ); | ||
| --> statement-breakpoint | ||
| ALTER TABLE "policy_rule_summary" ADD CONSTRAINT "policy_rule_summary_environment_id_environment_id_fk" FOREIGN KEY ("environment_id") REFERENCES "public"."environment"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "policy_rule_summary" ADD CONSTRAINT "policy_rule_summary_version_id_deployment_version_id_fk" FOREIGN KEY ("version_id") REFERENCES "public"."deployment_version"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| CREATE UNIQUE INDEX "policy_rule_summary_rule_id_environment_id_version_id_index" ON "policy_rule_summary" USING btree ("rule_id","environment_id","version_id");--> statement-breakpoint | ||
| CREATE INDEX "policy_rule_summary_environment_id_version_id_index" ON "policy_rule_summary" USING btree ("environment_id","version_id"); No newline at end of file |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that all policy rule deletion paths include cleanup of policy_rule_summary
# Find policy rule table definitions to understand the rule types
rg -n "policy_rule" --type sql -g '*.sql' | head -30
# Search for DELETE operations on policy rule tables to verify cleanup is called
rg -nP "DELETE\s+FROM\s+policy_rule" --type sqlRepository: ctrlplanedev/ctrlplane
Length of output: 5747
🏁 Script executed:
#!/bin/bash
# Find the main policy_rule table definition
rg -n "CREATE TABLE.*policy_rule" --type sql -A 10 | grep -v "policy_rule_" | head -20
# Find references to rule_id in queries
rg -n "rule_id" --type sql | head -20
# Check the schema definitions
fd -e ts -e tsx -e js | xargs rg -l "policy_rule_summary" | head -5Repository: ctrlplanedev/ctrlplane
Length of output: 6073
🏁 Script executed:
#!/bin/bash
# Read the schema definition for policy_rule_summary
cat packages/db/src/schema/policy-rule-summary.tsRepository: ctrlplanedev/ctrlplane
Length of output: 1746
🏁 Script executed:
#!/bin/bash
# Find where policy_rule_summary is inserted/created
rg -n "policy_rule_summary|policyRuleSummary" --type ts --type tsx --type js | grep -E "(insert|create|INSERT)" | head -20
# Find the query file
cat packages/db/drizzle/queries/policy_rule_summary.sqlRepository: ctrlplanedev/ctrlplane
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Find the query file
find . -name "*policy_rule_summary*"
# Search for inserts of policy_rule_summary in code
rg -n "policy_rule_summary|policyRuleSummary" --type-list | head -5
rg -n "INSERT INTO policy_rule_summary|policyRuleSummary" -A 5 | head -40Repository: ctrlplanedev/ctrlplane
Length of output: 3084
🏁 Script executed:
#!/bin/bash
# Read the query file
cat apps/workspace-engine/pkg/db/queries/policy_rule_summary.sql
# Also check batch.go for context
sed -n '210,230p' apps/workspace-engine/pkg/db/batch.goRepository: ctrlplanedev/ctrlplane
Length of output: 1774
🏁 Script executed:
#!/bin/bash
# Find where DeletePolicyRuleSummariesByRuleID is called
rg -n "DeletePolicyRuleSummariesByRuleID" --type go
# Also search for where policy rules are deleted
rg -n "DELETE.*rule" apps/workspace-engine/pkg/db/ --type go | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 1672
🏁 Script executed:
#!/bin/bash
# Search for actual calls to DeletePolicyRuleSummariesByRuleID (not definitions)
rg -n "\.DeletePolicyRuleSummariesByRuleID|DeletePolicyRuleSummariesByRuleID\("
# Find all files in workspace-engine that interact with policy rules
find apps/workspace-engine -name "*.go" | xargs grep -l "rule_id\|ruleId" | head -10
# Search in workspace-engine for where rules are deleted
rg -n "rule.*delete|delete.*rule" apps/workspace-engine --type go -i | head -30Repository: ctrlplanedev/ctrlplane
Length of output: 4778
🏁 Script executed:
#!/bin/bash
# Search more explicitly for calls to the delete function
rg "DeletePolicyRuleSummariesByRuleID\(" --type go -B 2 -A 2
# Check batch.go to understand the context
head -50 apps/workspace-engine/pkg/db/batch.goRepository: ctrlplanedev/ctrlplane
Length of output: 1879
🏁 Script executed:
#!/bin/bash
# Search entire codebase for the function call
rg "DeletePolicyRuleSummariesByRuleID" -t go --no-heading
# Search for where policies might delete rule summaries
rg "policyRuleSummary|policy_rule_summary" apps/ --type go -A 3 -B 3 | head -60Repository: ctrlplanedev/ctrlplane
Length of output: 2591
🏁 Script executed:
#!/bin/bash
# Search for the actual method call with receiver pattern
rg "\.DeletePolicyRuleSummariesByRuleID" --type go
# Search in handler code for policy rule deletion
fd -e go | xargs grep -l "handler.*Delete\|DeletePolicy\|delete.*policy" | head -10
# Check if there are any references in TypeScript/JavaScript
rg "DeletePolicyRuleSummariesByRuleID" --type ts --type jsRepository: ctrlplanedev/ctrlplane
Length of output: 669
🏁 Script executed:
#!/bin/bash
# Check the policies repository for delete logic
cat apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go | head -100
# Search for any policy rule deletion in the codebase
rg -n "DELETE.*policy_rule\|DeletePolicy" apps/workspace-engine/pkg/workspace --type go -A 5 | head -60Repository: ctrlplanedev/ctrlplane
Length of output: 2770
🏁 Script executed:
#!/bin/bash
# Find the deleteAllRulesWithQueries function
rg -n "deleteAllRulesWithQueries" apps/workspace-engine/pkg/workspace/store/repository/db/policies/ -A 30Repository: ctrlplanedev/ctrlplane
Length of output: 6716
🏁 Script executed:
#!/bin/bash
# Verify what DeletePolicy does
cat apps/workspace-engine/pkg/db/policies.sql.go | grep -A 10 "func.*DeletePolicy"
# Check if there are any cascade constraints on policy_rule_summary in the migrations
rg -n "policy_rule_summary" packages/db/drizzle/ -B 2 -A 2Repository: ctrlplanedev/ctrlplane
Length of output: 4610
Add missing cleanup call when policy rules are deleted.
The rule_id column lacks a foreign key constraint, relying on application-level cleanup via DeletePolicyRuleSummariesByRuleID. However, this cleanup function is defined but never invoked. When policy rules are deleted (in deleteAllRulesWithQueries), orphaned policy_rule_summary rows will accumulate.
Call DeletePolicyRuleSummariesByRuleID for each rule ID being deleted to ensure summary records are cleaned up consistently. Alternatively, add a foreign key constraint on rule_id if it references a specific policy rule table.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/0162_bizarre_jackal.sql` around lines 1 - 19, The
policy_rule_summary.rule_id has no FK and the cleanup function
DeletePolicyRuleSummariesByRuleID is defined but never called; update the
deletion flow in deleteAllRulesWithQueries to invoke
DeletePolicyRuleSummariesByRuleID for each rule ID being removed (or
alternatively add a foreign key constraint on policy_rule_summary.rule_id
pointing to the policy rule table) so summary rows are removed when rules are
deleted; locate deleteAllRulesWithQueries and call
DeletePolicyRuleSummariesByRuleID for each rule ID prior to or as part of the
rule deletion transaction to avoid orphaned policy_rule_summary rows.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/workspace-engine/svc/controllers/policysummary/reconcile.go (2)
92-93:⚠️ Potential issue | 🟠 MajorUse
uuid.Parseinstead ofuuid.MustParsefor workspaceID.
workspaceIDis an external input parameter. A malformed UUID will panic the worker. Useuuid.Parsewith proper error propagation.Suggested change
+ 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, }🤖 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 92 - 93, Replace the panic-prone uuid.MustParse call with uuid.Parse for the workspaceID assignment in the reconciler construction so malformed external input doesn't crash the worker; update the surrounding code that constructs the reconciler (the reconciler literal where workspaceID is set) to call uuid.Parse, handle the returned (uuid, error) by propagating or returning the error from the caller (or converting to a reconciler creation error) instead of allowing a panic, and ensure any upstream function (the reconciler factory or calling function) checks and handles that error path appropriately.
64-64:⚠️ Potential issue | 🟡 MinorUse
uuid.Parseinstead ofuuid.MustParsefor rule IDs.While
rule.Idcomes from the database and should be valid, usingMustParsewill panic on malformed data. Preferuuid.Parsewith error handling for defense in depth.Suggested change
- rows = append(rows, RuleSummaryRow{ - RuleID: uuid.MustParse(rule.Id), + ruleID, err := uuid.Parse(rule.Id) + if err != nil { + continue // or log and skip + } + rows = append(rows, RuleSummaryRow{ + RuleID: ruleID,🤖 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 64, Replace the panic-prone uuid.MustParse usage for RuleID with uuid.Parse and add error handling: in the reconcile logic where RuleID is set from rule.Id (the line with RuleID: uuid.MustParse(rule.Id)), call uuid.Parse(rule.Id), check the error, and handle it appropriately (e.g., log the parse error and return/requeue from the reconcile function or skip the invalid rule) so malformed DB values do not crash the controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/workspace-engine/svc/controllers/policysummary/reconcile.go`:
- Around line 92-93: Replace the panic-prone uuid.MustParse call with uuid.Parse
for the workspaceID assignment in the reconciler construction so malformed
external input doesn't crash the worker; update the surrounding code that
constructs the reconciler (the reconciler literal where workspaceID is set) to
call uuid.Parse, handle the returned (uuid, error) by propagating or returning
the error from the caller (or converting to a reconciler creation error) instead
of allowing a panic, and ensure any upstream function (the reconciler factory or
calling function) checks and handles that error path appropriately.
- Line 64: Replace the panic-prone uuid.MustParse usage for RuleID with
uuid.Parse and add error handling: in the reconcile logic where RuleID is set
from rule.Id (the line with RuleID: uuid.MustParse(rule.Id)), call
uuid.Parse(rule.Id), check the error, and handle it appropriately (e.g., log the
parse error and return/requeue from the reconcile function or skip the invalid
rule) so malformed DB values do not crash the controller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 485f21fd-d8aa-4d0d-8cba-db6c0655cae5
📒 Files selected for processing (4)
apps/workspace-engine/svc/controllers/policysummary/reconcile.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/getter_postgres.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval.goapps/workspace-engine/svc/controllers/policysummary/summaryeval/summaryeval_test.go
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