Fix/policy priority anyapproval authoritative#823
Fix/policy priority anyapproval authoritative#823zacharyblasczyk wants to merge 10 commits intomainfrom
Conversation
…roper file formatting
… job mapping; modify OpenAPI paths for deployment variable management and streamline deployment-related endpoints
… to clean up code and improve readability
…dels and queries; enhance job management with unique release IDs
… and related queries; update job retrieval and insertion logic to support new field
…hance job retrieval by release ID; implement GetByReleaseID method in job repository
When two policies with anyApproval rules target the same deployment, the higher priority policy should be authoritative. Currently both policies are applied independently, so a lower priority policy can block a deployment that a higher priority policy explicitly allows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Make explicit that higher numeric values mean higher priority, and that when multiple policies define the same rule type for the same target, only the highest priority policy's rule is authoritative. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
📝 WalkthroughWalkthroughThis pull request adds test coverage and documentation for policy priority semantics. A new end-to-end test validates that higher-priority policies take precedence over lower-priority ones during rule evaluation. A helper function enables priority assignment in test setup, and documentation clarifies that higher numeric priority values take precedence during rule resolution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/policies/overview.mdx (1)
294-297: Document tie behavior for equal priorities.The doc now defines “highest priority wins,” but it still leaves equal-priority collisions undefined for the same rule type. Please add the tie-break rule (or explicitly state it’s unsupported) to avoid nondeterministic interpretation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/policies/overview.mdx` around lines 294 - 297, Update the "Resolves rules by priority" item to explicitly define tie behavior for equal-priority rules: state the deterministic tie-breaker (for example, "if two rules have equal priority, the rule from the policy with the lexicographically smallest policy ID wins" or "the policy most recently applied wins") or explicitly declare that equal priorities are unsupported and will be rejected; update the sentence that currently reads "For each rule type, only the highest priority policy's rule is used" to include this tie-break rule so readers know how conflicts at the same priority are resolved.apps/workspace-engine/test/e2e/engine_policy_priority_test.go (2)
55-66: Trim inline comments that duplicate the code literals.These comments mostly restate immediate values/actions and add noise in an otherwise clear test setup.
As per coding guidelines, "Do not add comments that simply restate what the code does".
Also applies to: 79-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/e2e/engine_policy_priority_test.go` around lines 55 - 66, Remove the inline comments that simply repeat the code literals in the test policy setup; specifically delete the comment lines like "// High priority policy (priority=1): no approval needed" and "// Low priority policy (priority=0): requires 1 approval" that sit above the integration.WithPolicy blocks, and likewise remove the duplicate restating comments around the other policy definitions (around lines with integration.WithPolicy, integration.PolicyID, integration.PolicyName, integration.PolicyPriority, integration.WithPolicySelector, integration.WithPolicyRule, and integration.WithRuleAnyApproval) so the test remains concise and only contains descriptive comments when they add value.
85-91: Strengthen the assertion to prove approval is bypassed.The current check (
len(allJobs) == 1) validates job creation, but it doesn’t fully prove the lower-priority approval rule was ignored. Consider also asserting the deployment is not waiting on approvals (or equivalent approval-gated state) to avoid false positives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/e2e/engine_policy_priority_test.go` around lines 85 - 91, The test currently only checks job creation; after fetching allJobs via engine.Workspace().Jobs().Items(), also assert the created job is not in an approval-gated state by inspecting the job's approval/status fields (e.g., allJobs[0].Status.WaitingForApproval == false or calling a helper like allJobs[0].IsWaitingForApproval() or verifying the job phase is Running/Completed), so you prove the lower-priority anyApproval rule was bypassed rather than the job simply being created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/workspace-engine/test/e2e/engine_policy_priority_test.go`:
- Around line 55-66: Remove the inline comments that simply repeat the code
literals in the test policy setup; specifically delete the comment lines like
"// High priority policy (priority=1): no approval needed" and "// Low priority
policy (priority=0): requires 1 approval" that sit above the
integration.WithPolicy blocks, and likewise remove the duplicate restating
comments around the other policy definitions (around lines with
integration.WithPolicy, integration.PolicyID, integration.PolicyName,
integration.PolicyPriority, integration.WithPolicySelector,
integration.WithPolicyRule, and integration.WithRuleAnyApproval) so the test
remains concise and only contains descriptive comments when they add value.
- Around line 85-91: The test currently only checks job creation; after fetching
allJobs via engine.Workspace().Jobs().Items(), also assert the created job is
not in an approval-gated state by inspecting the job's approval/status fields
(e.g., allJobs[0].Status.WaitingForApproval == false or calling a helper like
allJobs[0].IsWaitingForApproval() or verifying the job phase is
Running/Completed), so you prove the lower-priority anyApproval rule was
bypassed rather than the job simply being created.
In `@docs/policies/overview.mdx`:
- Around line 294-297: Update the "Resolves rules by priority" item to
explicitly define tie behavior for equal-priority rules: state the deterministic
tie-breaker (for example, "if two rules have equal priority, the rule from the
policy with the lexicographically smallest policy ID wins" or "the policy most
recently applied wins") or explicitly declare that equal priorities are
unsupported and will be rejected; update the sentence that currently reads "For
each rule type, only the highest priority policy's rule is used" to include this
tie-break rule so readers know how conflicts at the same priority are resolved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/workspace-engine/test/e2e/engine_policy_priority_test.goapps/workspace-engine/test/integration/opts.godocs/policies/overview.mdx
4 participants
Two policies target the same deployment with anyApproval rules. The higher priority policy (priority 1) requires 0 approvals, the lower priority policy (priority 0) requires 1 approval. The test expects the higher priority policy to win, allowing deployment without approvals. It fails because both rules are evaluated independently and both must pass.
Summary by CodeRabbit