Open
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a new database query Changes
Sequence DiagramsequenceDiagram
participant Client
participant PostgresGetter
participant Database
participant Converter
participant API
Client->>PostgresGetter: GetPolicies(ctx, ReleaseTarget)
PostgresGetter->>Database: ListPoliciesWithRulesByWorkspaceID(ctx, workspaceID)
Database-->>PostgresGetter: []ListPoliciesWithRulesByWorkspaceIDRow
loop For each policy row
PostgresGetter->>Converter: PolicyWithRules(row)
Note over Converter: Parse 10 rule types from JSON
Note over Converter: Convert to oapi.Policy structure
Converter-->>PostgresGetter: *oapi.Policy
end
PostgresGetter-->>Client: []*oapi.Policy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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: 1
🧹 Nitpick comments (3)
apps/workspace-engine/svc/controllers/desiredrelease/convert/policy.go (2)
109-109: Add a doc comment for exportedPolicyWithRules.Please add a short Go doc comment that explains intent/contract for this exported converter.
As per coding guidelines, "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and 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/desiredrelease/convert/policy.go` at line 109, Add a Go doc comment for the exported function PolicyWithRules that briefly explains what the function converts and its contract: describe that PolicyWithRules takes a db.ListPoliciesWithRulesByWorkspaceIDRow (database row containing policy and associated rules) and returns an OpenAPI policy representation (*oapi.Policy) or an error, including any important behaviors (e.g., when it returns nil vs an error, whether it copies/normalizes fields, and that it expects the row to include rules for the workspace). Place the comment immediately above the PolicyWithRules function declaration.
134-195: Wrap parser errors with rule-type context.Right now malformed JSON surfaces as a generic unmarshal error. Add field-specific context to speed up debugging.
🛠️ Suggested change
import ( "encoding/json" + "fmt" "time" "workspace-engine/pkg/db" "workspace-engine/pkg/oapi" ) @@ anyApprovals, err := parseAnyApprovalRules(row.AnyApprovalRules) if err != nil { - return nil, err + return nil, fmt.Errorf("parse any_approval_rules: %w", err) } @@ depDeps, err := parseDeploymentDependencyRules(row.DeploymentDependencyRules) if err != nil { - return nil, err + return nil, fmt.Errorf("parse deployment_dependency_rules: %w", err) } @@ windows, err := parseDeploymentWindowRules(row.DeploymentWindowRules) if err != nil { - return nil, err + return nil, fmt.Errorf("parse deployment_window_rules: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/convert/policy.go` around lines 134 - 195, parseAllRules currently returns raw unmarshal errors from each specific parser (e.g., parseAnyApprovalRules, parseDeploymentDependencyRules, parseDeploymentWindowRules, parseEnvironmentProgressionRules, parseGradualRolloutRules, parseRetryRules, parseRollbackRules, parseVerificationRules, parseVersionCooldownRules, parseVersionSelectorRules) which makes it hard to know which rule type failed; update parseAllRules so that whenever a parser call returns an error you wrap it with contextual information naming the rule type and source field (for example fmt.Errorf("parseAnyApprovalRules (AnyApprovalRules): %w", err)) and propagate that wrapped error, ensuring each error return includes the parser function name and the corresponding row field to improve debuggability.apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go (1)
73-76: Add identifying context to the list-policies error.Including
deployment_idand/orworkspace_idin this error message makes production triage faster.🔧 Suggested tweak
rows, err := q.ListPoliciesWithRulesByWorkspaceID(ctx, depRow.WorkspaceID) if err != nil { - return nil, fmt.Errorf("list policies: %w", err) + return nil, fmt.Errorf("list policies for workspace %s (deployment %s): %w", depRow.WorkspaceID, rt.DeploymentID, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go` around lines 73 - 76, The error returned from calling ListPoliciesWithRulesByWorkspaceID lacks identifying context; update the error returned in the block where rows, err := q.ListPoliciesWithRulesByWorkspaceID(ctx, depRow.WorkspaceID) is handled to include depRow.WorkspaceID and depRow.ID (or deployment_id) in the message—e.g., wrap the original error with fmt.Errorf("list policies workspace_id=%s deployment_id=%s: %w", depRow.WorkspaceID, depRow.ID, err) so the log includes both workspace and deployment identifiers; ensure you reference ListPoliciesWithRulesByWorkspaceID, depRow.WorkspaceID and depRow.ID (or the field name used for deployment id) when making the change.
🤖 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/policies.sql`:
- Around line 223-242: The JSON aggregate subqueries (e.g., any_approval_rules /
policy_rule_any_approval, deployment_dependency_rules /
policy_rule_deployment_dependency, deployment_window_rules /
policy_rule_deployment_window, environment_progression_rules /
policy_rule_environment_progression, gradual_rollout_rules /
policy_rule_gradual_rollout, retry_rules / policy_rule_retry, rollback_rules /
policy_rule_rollback, verification_rules / policy_rule_verification,
version_cooldown_rules / policy_rule_version_cooldown, version_selector_rules /
policy_rule_version_selector) are using json_agg(...) without ORDER BY so their
element order is non-deterministic; fix each subquery by adding a deterministic
ORDER BY (for example ORDER BY r.id ASC or ORDER BY r.created_at ASC) inside the
SELECT ... FROM ... WHERE r.policy_id = p.id subquery so json_agg produces a
stable ordering for API responses.
---
Nitpick comments:
In `@apps/workspace-engine/svc/controllers/desiredrelease/convert/policy.go`:
- Line 109: Add a Go doc comment for the exported function PolicyWithRules that
briefly explains what the function converts and its contract: describe that
PolicyWithRules takes a db.ListPoliciesWithRulesByWorkspaceIDRow (database row
containing policy and associated rules) and returns an OpenAPI policy
representation (*oapi.Policy) or an error, including any important behaviors
(e.g., when it returns nil vs an error, whether it copies/normalizes fields, and
that it expects the row to include rules for the workspace). Place the comment
immediately above the PolicyWithRules function declaration.
- Around line 134-195: parseAllRules currently returns raw unmarshal errors from
each specific parser (e.g., parseAnyApprovalRules,
parseDeploymentDependencyRules, parseDeploymentWindowRules,
parseEnvironmentProgressionRules, parseGradualRolloutRules, parseRetryRules,
parseRollbackRules, parseVerificationRules, parseVersionCooldownRules,
parseVersionSelectorRules) which makes it hard to know which rule type failed;
update parseAllRules so that whenever a parser call returns an error you wrap it
with contextual information naming the rule type and source field (for example
fmt.Errorf("parseAnyApprovalRules (AnyApprovalRules): %w", err)) and propagate
that wrapped error, ensuring each error return includes the parser function name
and the corresponding row field to improve debuggability.
In `@apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go`:
- Around line 73-76: The error returned from calling
ListPoliciesWithRulesByWorkspaceID lacks identifying context; update the error
returned in the block where rows, err :=
q.ListPoliciesWithRulesByWorkspaceID(ctx, depRow.WorkspaceID) is handled to
include depRow.WorkspaceID and depRow.ID (or deployment_id) in the message—e.g.,
wrap the original error with fmt.Errorf("list policies workspace_id=%s
deployment_id=%s: %w", depRow.WorkspaceID, depRow.ID, err) so the log includes
both workspace and deployment identifiers; ensure you reference
ListPoliciesWithRulesByWorkspaceID, depRow.WorkspaceID and depRow.ID (or the
field name used for deployment id) when making the change.
ℹ️ 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 (5)
apps/workspace-engine/pkg/db/policies.sql.goapps/workspace-engine/pkg/db/queries/policies.sqlapps/workspace-engine/svc/controllers/desiredrelease/convert/policy.goapps/workspace-engine/svc/controllers/desiredrelease/convert/policy_test.goapps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go
Comment on lines
+223
to
+242
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'min_approvals', r.min_approvals, 'created_at', r.created_at)) | ||
| FROM policy_rule_any_approval r WHERE r.policy_id = p.id), '[]')::jsonb AS any_approval_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'depends_on', r.depends_on, 'created_at', r.created_at)) | ||
| FROM policy_rule_deployment_dependency r WHERE r.policy_id = p.id), '[]')::jsonb AS deployment_dependency_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'allow_window', r.allow_window, 'duration_minutes', r.duration_minutes, 'rrule', r.rrule, 'timezone', r.timezone, 'created_at', r.created_at)) | ||
| FROM policy_rule_deployment_window r WHERE r.policy_id = p.id), '[]')::jsonb AS deployment_window_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'depends_on_environment_selector', r.depends_on_environment_selector, 'maximum_age_hours', r.maximum_age_hours, 'minimum_soak_time_minutes', r.minimum_soak_time_minutes, 'minimum_success_percentage', r.minimum_success_percentage, 'success_statuses', r.success_statuses, 'created_at', r.created_at)) | ||
| FROM policy_rule_environment_progression r WHERE r.policy_id = p.id), '[]')::jsonb AS environment_progression_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'rollout_type', r.rollout_type, 'time_scale_interval', r.time_scale_interval, 'created_at', r.created_at)) | ||
| FROM policy_rule_gradual_rollout r WHERE r.policy_id = p.id), '[]')::jsonb AS gradual_rollout_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'max_retries', r.max_retries, 'backoff_seconds', r.backoff_seconds, 'backoff_strategy', r.backoff_strategy, 'max_backoff_seconds', r.max_backoff_seconds, 'retry_on_statuses', r.retry_on_statuses, 'created_at', r.created_at)) | ||
| FROM policy_rule_retry r WHERE r.policy_id = p.id), '[]')::jsonb AS retry_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'on_job_statuses', r.on_job_statuses, 'on_verification_failure', r.on_verification_failure, 'created_at', r.created_at)) | ||
| FROM policy_rule_rollback r WHERE r.policy_id = p.id), '[]')::jsonb AS rollback_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'metrics', r.metrics, 'trigger_on', r.trigger_on, 'created_at', r.created_at)) | ||
| FROM policy_rule_verification r WHERE r.policy_id = p.id), '[]')::jsonb AS verification_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'interval_seconds', r.interval_seconds, 'created_at', r.created_at)) | ||
| FROM policy_rule_version_cooldown r WHERE r.policy_id = p.id), '[]')::jsonb AS version_cooldown_rules, | ||
| COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'description', r.description, 'selector', r.selector, 'created_at', r.created_at)) | ||
| FROM policy_rule_version_selector r WHERE r.policy_id = p.id), '[]')::jsonb AS version_selector_rules |
Contributor
There was a problem hiding this comment.
Make aggregated rule array ordering deterministic.
Each json_agg(...) here has no ORDER BY, so element order is not guaranteed. That can make API responses unstable across runs.
💡 Suggested pattern (apply to each rule subquery)
- COALESCE((SELECT json_agg(json_build_object('id', r.id, 'policy_id', r.policy_id, 'min_approvals', r.min_approvals, 'created_at', r.created_at))
+ COALESCE((SELECT json_agg(
+ json_build_object('id', r.id, 'policy_id', r.policy_id, 'min_approvals', r.min_approvals, 'created_at', r.created_at)
+ ORDER BY r.created_at, r.id
+ )
FROM policy_rule_any_approval r WHERE r.policy_id = p.id), '[]')::jsonb AS any_approval_rules,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/db/queries/policies.sql` around lines 223 - 242,
The JSON aggregate subqueries (e.g., any_approval_rules /
policy_rule_any_approval, deployment_dependency_rules /
policy_rule_deployment_dependency, deployment_window_rules /
policy_rule_deployment_window, environment_progression_rules /
policy_rule_environment_progression, gradual_rollout_rules /
policy_rule_gradual_rollout, retry_rules / policy_rule_retry, rollback_rules /
policy_rule_rollback, verification_rules / policy_rule_verification,
version_cooldown_rules / policy_rule_version_cooldown, version_selector_rules /
policy_rule_version_selector) are using json_agg(...) without ORDER BY so their
element order is non-deterministic; fix each subquery by adding a deterministic
ORDER BY (for example ORDER BY r.id ASC or ORDER BY r.created_at ASC) inside the
SELECT ... FROM ... WHERE r.policy_id = p.id subquery so json_agg produces a
stable ordering for API responses.
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