feat: opa plan validations in workspace engine#1094
Merged
adityachoudhari26 merged 2 commits intomainfrom May 1, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces OPA-based plan validation to the deployment plan result workflow. It adds database tables to store validation rules and results, API schemas for rule/result types, database queries for data access, and controller logic to evaluate rules and persist outcomes. Changes
Sequence DiagramsequenceDiagram
actor Controller
participant Getter
participant DB
participant OPA as OPA Engine
participant Setter
Controller->>Getter: GetMatchingPlanValidationOpaRules(workspaceID, target)
Getter->>DB: Query policy_rule_plan_validation_opa rules
DB-->>Getter: Return rule rows
Getter->>Getter: Match selectors against target
Getter-->>Controller: Return matching PolicyRules
Controller->>Getter: GetCurrentVersionForPlanTarget(planTargetID)
Getter->>DB: Query current deployment version
DB-->>Getter: Return DeploymentVersion
Getter-->>Controller: Return version
Controller->>Controller: Construct OPA input (plan, version, context)
loop For each matching rule
Controller->>OPA: Evaluate rule.Rego with input
OPA-->>Controller: Return violations (if any)
end
Controller->>Setter: UpsertPlanValidationResult(resultID, ruleID, passed, violations)
Setter->>DB: INSERT/UPDATE deployment_plan_target_result_validation
DB-->>Setter: Confirm upsert
Setter-->>Controller: Return error status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 22 minutes and 1 second.Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the plumbing for OPA-based “plan validation” rules in workspace-engine, including new DB tables/queries and OpenAPI schemas, plus a controller-side validator implementation intended to run after a plan target result completes.
Changes:
- Add
policy_rule_plan_validation_opaanddeployment_plan_target_result_validationtables + sqlc queries to load rules, fetch current version, and upsert validation results. - Extend deploymentplanresult Getter/Setter interfaces and Postgres implementations to support plan validation data access.
- Add OpenAPI schema/types for plan validation rules and results.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/workspace-engine/svc/controllers/deploymentplanresult/validation.go | Implements rule matching, OPA input building, evaluation, and persistence. |
| apps/workspace-engine/svc/controllers/deploymentplanresult/getters.go | Extends Getter interface for rule loading + current version lookup. |
| apps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.go | Implements Postgres getters for plan validation rule selection and current version lookup. |
| apps/workspace-engine/svc/controllers/deploymentplanresult/setters.go | Extends Setter interface for validation upsert. |
| apps/workspace-engine/svc/controllers/deploymentplanresult/setters_postgres.go | Implements Postgres upsert for validation results. |
| apps/workspace-engine/svc/controllers/deploymentplanresult/controller_test.go | Updates mocks to satisfy new interfaces. |
| apps/workspace-engine/pkg/db/sqlc.yaml | Registers new query file + type override for violations JSONB. |
| apps/workspace-engine/pkg/db/queries/schema.sql | Adds schema definitions for plan validation rules/results tables. |
| apps/workspace-engine/pkg/db/queries/plan_validation.sql | Adds sqlc queries for listing rules, reading current version, and upserting results. |
| apps/workspace-engine/pkg/db/plan_validation.sql.go | Generated sqlc output for new queries. |
| apps/workspace-engine/pkg/db/models.go | Adds generated model structs for new tables. |
| apps/workspace-engine/oapi/spec/schemas/policy.jsonnet | Adds planValidationOpa rule type to PolicyRule schema. |
| apps/workspace-engine/oapi/spec/schemas/plan_validation.jsonnet | Defines schemas for OPA plan validation rule + results. |
| apps/workspace-engine/oapi/spec/main.jsonnet | Includes the new plan validation schema module. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Generated OpenAPI types for plan validation rule/result. |
| apps/workspace-engine/oapi/openapi.json | Generated OpenAPI schema output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
+24
| func RunPlanValidation( | ||
| ctx context.Context, | ||
| getter Getter, | ||
| setter Setter, | ||
| result db.DeploymentPlanTargetResult, | ||
| planResult *types.PlanResult, | ||
| dispatchCtx oapi.DispatchContext, | ||
| ) error { |
There was a problem hiding this comment.
RunPlanValidation is not referenced anywhere in the repo right now, so the new plan validation logic will never run (and the PR description says it should run after saving a completed plan result). Wire this into the deploymentplanresult controller’s completed path (after UpdateDeploymentPlanTargetResultCompleted, before MaybeUpdateTargetCheck), or remove the unused implementation if it’s intentionally deferred.
Comment on lines
+17
to
+38
| func RunPlanValidation( | ||
| ctx context.Context, | ||
| getter Getter, | ||
| setter Setter, | ||
| result db.DeploymentPlanTargetResult, | ||
| planResult *types.PlanResult, | ||
| dispatchCtx oapi.DispatchContext, | ||
| ) error { | ||
| ctx, span := tracer.Start(ctx, "RunPlanValidation") | ||
| defer span.End() | ||
|
|
||
| span.SetAttributes(attribute.String("result_id", result.ID.String())) | ||
|
|
||
| target := &match.Target{ | ||
| Deployment: dispatchCtx.Deployment, | ||
| Environment: dispatchCtx.Environment, | ||
| Resource: dispatchCtx.Resource, | ||
| } | ||
|
|
||
| workspaceID, err := uuid.Parse(dispatchCtx.Environment.WorkspaceId) | ||
| if err != nil { | ||
| return fmt.Errorf("parse workspace id: %w", err) |
There was a problem hiding this comment.
RunPlanValidation dereferences dispatchCtx.Environment and planResult without nil checks (e.g., dispatchCtx.Environment.WorkspaceId, planResult.Current). Since both are pointers in the input types, this can panic if the caller passes an incomplete dispatch context or a nil plan result. Consider changing the signature to take a non-pointer plan result and explicitly validating required dispatch context fields up front (returning an error) to avoid panics.
Comment on lines
+17
to
+75
| func RunPlanValidation( | ||
| ctx context.Context, | ||
| getter Getter, | ||
| setter Setter, | ||
| result db.DeploymentPlanTargetResult, | ||
| planResult *types.PlanResult, | ||
| dispatchCtx oapi.DispatchContext, | ||
| ) error { | ||
| ctx, span := tracer.Start(ctx, "RunPlanValidation") | ||
| defer span.End() | ||
|
|
||
| span.SetAttributes(attribute.String("result_id", result.ID.String())) | ||
|
|
||
| target := &match.Target{ | ||
| Deployment: dispatchCtx.Deployment, | ||
| Environment: dispatchCtx.Environment, | ||
| Resource: dispatchCtx.Resource, | ||
| } | ||
|
|
||
| workspaceID, err := uuid.Parse(dispatchCtx.Environment.WorkspaceId) | ||
| if err != nil { | ||
| return fmt.Errorf("parse workspace id: %w", err) | ||
| } | ||
|
|
||
| rules, err := getter.GetMatchingPlanValidationOpaRules(ctx, workspaceID, target) | ||
| if err != nil { | ||
| return fmt.Errorf("get matching opa rules: %w", err) | ||
| } | ||
|
|
||
| span.SetAttributes(attribute.Int("rules.count", len(rules))) | ||
|
|
||
| if len(rules) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| input, err := buildOpaInput(ctx, getter, result.TargetID, planResult, dispatchCtx) | ||
| if err != nil { | ||
| return fmt.Errorf("build opa input: %w", err) | ||
| } | ||
|
|
||
| results, err := evaluateRules(ctx, rules, input) | ||
| if err != nil { | ||
| return fmt.Errorf("evaluate rules: %w", err) | ||
| } | ||
|
|
||
| span.SetAttributes(attribute.Int("rules.evaluated", len(results))) | ||
|
|
||
| for _, rule := range rules { | ||
| res, ok := results[rule.Id] | ||
| if !ok { | ||
| continue | ||
| } | ||
| if err := persistResult(ctx, setter, result.ID, rule, res); err != nil { | ||
| return fmt.Errorf("persist result for rule %s: %w", rule.Id, err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
This PR introduces substantial new behavior (rule loading, OPA input construction, rule evaluation, DB upsert) but there are no unit tests covering it. Add tests that verify: matching rules are selected by selector, OPA evaluation results are interpreted correctly (pass/deny messages), and UpsertPlanValidationResult is called with the expected passed/violations payloads.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/workspace-engine/svc/controllers/deploymentplanresult/controller_test.go (2)
128-133: ⚡ Quick winCapture validation upsert calls and allow injected errors in mock setter.
The current stub always succeeds and discards args, which prevents asserting invocation details and error propagation for the new validation write step.
Proposed test-harness improvement
type mockSetter struct { completedCalls []completedCall completedErr error stateCalls []stateCall stateErr error + + validationUpserts []db.UpsertPlanValidationResultParams + validationErr error } @@ func (m *mockSetter) UpsertPlanValidationResult( _ context.Context, - _ db.UpsertPlanValidationResultParams, + arg db.UpsertPlanValidationResultParams, ) error { - return nil + m.validationUpserts = append(m.validationUpserts, arg) + return m.validationErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplanresult/controller_test.go` around lines 128 - 133, The mock UpsertPlanValidationResult currently swallows its inputs and always returns nil; modify the mockSetter struct to include fields for capturing the last context and last params (e.g., lastUpsertCtx, lastUpsertParams) and an injectable error field (e.g., upsertErr), then update mockSetter.UpsertPlanValidationResult to store the incoming context and db.UpsertPlanValidationResultParams into those fields and return the injectable upsertErr so tests can assert the call parameters and simulate errors.
71-84: ⚡ Quick winExpose configurable returns in new getter stubs to enable validation-path tests.
Right now these methods are hardcoded to
nil, nil, so tests can’t meaningfully cover matched-rule/current-version branches for the new validation flow.Proposed test-harness improvement
type mockGetter struct { result db.DeploymentPlanTargetResult err error + planValidationRules []oapi.PolicyRule + planValidationErr error + currentVersion *oapi.DeploymentVersion + currentVersionErr error } @@ func (m *mockGetter) GetMatchingPlanValidationOpaRules( _ context.Context, _ uuid.UUID, _ *match.Target, ) ([]oapi.PolicyRule, error) { - return nil, nil + return m.planValidationRules, m.planValidationErr } @@ func (m *mockGetter) GetCurrentVersionForPlanTarget( _ context.Context, _ uuid.UUID, ) (*oapi.DeploymentVersion, error) { - return nil, nil + return m.currentVersion, m.currentVersionErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplanresult/controller_test.go` around lines 71 - 84, The two mock methods GetMatchingPlanValidationOpaRules and GetCurrentVersionForPlanTarget currently always return nil, nil which prevents exercising validation branches; modify the mockGetter type to include configurable fields (e.g., MatchingRulesReturn []oapi.PolicyRule and MatchingRulesErr error, and CurrentVersionReturn *oapi.DeploymentVersion and CurrentVersionErr error), then update GetMatchingPlanValidationOpaRules to return m.MatchingRulesReturn, m.MatchingRulesErr and GetCurrentVersionForPlanTarget to return m.CurrentVersionReturn, m.CurrentVersionErr so tests can inject different rule lists and version values/errors for validation-path test cases.
🤖 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/deploymentplanresult/validation.go`:
- Around line 30-39: The code dereferences dispatchCtx.Environment and
planResult without nil checks (e.g., when constructing match.Target and parsing
dispatchCtx.Environment.WorkspaceId), which can panic; update the validation
logic in validation.go to guard against nil dispatchCtx.Environment and nil
planResult before accessing their fields, returning a retriable error (with
clear context) when either is missing, and only call uuid.Parse on a non-empty
dispatchCtx.Environment.WorkspaceId; reference the match.Target construction,
the workspaceID parsing block, and any later uses of planResult to add these nil
checks and early returns.
---
Nitpick comments:
In
`@apps/workspace-engine/svc/controllers/deploymentplanresult/controller_test.go`:
- Around line 128-133: The mock UpsertPlanValidationResult currently swallows
its inputs and always returns nil; modify the mockSetter struct to include
fields for capturing the last context and last params (e.g., lastUpsertCtx,
lastUpsertParams) and an injectable error field (e.g., upsertErr), then update
mockSetter.UpsertPlanValidationResult to store the incoming context and
db.UpsertPlanValidationResultParams into those fields and return the injectable
upsertErr so tests can assert the call parameters and simulate errors.
- Around line 71-84: The two mock methods GetMatchingPlanValidationOpaRules and
GetCurrentVersionForPlanTarget currently always return nil, nil which prevents
exercising validation branches; modify the mockGetter type to include
configurable fields (e.g., MatchingRulesReturn []oapi.PolicyRule and
MatchingRulesErr error, and CurrentVersionReturn *oapi.DeploymentVersion and
CurrentVersionErr error), then update GetMatchingPlanValidationOpaRules to
return m.MatchingRulesReturn, m.MatchingRulesErr and
GetCurrentVersionForPlanTarget to return m.CurrentVersionReturn,
m.CurrentVersionErr so tests can inject different rule lists and version
values/errors for validation-path test cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22391fdb-d357-4a1a-9022-e9e9fd615564
📒 Files selected for processing (16)
apps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/main.jsonnetapps/workspace-engine/oapi/spec/schemas/plan_validation.jsonnetapps/workspace-engine/oapi/spec/schemas/policy.jsonnetapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/plan_validation.sql.goapps/workspace-engine/pkg/db/queries/plan_validation.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/sqlc.yamlapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/svc/controllers/deploymentplanresult/controller_test.goapps/workspace-engine/svc/controllers/deploymentplanresult/getters.goapps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.goapps/workspace-engine/svc/controllers/deploymentplanresult/setters.goapps/workspace-engine/svc/controllers/deploymentplanresult/setters_postgres.goapps/workspace-engine/svc/controllers/deploymentplanresult/validation.go
Comment on lines
+30
to
+39
| target := &match.Target{ | ||
| Deployment: dispatchCtx.Deployment, | ||
| Environment: dispatchCtx.Environment, | ||
| Resource: dispatchCtx.Resource, | ||
| } | ||
|
|
||
| workspaceID, err := uuid.Parse(dispatchCtx.Environment.WorkspaceId) | ||
| if err != nil { | ||
| return fmt.Errorf("parse workspace id: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent nil-pointer panics in validation path.
Line 36 and Line 90+ dereference pointer fields (dispatchCtx.Environment, planResult) without guards. If either is absent in stored dispatch context/plan payload, this panics the controller instead of returning a retriable error.
Suggested fix
func RunPlanValidation(
ctx context.Context,
getter Getter,
setter Setter,
result db.DeploymentPlanTargetResult,
planResult *types.PlanResult,
dispatchCtx oapi.DispatchContext,
) error {
ctx, span := tracer.Start(ctx, "RunPlanValidation")
defer span.End()
+ if planResult == nil {
+ return fmt.Errorf("missing plan result")
+ }
+ if dispatchCtx.Environment == nil {
+ return fmt.Errorf("missing environment in dispatch context")
+ }
+
span.SetAttributes(attribute.String("result_id", result.ID.String()))
@@
- workspaceID, err := uuid.Parse(dispatchCtx.Environment.WorkspaceId)
+ workspaceID, err := uuid.Parse(dispatchCtx.Environment.WorkspaceId)Also applies to: 89-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/validation.go`
around lines 30 - 39, The code dereferences dispatchCtx.Environment and
planResult without nil checks (e.g., when constructing match.Target and parsing
dispatchCtx.Environment.WorkspaceId), which can panic; update the validation
logic in validation.go to guard against nil dispatchCtx.Environment and nil
planResult before accessing their fields, returning a retriable error (with
clear context) when either is missing, and only call uuid.Parse on a non-empty
dispatchCtx.Environment.WorkspaceId; reference the match.Target construction,
the workspaceID parsing block, and any later uses of planResult to add these nil
checks and early returns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #1089
Summary by CodeRabbit