refactor: replace selectors with a single selector field in policy sc…#794
refactor: replace selectors with a single selector field in policy sc…#794
Conversation
…hemas - Consolidated the `selectors` field into a single `selector` field across various policy schemas, simplifying the structure and enhancing clarity. - Updated OpenAPI specifications and related code to reflect this change, including adjustments in the database models and API routes. - Removed the deprecated `PolicyTargetSelector` schema, streamlining the policy management process. - Enhanced the CEL expression handling for matching release targets, allowing for more flexible policy definitions.
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughReplaces per-target Changes
Sequence DiagramsequenceDiagram
participant Client as Client/API
participant Route as Policy Route Handler
participant Registry as JSONEntityRegistry
participant Migration as Policy Migration
participant Store as Entity Store
participant Matcher as Policy Matcher
participant CEL as CEL Runtime
Client->>Route: UpsertPolicy (legacy `selectors` or new `selector`)
Route->>Registry: Unmarshal(raw policy JSON)
Registry->>Migration: MigrateRaw(entityType="policy", data)
Migration->>Migration: PolicySelectorsToSelector() -> produce `selector` string
Migration-->>Registry: migrated JSON with `selector`
Registry->>Store: Unmarshal into Policy struct (with `selector`)
Route->>Store: Save policy
Matcher->>Store: GetPolicies(releaseTarget)
loop for each policy
Matcher->>CEL: CompileProgram(policy.Selector)
CEL-->>Matcher: Program
Matcher->>CEL: BuildEntityContext(resource, deployment, environment)
CEL-->>Matcher: context map
Matcher->>CEL: Eval(program, context)
CEL-->>Matcher: bool (match)
alt matches
Matcher-->>Store: include policy
else
Matcher-->>Store: skip policy
end
end
Store-->>Matcher: filtered policies
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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 |
- Modified the MatchPolicy function to return false for empty selectors, aligning behavior with the updated policy schema. - Adjusted related test cases to reflect the new logic, ensuring accurate evaluation of policy matching. - Cleaned up unnecessary whitespace in test files for improved readability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/api/src/types/openapi.ts`:
- Around line 1512-1524: The OpenAPI type UpsertPolicyRequest currently requires
enabled, metadata, priority, rules, and selector which breaks backward
compatibility; either make these fields optional in
apps/api/src/types/openapi.ts (change enabled, metadata, priority, rules,
selector to optional with same types) or update the upsert handler
(upsertPolicy) to apply the same defaults as createPolicy (enabled: true,
priority: 0, metadata: {}, selector: "true") when those properties are missing;
pick one approach and keep the other consistent (type vs runtime) so partial
upsert payloads from existing clients continue to work.
In `@apps/web/app/api/openapi.ts`:
- Around line 1512-1523: The OpenAPI schema change makes UpsertPolicyRequest
breaking by making enabled, metadata, priority, and rules required and by
replacing selectors?: PolicyTargetSelector[] with selector: string; revert or
adjust the schema so existing clients keep compatibility: either mark enabled,
metadata, priority, and rules as optional again in the UpsertPolicyRequest
schema or add a new versioned request type (e.g., UpsertPolicyRequestV2) while
preserving the original UpsertPolicyRequest with selectors?:
PolicyTargetSelector[]; update any server-side validation logic that enforces
required fields (the upsert endpoint handler) to accept the original optional
fields or branch on request version, and ensure selector/selectors mapping is
handled (convert incoming selectors array to the new selector string if
supporting both) so current consumers don’t break.
In `@apps/workspace-engine/pkg/persistence/migrations/policy.go`:
- Around line 59-66: The code joins sub-selector CEL expressions in andParts
with " && " without grouping each part, which breaks precedence when parts
contain "||"; update the assembly so each element from andParts is wrapped in
parentheses before joining (same pattern used for orClauses) — locate the block
that builds clause from andParts (the variables andParts, clause, and the
strings.Join call) and replace the join step with one that parenthesizes each
andParts entry prior to joining so the combined clause preserves intended
operator grouping.
In `@apps/workspace-engine/pkg/persistence/registry.go`:
- Around line 59-77: Snapshot the migrations slice while holding the registry
lock to avoid concurrent mutation: inside the lock read r.migrations[entityType]
into a new slice copy (e.g., make a slice of the same length and copy(...) or
append([]Migration(nil), ...)) and then release the lock, and iterate over that
copied slice when calling each migration's Migrate method; this ensures
RegisterMigration cannot mutate the underlying slice during iteration over
migrations in the code handling r.migrations, Migrate and mig.Name.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/deployment/planner_bench_test.go`:
- Line 220: Remove the redundant inline comment that restates the code (the
comment saying "Policy selector is already set to "true" which matches all
targets") in the test near the policy selector setup in planner_bench_test.go;
simply delete that comment line so the code is cleaner and leave the surrounding
test logic (the policy selector assignment and assertions) unchanged, then run
the tests to ensure no behavioral changes.
🧹 Nitpick comments (9)
apps/workspace-engine/pkg/selector/langs/cel/cel_test.go (2)
85-100: Consider adding an end-to-end compile + evaluate test.The
CompileProgramtests verify compilation only. A test that compiles an expression, builds a context viaBuildEntityContext, and then evaluates the program against that context would validate the two functions work together and that the CEL environment declarations match the context map shape. For example:func TestCompileAndEvaluate(t *testing.T) { program, err := cel.CompileProgram("resource.name == 'web'") require.NoError(t, err) r := &oapi.Resource{Id: "r1", Name: "web", Kind: "k8s"} ctx := cel.BuildEntityContext(r, nil, nil) out, _, err := program.Eval(ctx) require.NoError(t, err) assert.Equal(t, true, out.Value()) }This would catch mismatches between the declared CEL variables and the runtime context structure early.
67-83: Consider testing additional partial combinations and edge-case field values.Currently only the "resource populated, others nil" partial case is tested. Adding a case with only
Deploymentor onlyEnvironmentpopulated would strengthen coverage. Also, testing edge-case field values (empty strings, special characters in metadata keys/values) would help catch unexpected behavior. Based on learnings, edge cases like empty values, special characters, and unicode should be included for condition types.apps/workspace-engine/pkg/db/workspace_load.go (1)
172-179: Consider removing this stub and its call in LoadWorkspace since policies are loaded via the persistence layer, not here.The
loadPoliciesfunction always returnsnil, nil, making the error handling at line 295 unreachable and the span monitoring overhead (always reports count=0). Unlike other load functions that call database getters, this is pure dead code. If keeping it temporarily during the refactor, add a// TODO: remove once ...comment to clarify the intent.apps/workspace-engine/pkg/persistence/migrations/policy_test.go (1)
104-130: Minor inconsistency: some tests omit theselectorsdeletion assertion.
TestPolicySelectorsToSelector_SingleSelector_OnlyEnvironment(Line 116) andTestPolicySelectorsToSelector_SingleSelector_OnlyResource(Line 129) don't assertassert.Nil(t, result["selectors"]), unlike the other single-selector tests (e.g., Lines 86, 101). Consider adding the assertion for consistency.apps/workspace-engine/pkg/selector/langs/cel/cel.go (1)
172-175: Nil-guard fallback type ismap[string]anybut the non-nil path ismap[string]string.When
d.Metadata/e.Metadatais non-nil, the map value ismap[string]string. When nil, the fallback ismap[string]any. This type inconsistency could cause subtle CEL evaluation differences. Consider usingmap[string]string{}for the fallback to keep the type uniform.Also,
resourceToMap(Line 148) doesn't apply the same nil guard forr.Metadata— worth keeping consistent if nil metadata is possible on resources too.Proposed fix
m["metadata"] = d.Metadata if d.Metadata == nil { - m["metadata"] = make(map[string]any) + m["metadata"] = make(map[string]string) }Same for
environmentToMap:m["metadata"] = e.Metadata if e.Metadata == nil { - m["metadata"] = make(map[string]any) + m["metadata"] = make(map[string]string) }Also applies to: 194-197
apps/workspace-engine/pkg/selector/policies.go (1)
43-50: CEL compilation errors and evaluation errors are silently discarded.Both the compilation error (line 44) and the evaluation error (line 49) are swallowed. While the fail-closed behavior (returning
false) is safe, silent failures make misconfigured selectors very hard to debug — an operator will see a policy silently not matching any target with no indication of why.Consider logging at
warn/debuglevel so misconfigurations surface in observability:Suggested improvement
+import "log/slog" + func MatchPolicy(_ context.Context, policy *oapi.Policy, releaseTarget *ResolvedReleaseTarget) bool { if policy.Selector == "" || policy.Selector == "true" { return true } program, err := celLang.CompileProgram(policy.Selector) if err != nil { + slog.Warn("failed to compile policy CEL selector", "policy", policy.Id, "selector", policy.Selector, "error", err) return false } celCtx := celLang.BuildEntityContext(releaseTarget.Resource(), releaseTarget.Deployment(), releaseTarget.Environment()) - result, _ := celutil.EvalBool(program, celCtx) + result, err := celutil.EvalBool(program, celCtx) + if err != nil { + slog.Warn("failed to evaluate policy CEL selector", "policy", policy.Id, "selector", policy.Selector, "error", err) + } return result }apps/workspace-engine/pkg/selector/policies_test.go (1)
128-135: Consider adding a test for a nilreleaseTargetparameter.The test at line 128 covers nil fields within a
ResolvedReleaseTarget, but ifMatchPolicyis ever called with a nilreleaseTargetand a non-trivial selector, it will panic atreleaseTarget.Resource()inpolicies.go:48. If this is a possible call-site scenario, a guard or a test documenting the expected panic would be useful.apps/workspace-engine/test/e2e/engine_presistence_test.go (1)
385-385: Consider asserting the exact selector value after persistence round-trip.
NotEmptyonly verifies the field isn't blank. Since this is a persistence test validating that data survives a reload, asserting the exact value would catch silent corruption or normalization bugs:- assert.NotEmpty(t, loadedPolicy.Selector) + assert.Equal(t, `deployment.name == "frontend"`, loadedPolicy.Selector)apps/workspace-engine/test/e2e/engine_policy_test.go (1)
831-838: Duplicate policy name across two distinct policies.Both policies on lines 832 and 836 are named
"policy-web-apps". While they have distinct auto-generated IDs and different selectors, using the same name could hinder debugging. Consider giving the second policy a distinct name (e.g.,"policy-web-apps-dev").✏️ Suggested name differentiation
integration.WithPolicy( - integration.PolicyName("policy-web-apps"), + integration.PolicyName("policy-web-apps-dev"), integration.WithPolicySelector("deployment.name.contains('dev') && deployment.name.contains('web') && environment.name.contains('west')"), ),
| UpsertPolicyRequest: { | ||
| description?: string; | ||
| enabled?: boolean; | ||
| enabled: boolean; | ||
| /** @description Arbitrary metadata for the policy (record<string, string>) */ | ||
| metadata?: { | ||
| metadata: { | ||
| [key: string]: string; | ||
| }; | ||
| name: string; | ||
| priority?: number; | ||
| rules?: components["schemas"]["PolicyRule"][]; | ||
| selectors?: components["schemas"]["PolicyTargetSelector"][]; | ||
| priority: number; | ||
| rules: components["schemas"]["PolicyRule"][]; | ||
| /** @description CEL expression for matching release targets. Use "true" to match all targets. */ | ||
| selector: string; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for callers constructing or using UpsertPolicyRequest payloads
rg -n --type=ts -C3 'UpsertPolicyRequest|requestPolicyUpsert|upsertPolicy' -g '!**/openapi.ts' | head -100Repository: ctrlplanedev/ctrlplane
Length of output: 5300
🏁 Script executed:
#!/bin/bash
# Also search for any test files or examples that might construct UpsertPolicyRequest
rg -n 'UpsertPolicyRequest' --type=ts --type=js -C2Repository: ctrlplanedev/ctrlplane
Length of output: 1326
🏁 Script executed:
#!/bin/bash
# Check if there are any type definitions or interfaces that extend or reference UpsertPolicyRequest
rg -n 'extends.*UpsertPolicyRequest|implements.*UpsertPolicyRequest' --type=tsRepository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Search for API calls that might be affected, looking for patterns like axios/fetch calls with policy upsert
rg -n 'PUT.*policy|PATCH.*policy' --type=ts --type=js -C2 | head -80Repository: ctrlplanedev/ctrlplane
Length of output: 3369
🏁 Script executed:
#!/bin/bash
# Look at the upsertPolicyFixtures implementation
sed -n '549,600p' e2e/api/entities-builder.tsRepository: ctrlplanedev/ctrlplane
Length of output: 1437
🏁 Script executed:
#!/bin/bash
# Examine the upsertPolicy route handler
sed -n '58,120p' apps/api/src/routes/v1/workspaces/policies.tsRepository: ctrlplanedev/ctrlplane
Length of output: 1891
🏁 Script executed:
#!/bin/bash
# Search for actual policy payload constructions in fixtures
rg -A 10 'policies:' e2e/fixtures/ --type=yaml --type=ymlRepository: ctrlplanedev/ctrlplane
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check if there's a fixture file referenced
find e2e -name "*.yaml" -o -name "*.yml" | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 990
🏁 Script executed:
#!/bin/bash
# Check one of the policy fixture files to see actual payload structure
cat e2e/tests/api/policies/approval-policy.spec.yaml | head -100Repository: ctrlplanedev/ctrlplane
Length of output: 1041
🏁 Script executed:
#!/bin/bash
# Search for actual PUT requests to the upsert endpoint
rg -n 'PUT.*policies' e2e --type=ts -C5Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if there are integration tests actually using the PUT upsert endpoint
rg -n 'api\.PUT.*policies' e2e --type=ts -A 10 | head -80Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Let's look closer at what fields the handler actually requires vs what's optional
sed -n '58,95p' apps/api/src/routes/v1/workspaces/policies.tsRepository: ctrlplanedev/ctrlplane
Length of output: 1229
🏁 Script executed:
#!/bin/bash
# Check the CreatePolicy endpoint to see what it requires for comparison
rg -n -A 30 'const createPolicy:' apps/api/src/routes/v1/workspaces/policies.tsRepository: ctrlplanedev/ctrlplane
Length of output: 870
UpsertPolicyRequest requires all major fields; this is a breaking API change from create behavior.
The enabled, metadata, priority, rules, and selector fields are now all required in upsert payloads. Unlike the createPolicy handler (which provides defaults: enabled: true, priority: 0, metadata: {}, selector: "true"), the upsertPolicy handler has no such defaults—it extracts fields directly from the request body. Clients that previously relied on optional defaults for partial upsert payloads will receive validation errors. This is an intentional asymmetry between create and upsert semantics, but callers (SDKs, internal services, or direct API consumers) must be updated to provide all required fields.
🤖 Prompt for AI Agents
In `@apps/api/src/types/openapi.ts` around lines 1512 - 1524, The OpenAPI type
UpsertPolicyRequest currently requires enabled, metadata, priority, rules, and
selector which breaks backward compatibility; either make these fields optional
in apps/api/src/types/openapi.ts (change enabled, metadata, priority, rules,
selector to optional with same types) or update the upsert handler
(upsertPolicy) to apply the same defaults as createPolicy (enabled: true,
priority: 0, metadata: {}, selector: "true") when those properties are missing;
pick one approach and keep the other consistent (type vs runtime) so partial
upsert payloads from existing clients continue to work.
| UpsertPolicyRequest: { | ||
| description?: string; | ||
| enabled?: boolean; | ||
| enabled: boolean; | ||
| /** @description Arbitrary metadata for the policy (record<string, string>) */ | ||
| metadata?: { | ||
| metadata: { | ||
| [key: string]: string; | ||
| }; | ||
| name: string; | ||
| priority?: number; | ||
| rules?: components["schemas"]["PolicyRule"][]; | ||
| selectors?: components["schemas"]["PolicyTargetSelector"][]; | ||
| priority: number; | ||
| rules: components["schemas"]["PolicyRule"][]; | ||
| /** @description CEL expression for matching release targets. Use "true" to match all targets. */ | ||
| selector: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'PolicyTargetSelector' --type=tsRepository: ctrlplanedev/ctrlplane
Length of output: 435
🏁 Script executed:
cat -n apps/web/app/api/openapi.ts | sed -n '1512,1523p'Repository: ctrlplanedev/ctrlplane
Length of output: 667
🏁 Script executed:
# Also check if PolicyTargetSelector is defined anywhere in the file
rg -n 'PolicyTargetSelector' apps/web/app/api/openapi.ts -C3Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check a wider context around lines 1512-1523 to see the old structure (if any) and confirm the current state
cat -n apps/web/app/api/openapi.ts | sed -n '1500,1550p'Repository: ctrlplanedev/ctrlplane
Length of output: 2229
🏁 Script executed:
cd apps/web/app/api && git diff HEAD -- openapi.ts | head -150Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Also check if there's a commit message or PR info that confirms what changed
git log --oneline -20 -- apps/web/app/api/openapi.tsRepository: ctrlplanedev/ctrlplane
Length of output: 149
🏁 Script executed:
git show cb40d5f9:apps/web/app/api/openapi.ts | sed -n '1500,1550p'Repository: ctrlplanedev/ctrlplane
Length of output: 1872
🏁 Script executed:
# Show the actual commit diff for the UpsertPolicyRequest section
git show cb40d5f9 -- apps/web/app/api/openapi.ts | grep -A50 "UpsertPolicyRequest"Repository: ctrlplanedev/ctrlplane
Length of output: 3649
🏁 Script executed:
# Show the actual diff (before and after) for this commit
git show cb40d5f9 -- apps/web/app/api/openapi.ts | grep -B20 -A20 "UpsertPolicyRequest" | head -100Repository: ctrlplanedev/ctrlplane
Length of output: 2850
🏁 Script executed:
# Check the previous version of the file to see what UpsertPolicyRequest looked like before
git show cb40d5f9^:apps/web/app/api/openapi.ts 2>/dev/null | grep -A30 "UpsertPolicyRequest:" || echo "Pattern not found or commit has no parent"Repository: ctrlplanedev/ctrlplane
Length of output: 107
🏁 Script executed:
# Check git log to see if there's a prior version with 'selectors' instead of 'selector'
git log --all --oneline -p -- apps/web/app/api/openapi.ts | grep -B5 -A15 "selectors.*PolicyTargetSelector" | head -50Repository: ctrlplanedev/ctrlplane
Length of output: 1666
🏁 Script executed:
# Also check if this file is auto-generated by looking for a generation script or header
head -20 apps/web/app/api/openapi.tsRepository: ctrlplanedev/ctrlplane
Length of output: 570
Breaking API change: UpsertPolicyRequest introduces multiple required fields and replaces selectors with selector.
This is a breaking change for existing API consumers:
priorityandrules— promoted from optional to required.enabledandmetadata— new required fields.selectors?: PolicyTargetSelector[]→selector: string— field renamed, type changed from array to string, and required.
Existing clients calling the upsert endpoint without these fields will receive validation errors. Ensure all known consumers are updated and that API versioning or migration documentation addresses this breaking change. Note: PolicyTargetSelector remains in use in the UI layer; only the OpenAPI schema was regenerated.
🤖 Prompt for AI Agents
In `@apps/web/app/api/openapi.ts` around lines 1512 - 1523, The OpenAPI schema
change makes UpsertPolicyRequest breaking by making enabled, metadata, priority,
and rules required and by replacing selectors?: PolicyTargetSelector[] with
selector: string; revert or adjust the schema so existing clients keep
compatibility: either mark enabled, metadata, priority, and rules as optional
again in the UpsertPolicyRequest schema or add a new versioned request type
(e.g., UpsertPolicyRequestV2) while preserving the original UpsertPolicyRequest
with selectors?: PolicyTargetSelector[]; update any server-side validation logic
that enforces required fields (the upsert endpoint handler) to accept the
original optional fields or branch on request version, and ensure
selector/selectors mapping is handled (convert incoming selectors array to the
new selector string if supporting both) so current consumers don’t break.
apps/workspace-engine/pkg/workspace/releasemanager/deployment/planner_bench_test.go
Outdated
Show resolved
Hide resolved
- Introduced new test cases to validate the behavior of the PolicySelectorsToSelector function, specifically for scenarios where sub-selectors are omitted or consist entirely of "true" values. - Enhanced logging within the PolicySelectorsToSelector function to track migration execution for better debugging and monitoring. - Updated the extractSelectorCELParts function to handle "true" as a valid case for omission, ensuring accurate selector processing.
- Eliminated several outdated API paths related to deployment versions and environments from the schema, streamlining the interface and improving clarity. - Updated the OpenAPI specifications to reflect these changes, ensuring accurate documentation and alignment with current functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/persistence/migrations/policy_test.go`:
- Around line 69-87: Add a new unit test
TestPolicySelectorsToSelector_SubSelectorWithOR that constructs a policy with a
single selector whose deploymentSelector CEL contains "||" and an
environmentSelector, call migrations.PolicySelectorsToSelector("policy", data),
require no error, and assert that result["selector"] equals "(deployment.name ==
'a' || deployment.name == 'b') && (environment.name == 'prod')"; after you apply
the andParts wrapping fix in policy.go, update the existing assertions in
TestPolicySelectorsToSelector_SingleSelector_AllDimensions and
TestPolicySelectorsToSelector_MultipleSelectors_ORed so each AND-joined part is
wrapped in parentheses to match the new output format.
|
@cursoragent review |
High-levelSolid refactor—moving to a single CEL Major issues (blockers)
Medium issues / cleanup
Suggested path to green
|
- Updated the upsertPolicy function to ensure that the rules field is always populated with the provided rules from the request body, removing the fallback to an empty array. - This change enhances the integrity of policy definitions by enforcing the presence of rules during policy creation and updates.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/routes/v1/workspaces/policies.ts (1)
100-102:⚠️ Potential issue | 🟡 MinorError message is misleading on the create path.
Line 101 always says
"Failed to update policy", but whenisUpdateisfalsethis is actually a create operation.Proposed fix
} catch { - throw new ApiError("Failed to update policy", 500); + throw new ApiError( + isUpdate ? "Failed to update policy" : "Failed to create policy", + 500, + ); }
🧹 Nitpick comments (1)
apps/api/src/routes/v1/workspaces/policies.ts (1)
66-74: UUID validation should precede the GET request.The
policyIdformat is validated on Line 72 after the GET call on Line 67, which means an invalid UUID still triggers a network request that will inevitably fail or return nothing. Move the validation before the GET to short-circuit early.♻️ Suggested reorder
const { workspaceId, policyId } = req.params; const { body } = req; const client = getClientFor(workspaceId); + + const policyIdResult = z.string().uuid().safeParse(policyId); + if (!policyIdResult.success) + throw new ApiError("Invalid policy ID: must be a valid UUID v4", 400); // Check if policy already exists const existingPolicy = await client.GET( "/v1/workspaces/{workspaceId}/policies/{policyId}", { params: { path: { workspaceId, policyId } } }, ); - const policyIdResult = z.string().uuid().safeParse(policyId); - if (!policyIdResult.success) - throw new ApiError("Invalid policy ID: must be a valid UUID v4", 400);
…rts; snapshot migrations slice under lock; update tests; remove redundant comment Co-authored-by: Justin Brooks <jsbroks@gmail.com>
…cy PolicyTargetSelector usage Co-authored-by: Justin Brooks <jsbroks@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/persistence/migrations/policy_test.go`:
- Around line 89-130: The tests fail because PolicySelectorsToSelector currently
wraps every AND-part with parentheses at the wrapped[j] = "(" + p + ")" site,
producing outputs like "(deployment.name == 'api')"; update the logic in
PolicySelectorsToSelector so it only adds parentheses when necessary (e.g., when
len(andParts) > 1 or when p itself contains operators/clauses) — special-case
single-part ANDs to return the raw CEL string unwrapped (and ensure the
OR-clause assembly still handles single vs multiple clauses correctly).
🧹 Nitpick comments (1)
apps/web/app/routes/ws/deployments/page.$deploymentId.policies.tsx (1)
268-275: Optional: simplify rendering since there's at most one selector.
getPolicySelectorDescriptionreturns either[]or a single-element array. The.map()loop works but is slightly roundabout — you could render the selector string directly:♻️ Suggested simplification
- {selectorSummaries.length > 0 && ( + {policy.policy.selector?.trim() && policy.policy.selector.trim() !== "true" && ( <div className="space-y-1 text-xs text-muted-foreground"> <div className="text-xs font-medium uppercase tracking-wide text-foreground"> Target selector (CEL) </div> - {selectorSummaries.map((value, idx) => ( - <div key={idx}>{value}</div> - ))} + <div>{policy.policy.selector.trim()}</div> </div> )}Alternatively, keep the current approach if you foresee the function returning multiple descriptions in the future.
…n to preserve CEL precedence
2 participants
…hemas
selectorsfield into a singleselectorfield across various policy schemas, simplifying the structure and enhancing clarity.PolicyTargetSelectorschema, streamlining the policy management process.Summary by CodeRabbit
Refactor
Chores
Tests