Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds first-class Policy persistence: DB schema and migration, TypeScript schema, Go DB models and sqlc queries, DB-backed PolicyRepo with mappers, wiring into store and DB repo, store migration from in-memory to DB-backed policies, and test updates (IDs, CreatedAt, callsite changes). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StorePolicies as Policies Store
participant Mapper
participant PolicyRepo
participant DB as Database
Client->>StorePolicies: Get(policyID)
StorePolicies->>PolicyRepo: Get(policyID)
PolicyRepo->>DB: GetPolicyByID(id)
DB-->>PolicyRepo: policy row
PolicyRepo->>DB: List*RulesByPolicyID(policyID)
DB-->>PolicyRepo: rule rows
PolicyRepo->>Mapper: PolicyToOapi(row, rules)
Mapper-->>PolicyRepo: oapi.Policy
PolicyRepo-->>StorePolicies: *oapi.Policy
StorePolicies-->>Client: Policy
sequenceDiagram
participant Client
participant StorePolicies as Policies Store
participant Mapper
participant PolicyRepo
participant DB as Database
Client->>StorePolicies: Set(entity)
StorePolicies->>Mapper: ToPolicyUpsertParams(entity)
Mapper-->>StorePolicies: UpsertPolicyParams
StorePolicies->>PolicyRepo: Set(entity)
PolicyRepo->>DB: UpsertPolicy(params)
DB-->>PolicyRepo: upsert result
PolicyRepo->>DB: Delete*RulesByPolicyID(policyID)
DB-->>PolicyRepo: success
PolicyRepo->>DB: Upsert*Rule(per rule)
DB-->>PolicyRepo: success
PolicyRepo-->>StorePolicies: completion
StorePolicies-->>Client: Success/Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/environment_summary_test.go (1)
178-189: SameCreatedAtinconsistency as ingradualrollout_test.go.
CreatedAtis set here (Line 181) but omitted on theapprovalPolicyat Lines 616–626 inTestGradualRolloutEnvironmentSummaryEvaluator_WithApprovalPolicy. If the rollout-start logic readsPolicy.CreatedAt, the omission there could silently affect results. This is a duplicate of the concern raised ingradualrollout_test.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/environment_summary_test.go` around lines 178 - 189, The test sets approvalPolicy.CreatedAt in this file but the same approvalPolicy in TestGradualRolloutEnvironmentSummaryEvaluator_WithApprovalPolicy omits CreatedAt; update the approvalPolicy instance in that test to include CreatedAt (e.g. use the same baseTime.Add(-1 * time.Hour).Format(time.RFC3339) or reuse the baseTime variable) so rollout-start logic that depends on oapi.Policy.CreatedAt sees consistent values across tests; modify the approvalPolicy in TestGradualRolloutEnvironmentSummaryEvaluator_WithApprovalPolicy to match the CreatedAt used here.
🧹 Nitpick comments (7)
packages/db/src/schema/policy.ts (2)
229-230: Add a.$type<>()annotation topolicyRuleVerification.metricsfor consistency and type safety.
policy.metadata(Line 21–24) uses.$type<Record<string, string>>()to provide TypeScript-level type information, butpolicyRuleVerification.metricson Line 230 omits it, leaving the column typed asunknown. Define and annotate the expected shape (e.g., an array of metric objects) for consistency and safer downstream consumption.♻️ Proposed fix
- metrics: jsonb("metrics").notNull().default("[]"), + metrics: jsonb("metrics").notNull().default("[]").$type<MetricDefinition[]>(),Add the
MetricDefinitioninterface (or import it from a shared types module) above the table definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schema/policy.ts` around lines 229 - 230, policyRuleVerification.metrics is currently typed as unknown; add a TypeScript type annotation for the column (e.g., define or import a MetricDefinition interface and annotate the column as MetricDefinition[]) and apply it via the .\$type<MetricDefinition[]>() call on the jsonb("metrics") column (mirror how policy.metadata uses .\$type<Record<string,string>>()) so the table definition (policyRuleVerification) exposes a strongly typed metrics field; ensure MetricDefinition is declared or imported above the table definition and referenced in the .\$type<> generic.
15-36: Consider adding indexes onworkspace_id(policy table) andpolicy_id(all rule tables).All ten rule tables reference
policy.idvia a foreign key withON DELETE CASCADE, andpolicyreferencesworkspace.id. Without indexes on these FK columns:
- Cascade deletes will perform sequential scans across all rule tables.
- Queries filtering policies by workspace will scan the full
policytable.Add indexes in the migration file, e.g.:
CREATE INDEX idx_policy_workspace_id ON policy(workspace_id); CREATE INDEX idx_policy_rule_any_approval_policy_id ON policy_rule_any_approval(policy_id); -- ... repeat for each rule table🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schema/policy.ts` around lines 15 - 36, The policy schema lacks DB indexes for the foreign keys: add an index on policy.workspaceId (workspace_id) and add indexes on each rule table's policy_id (the FK referencing policy.id) to avoid full-table scans and slow ON DELETE CASCADE cascades; update your migration to create e.g. an index for policy(workspace_id) and one index per rule table (e.g., policy_rule_any_approval.policy_id, etc.) so deletes and workspace-scoped queries use the indexes. Ensure the migration names are unique and reversible (CREATE INDEX ... and DROP INDEX ...) and reference the exact table/column identifiers used in the schema (policy, workspace_id, each rule table and policy_id).apps/workspace-engine/pkg/db/queries/schema.sql (1)
129-224: Missing FK indexes will cause full table scans on cascade deletes.Same concern as in
packages/db/src/schema/policy.ts: none of thepolicy_idcolumns in the ten rule tables have indexes, andpolicy.workspace_idis also unindexed. On production datasets,DELETE FROM policy WHERE workspace_id = $1(or any cascade from workspace deletion) will sequentially scan each of the ten rule tables.Add to the migration:
CREATE INDEX idx_policy_workspace_id ON policy(workspace_id); CREATE INDEX idx_policy_rule_any_approval_policy_id ON policy_rule_any_approval(policy_id); CREATE INDEX idx_policy_rule_deployment_dependency_policy_id ON policy_rule_deployment_dependency(policy_id); CREATE INDEX idx_policy_rule_deployment_window_policy_id ON policy_rule_deployment_window(policy_id); CREATE INDEX idx_policy_rule_environment_progression_policy_id ON policy_rule_environment_progression(policy_id); CREATE INDEX idx_policy_rule_gradual_rollout_policy_id ON policy_rule_gradual_rollout(policy_id); CREATE INDEX idx_policy_rule_retry_policy_id ON policy_rule_retry(policy_id); CREATE INDEX idx_policy_rule_rollback_policy_id ON policy_rule_rollback(policy_id); CREATE INDEX idx_policy_rule_verification_policy_id ON policy_rule_verification(policy_id); CREATE INDEX idx_policy_rule_version_cooldown_policy_id ON policy_rule_version_cooldown(policy_id); CREATE INDEX idx_policy_rule_version_selector_policy_id ON policy_rule_version_selector(policy_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/queries/schema.sql` around lines 129 - 224, The migration is missing indexes on policy.workspace_id and the policy_id FK columns in the ten rule tables which will force sequential scans on cascade deletes; add CREATE INDEX statements for policy(workspace_id) and for policy_id on each rule table (policy_rule_any_approval.policy_id, policy_rule_deployment_dependency.policy_id, policy_rule_deployment_window.policy_id, policy_rule_environment_progression.policy_id, policy_rule_gradual_rollout.policy_id, policy_rule_retry.policy_id, policy_rule_rollback.policy_id, policy_rule_verification.policy_id, policy_rule_version_cooldown.policy_id, policy_rule_version_selector.policy_id) in this schema.sql migration so DELETE FROM policy WHERE workspace_id = ... and workspace cascades use the indexes.apps/workspace-engine/pkg/workspace/store/repository/memory/repo.go (1)
432-432: Remove the boilerplate method comment.
// Policies implements repository.Repo.is redundant and doesn’t add non-obvious context.As per coding guidelines: "Do not add comments that simply restate what the code does".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/memory/repo.go` at line 432, Remove the redundant boilerplate comment preceding the Policies method in repo.go; delete the line "// Policies implements repository.Repo." so the Policies method stands without a comment that merely restates its signature or purpose (locate the Policies method declaration to remove that single-line comment).apps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.go (1)
68-68: Add doc comments for exported mapper functions.
PolicyToOapiandToPolicyUpsertParamsare exported and should be documented.Based on learnings: "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods".
Also applies to: 239-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.go` at line 68, Add Go doc comments for the exported mapper functions PolicyToOapi and ToPolicyUpsertParams: prepend each function with a short sentence starting with the function name that describes what it returns/does, include any non-obvious details about inputs (e.g., that PolicyToOapi maps a db.Policy and RuleRows into an oapi.Policy and how rules are represented) and note any important behavior or TODOs; ensure comments are full sentences beginning with the function name to satisfy godoc conventions so both PolicyToOapi and ToPolicyUpsertParams are documented.packages/db/drizzle/0148_jittery_wild_child.sql (1)
9-10: Add indexes for workspace and rule-lookup access paths.The migration creates FK/filter columns but no indexes on them. That will force full scans for common
workspace_idandpolicy_idlookups.📈 Suggested migration additions
+CREATE INDEX "policy_workspace_id_idx" ON "policy" ("workspace_id"); +CREATE INDEX "policy_rule_any_approval_policy_id_idx" ON "policy_rule_any_approval" ("policy_id"); +CREATE INDEX "policy_rule_deployment_dependency_policy_id_idx" ON "policy_rule_deployment_dependency" ("policy_id"); +CREATE INDEX "policy_rule_deployment_window_policy_id_idx" ON "policy_rule_deployment_window" ("policy_id"); +CREATE INDEX "policy_rule_environment_progression_policy_id_idx" ON "policy_rule_environment_progression" ("policy_id"); +CREATE INDEX "policy_rule_gradual_rollout_policy_id_idx" ON "policy_rule_gradual_rollout" ("policy_id"); +CREATE INDEX "policy_rule_retry_policy_id_idx" ON "policy_rule_retry" ("policy_id"); +CREATE INDEX "policy_rule_rollback_policy_id_idx" ON "policy_rule_rollback" ("policy_id"); +CREATE INDEX "policy_rule_verification_policy_id_idx" ON "policy_rule_verification" ("policy_id"); +CREATE INDEX "policy_rule_version_cooldown_policy_id_idx" ON "policy_rule_version_cooldown" ("policy_id"); +CREATE INDEX "policy_rule_version_selector_policy_id_idx" ON "policy_rule_version_selector" ("policy_id");Also applies to: 98-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/drizzle/0148_jittery_wild_child.sql` around lines 9 - 10, The migration adds FK/filter columns like "workspace_id" and "policy_id" but no indexes, causing full-table scans; update the migration(s) that define these columns (references to "workspace_id" and "policy_id" in this diff and the other affected migrations) to add appropriate B-tree indexes (e.g., CREATE INDEX ...) on "workspace_id" and on "policy_id" and consider adding a composite index that includes "created_at" for common time-ordered lookups; ensure index names are unique and aligned with the table names being altered so the new indexes are created as part of the same migration(s).apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go (1)
96-118: N+1 query pattern:Items()issues 10 queries per policy.With the default limit of 5000 policies, this method could execute up to 50,001 queries. Consider batch-loading rules for all policy IDs in a single query per rule type, or using a JOIN-based approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go` around lines 96 - 118, Items() currently lists policies then calls loadRules for each policy, causing an N+1 query pattern (one query in db.ListPoliciesByWorkspaceID plus per-policy queries in loadRules). Replace the per-policy loadRules calls with a batched rules fetch: after db.ListPoliciesByWorkspaceID, collect all policy IDs, call a single new query (or queries per rule type) such as ListRulesByPolicyIDs(policyIDs) or perform a JOIN to fetch rules for all policies at once, then map rules back to their policy IDs and call PolicyToOapi(row, rulesForRow) for each row; update or remove loadRules to avoid per-policy queries and ensure the result map is populated from the preloaded rules set.
🤖 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/workspace/store/policy.go`:
- Around line 54-57: The changelog entries are recorded even when repo writes
fail; update the Set and Remove call sites so that
p.store.changeset.RecordUpsert(policy) and
p.store.changeset.RecordDelete(policy) are only invoked when p.repo.Set(policy)
/ p.repo.Remove(policy) return nil—i.e., check the err and return or bail out on
error before calling RecordUpsert/RecordDelete (adjust in the blocks containing
p.repo.Set and p.repo.Remove, lines referenced around p.repo.Set,
p.store.changeset.RecordUpsert and p.repo.Remove,
p.store.changeset.RecordDelete).
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.go`:
- Around line 178-180: The code silently ignores json.Unmarshal errors for
r.Metrics, which can hide malformed persisted rule config and alter policy
behavior; change the json.Unmarshal call in mapper.go (where metrics :=
[]oapi.VerificationMetricSpec and _ = json.Unmarshal(r.Metrics, &metrics) are
used) to capture the error (err := json.Unmarshal(...)) and handle it instead of
discarding it — e.g., return or propagate a parse error (or at minimum log it
with context including r.ID or rule identifier) so invalid verification metrics
JSON is not treated as valid empty metrics.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go`:
- Around line 120-156: The loadRules function is currently swallowing all errors
from DB queries (ListAnyApprovalRulesByPolicyID,
ListDeploymentDependencyRulesByPolicyID, ListDeploymentWindowRulesByPolicyID,
ListEnvironmentProgressionRulesByPolicyID, ListGradualRolloutRulesByPolicyID,
ListRetryRulesByPolicyID, ListRollbackRulesByPolicyID,
ListVerificationRulesByPolicyID, ListVersionCooldownRulesByPolicyID,
ListVersionSelectorRulesByPolicyID) causing incomplete RuleRows to be returned
silently; change Repo.loadRules to return (RuleRows, error) instead of just
RuleRows, check each query call and if err != nil return a wrapped/annotated
error (or aggregate errors) immediately (or after attempting all calls) so
callers can distinguish "no rules" from "DB failure", and update callers of
loadRules to handle the error; alternatively if propagation is not possible, at
minimum log each non-nil error with context before continuing.
- Line 239: Update the OpenAPI spec to rename the property
MinimumSockTimeMinutes to MinimumSoakTimeMinutes, regenerate the client/server
code, and then update all references in the repo layer to use the corrected
field (e.g., change ep.MinimumSockTimeMinutes to ep.MinimumSoakTimeMinutes in
the code that calls optInt32ToPgint4); ensure the OpenAPI -> generated models
match the DB model's MinimumSoakTimeMinutes and run CI to catch any remaining
references to the old misspelling.
- Around line 54-85: Wrap the three DB operations in Repo.Set (the UpsertPolicy
call, deleteAllRules, and insertRules) in a single DB transaction: begin a
transaction at the start of Set, obtain transactional query handles (or pass the
tx into the existing queries layer) so db.GetQueries(...).UpsertPolicy,
r.deleteAllRules and r.insertRules execute on the same tx, perform the workspace
and policy UUID parsing and set params as you do now, and if any operation
returns an error call tx.Rollback() and return the wrapped error; if all succeed
call tx.Commit() before returning nil. Ensure deleteAllRules and insertRules
accept or use the transactional query handle (or tx) rather than the
non-transactional context so all three operations are atomic.
---
Duplicate comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/environment_summary_test.go`:
- Around line 178-189: The test sets approvalPolicy.CreatedAt in this file but
the same approvalPolicy in
TestGradualRolloutEnvironmentSummaryEvaluator_WithApprovalPolicy omits
CreatedAt; update the approvalPolicy instance in that test to include CreatedAt
(e.g. use the same baseTime.Add(-1 * time.Hour).Format(time.RFC3339) or reuse
the baseTime variable) so rollout-start logic that depends on
oapi.Policy.CreatedAt sees consistent values across tests; modify the
approvalPolicy in
TestGradualRolloutEnvironmentSummaryEvaluator_WithApprovalPolicy to match the
CreatedAt used here.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/db/queries/schema.sql`:
- Around line 129-224: The migration is missing indexes on policy.workspace_id
and the policy_id FK columns in the ten rule tables which will force sequential
scans on cascade deletes; add CREATE INDEX statements for policy(workspace_id)
and for policy_id on each rule table (policy_rule_any_approval.policy_id,
policy_rule_deployment_dependency.policy_id,
policy_rule_deployment_window.policy_id,
policy_rule_environment_progression.policy_id,
policy_rule_gradual_rollout.policy_id, policy_rule_retry.policy_id,
policy_rule_rollback.policy_id, policy_rule_verification.policy_id,
policy_rule_version_cooldown.policy_id, policy_rule_version_selector.policy_id)
in this schema.sql migration so DELETE FROM policy WHERE workspace_id = ... and
workspace cascades use the indexes.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.go`:
- Line 68: Add Go doc comments for the exported mapper functions PolicyToOapi
and ToPolicyUpsertParams: prepend each function with a short sentence starting
with the function name that describes what it returns/does, include any
non-obvious details about inputs (e.g., that PolicyToOapi maps a db.Policy and
RuleRows into an oapi.Policy and how rules are represented) and note any
important behavior or TODOs; ensure comments are full sentences beginning with
the function name to satisfy godoc conventions so both PolicyToOapi and
ToPolicyUpsertParams are documented.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go`:
- Around line 96-118: Items() currently lists policies then calls loadRules for
each policy, causing an N+1 query pattern (one query in
db.ListPoliciesByWorkspaceID plus per-policy queries in loadRules). Replace the
per-policy loadRules calls with a batched rules fetch: after
db.ListPoliciesByWorkspaceID, collect all policy IDs, call a single new query
(or queries per rule type) such as ListRulesByPolicyIDs(policyIDs) or perform a
JOIN to fetch rules for all policies at once, then map rules back to their
policy IDs and call PolicyToOapi(row, rulesForRow) for each row; update or
remove loadRules to avoid per-policy queries and ensure the result map is
populated from the preloaded rules set.
In `@apps/workspace-engine/pkg/workspace/store/repository/memory/repo.go`:
- Line 432: Remove the redundant boilerplate comment preceding the Policies
method in repo.go; delete the line "// Policies implements repository.Repo." so
the Policies method stands without a comment that merely restates its signature
or purpose (locate the Policies method declaration to remove that single-line
comment).
In `@packages/db/drizzle/0148_jittery_wild_child.sql`:
- Around line 9-10: The migration adds FK/filter columns like "workspace_id" and
"policy_id" but no indexes, causing full-table scans; update the migration(s)
that define these columns (references to "workspace_id" and "policy_id" in this
diff and the other affected migrations) to add appropriate B-tree indexes (e.g.,
CREATE INDEX ...) on "workspace_id" and on "policy_id" and consider adding a
composite index that includes "created_at" for common time-ordered lookups;
ensure index names are unique and aligned with the table names being altered so
the new indexes are created as part of the same migration(s).
In `@packages/db/src/schema/policy.ts`:
- Around line 229-230: policyRuleVerification.metrics is currently typed as
unknown; add a TypeScript type annotation for the column (e.g., define or import
a MetricDefinition interface and annotate the column as MetricDefinition[]) and
apply it via the .\$type<MetricDefinition[]>() call on the jsonb("metrics")
column (mirror how policy.metadata uses .\$type<Record<string,string>>()) so the
table definition (policyRuleVerification) exposes a strongly typed metrics
field; ensure MetricDefinition is declared or imported above the table
definition and referenced in the .\$type<> generic.
- Around line 15-36: The policy schema lacks DB indexes for the foreign keys:
add an index on policy.workspaceId (workspace_id) and add indexes on each rule
table's policy_id (the FK referencing policy.id) to avoid full-table scans and
slow ON DELETE CASCADE cascades; update your migration to create e.g. an index
for policy(workspace_id) and one index per rule table (e.g.,
policy_rule_any_approval.policy_id, etc.) so deletes and workspace-scoped
queries use the indexes. Ensure the migration names are unique and reversible
(CREATE INDEX ... and DROP INDEX ...) and reference the exact table/column
identifiers used in the schema (policy, workspace_id, each rule table and
policy_id).
ℹ️ 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 (21)
apps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/policies.sql.goapps/workspace-engine/pkg/db/queries/policies.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/sqlc.yamlapps/workspace-engine/pkg/persistence/integration_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/environment_summary_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.goapps/workspace-engine/pkg/workspace/store/policy.goapps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.goapps/workspace-engine/pkg/workspace/store/repository/db/repo.goapps/workspace-engine/pkg/workspace/store/repository/interfaces.goapps/workspace-engine/pkg/workspace/store/repository/memory/repo.goapps/workspace-engine/pkg/workspace/store/repository/repo.goapps/workspace-engine/pkg/workspace/store/store.gopackages/db/drizzle/0148_jittery_wild_child.sqlpackages/db/drizzle/meta/0148_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/index.tspackages/db/src/schema/policy.ts
Comment on lines
+54
to
57
| if err := p.repo.Set(policy); err != nil { | ||
| log.Error("Failed to upsert policy", "error", err) | ||
| } | ||
| p.store.changeset.RecordUpsert(policy) |
Contributor
There was a problem hiding this comment.
Do not record changelog entries when repo writes fail.
RecordUpsert/RecordDelete still run after Set/Remove errors, which can desynchronize changelog state from the backing repo.
🧩 Minimal consistency fix
if err := p.repo.Set(policy); err != nil {
log.Error("Failed to upsert policy", "error", err)
+ return
}
p.store.changeset.RecordUpsert(policy)
...
if err := p.repo.Remove(id); err != nil {
log.Error("Failed to remove policy", "error", err)
+ return
}
p.store.changeset.RecordDelete(policy)Also applies to: 66-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/policy.go` around lines 54 - 57,
The changelog entries are recorded even when repo writes fail; update the Set
and Remove call sites so that p.store.changeset.RecordUpsert(policy) and
p.store.changeset.RecordDelete(policy) are only invoked when p.repo.Set(policy)
/ p.repo.Remove(policy) return nil—i.e., check the err and return or bail out on
error before calling RecordUpsert/RecordDelete (adjust in the blocks containing
p.repo.Set and p.repo.Remove, lines referenced around p.repo.Set,
p.store.changeset.RecordUpsert and p.repo.Remove,
p.store.changeset.RecordDelete).
apps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.go
Show resolved
Hide resolved
apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go
Show resolved
Hide resolved
apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go
Outdated
Show resolved
Hide resolved
| PolicyID: policyID, | ||
| DependsOnEnvironmentSelector: selectorToString(ep.DependsOnEnvironmentSelector), | ||
| MaximumAgeHours: optInt32ToPgint4(ep.MaximumAgeHours), | ||
| MinimumSoakTimeMinutes: optInt32ToPgint4(ep.MinimumSockTimeMinutes), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the oapi definition for this field name
rg -n 'MinimumSo[ca]kTimeMinutes' apps/workspace-engine/Repository: ctrlplanedev/ctrlplane
Length of output: 3732
Fix the typo in the OpenAPI spec: MinimumSockTimeMinutes should be MinimumSoakTimeMinutes.
The OpenAPI spec incorrectly defines this field as MinimumSockTimeMinutes ("Sock" instead of "Soak"). The DB column is minimum_soak_time_minutes and the DB model uses the correct MinimumSoakTimeMinutes. The semantic meaning (minimum soak/wait time) confirms "Soak" is the intended spelling. Update the OpenAPI specification and regenerate the corresponding code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go` at
line 239, Update the OpenAPI spec to rename the property MinimumSockTimeMinutes
to MinimumSoakTimeMinutes, regenerate the client/server code, and then update
all references in the repo layer to use the corrected field (e.g., change
ep.MinimumSockTimeMinutes to ep.MinimumSoakTimeMinutes in the code that calls
optInt32ToPgint4); ensure the OpenAPI -> generated models match the DB model's
MinimumSoakTimeMinutes and run CI to catch any remaining references to the old
misspelling.
Contributor
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/workspace-engine/test/e2e/engine_policy_approval_test.go (1)
169-176:⚠️ Potential issue | 🟡 MinorIncomplete UUID migration:
TestEngine_ApprovalPolicy_UnapprovalFlowandTestEngine_ApprovalPolicy_ApprovalDeletionstill useId: "rule-1".Every other test in this file was updated to use
uuid.New().String()forPolicyRule.Id, but these two remain with hard-coded strings. With DB-backed policy storage, a fixed rule ID across parallel test runs sharing the same DB could cause upsert collisions or stale state between tests.🔧 Proposed fix (same pattern used in the surrounding tests)
In
TestEngine_ApprovalPolicy_UnapprovalFlow(around line 168):+ rule1Id := uuid.New().String() policy.Rules = []oapi.PolicyRule{ { - Id: "rule-1", + Id: rule1Id, PolicyId: policyID,In
TestEngine_ApprovalPolicy_ApprovalDeletion(around line 764):+ rule1Id := uuid.New().String() policy.Rules = []oapi.PolicyRule{ { - Id: "rule-1", + Id: rule1Id, PolicyId: policyID,Also applies to: 765-772
🤖 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_approval_test.go` around lines 169 - 176, Replace the hard-coded PolicyRule.Id "rule-1" with a generated UUID in both tests to avoid collisions: in TestEngine_ApprovalPolicy_UnapprovalFlow and TestEngine_ApprovalPolicy_ApprovalDeletion update the PolicyRule entries that set Id to "rule-1" (the oapi.PolicyRule struct instances) to use uuid.New().String() instead; ensure the surrounding tests follow the same pattern so the import/use of uuid matches existing tests in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/workspace-engine/test/e2e/engine_policy_approval_test.go`:
- Around line 169-176: Replace the hard-coded PolicyRule.Id "rule-1" with a
generated UUID in both tests to avoid collisions: in
TestEngine_ApprovalPolicy_UnapprovalFlow and
TestEngine_ApprovalPolicy_ApprovalDeletion update the PolicyRule entries that
set Id to "rule-1" (the oapi.PolicyRule struct instances) to use
uuid.New().String() instead; ensure the surrounding tests follow the same
pattern so the import/use of uuid matches existing tests in the file.
ℹ️ 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 (8)
apps/workspace-engine/pkg/workspace/store/release_targets.goapps/workspace-engine/svc/workspaceconsumer/consumer.goapps/workspace-engine/test/e2e/engine_policy_approval_test.goapps/workspace-engine/test/e2e/engine_policy_blocking_test.goapps/workspace-engine/test/e2e/engine_policy_skip_test.goapps/workspace-engine/test/e2e/engine_policy_versionselector_test.goapps/workspace-engine/test/integration/dbtest.goapps/workspace-engine/test/integration/opts.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go (1)
50-54:⚠️ Potential issue | 🟠 MajorAvoid fail-open behavior when rule loading fails.
On rule load errors,
Get/Itemsstill return mapped policies with incomplete rules. For policy enforcement, partial loads can weaken behavior unexpectedly.✅ Safer error handling direction
rules, err := r.loadRules(uid) if err != nil { log.Warn("Failed to load rules for policy", "policy_id", id, "error", err) -} -return PolicyToOapi(row, rules), true + return nil, false +} +return PolicyToOapi(row, rules), trueFor
Items, skip entries with failedloadRules(or fail the whole call) rather than returning partial rule sets.Also applies to: 123-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go` around lines 50 - 54, The current Get/Items behavior does a soft-fail when r.loadRules(uid) returns an error and then returns a PolicyToOapi(row, rules) with incomplete rules; change this so rule load errors are not ignored: in the Get method (where r.loadRules(uid) is called) propagate the error instead of logging and returning a partial policy (i.e., return an error result/false), and in Items either skip entries whose r.loadRules(uid) fails or fail the entire Items call consistently; ensure callers of Get/Items can observe the error and that PolicyToOapi is only called with a successfully loaded rules slice.
🧹 Nitpick comments (3)
apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go (1)
16-35: Add doc comments for exported repo types/functions.Please document
RuleRows,Repo, andNewRepo.As per coding guidelines, exported functions/types/methods in
apps/workspace-engine/**/*.goshould be documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go` around lines 16 - 35, Add Go doc comments for the exported types and constructor: document the RuleRows struct with a brief description of what collection of policy rule row slices it represents, document the Repo struct explaining its role and stored fields (ctx and workspaceID), and document NewRepo describing it constructs and returns a new *Repo for the given context and workspace ID; place comments immediately above RuleRows, Repo, and NewRepo using standard Go doc comment style (// RuleRows ... , // Repo ... , // NewRepo ...).apps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.go (1)
69-69: Document exported mapper functions.Please add doc comments for
PolicyToOapiandToPolicyUpsertParams.As per coding guidelines, exported functions/types/methods in
apps/workspace-engine/**/*.goshould be documented.Also applies to: 243-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.go` at line 69, Add Go doc comments for the exported functions PolicyToOapi and ToPolicyUpsertParams: place a sentence starting with the function name (e.g., "PolicyToOapi converts ...") directly above each function, briefly describing what the function does, its key inputs (e.g., that PolicyToOapi accepts a db.Policy and RuleRows and returns an *oapi.Policy) and its return value; do the same for ToPolicyUpsertParams describing the parameters and the upsert parameter struct it produces. Ensure the comments follow Go doc style (start with the function name) and are concise one- or two-line descriptions.apps/workspace-engine/pkg/workspace/store/policy.go (1)
24-26: Add doc comment for exportedSetRepo.Please add a Go doc comment to
SetReposince it is exported.As per coding guidelines, exported functions/types/methods in
apps/workspace-engine/**/*.goshould be documented with comments that explain non-obvious context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/policy.go` around lines 24 - 26, Add a Go doc comment above the exported method Policies.SetRepo describing its purpose and parameters: explain that SetRepo assigns the provided repository.PolicyRepo implementation to the Policies instance (i.e., it sets the backing policy repository) and note any important behavior/expectations (e.g., that repo must be non-nil if applicable). Place the comment immediately above the func (Policies).SetRepo declaration and use the standard Go doc style starting with "SetRepo ..." referencing Policies and repository.PolicyRepo.
🤖 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/workspace/store/repository/db/policies/repo.go`:
- Around line 45-47: Repo methods Get and Remove fetch policies by ID only,
allowing cross-workspace access; restrict by workspace ownership by updating the
calls to enforce workspace_id: either change the underlying query used by
GetPolicyByID/DeletePolicyByID to accept workspace_id and use WHERE id=$1 AND
workspace_id=$2, or after fetching in Repo.Get/Repo.Remove verify the returned
row's WorkspaceID equals r.workspaceID and treat mismatches as not found; update
references to GetPolicyByID and any DeletePolicyByID/RemovePolicyByID calls
accordingly and return (nil, false) or no-op delete when workspace IDs differ.
- Around line 223-224: The call to parseTimestamptz currently swallows malformed
timestamps; change parseTimestamptz to return (time, error) and update call
sites in insertRulesWithQueries (and the other occurrences around the later
block at lines ~389-397) to check and return the parse error instead of using a
zero/invalid time; specifically, update the parsing call (createdAt :=
parseTimestamptz(rule.CreatedAt)) to capture (createdAt, err :=
parseTimestamptz(...)) and propagate or wrap the error from
insertRulesWithQueries so malformed CreatedAt values are surfaced to the caller.
In `@packages/db/src/schema/policy.ts`:
- Around line 29-31: Add explicit indexes for the foreign-key lookup columns:
add an index on policy.workspace_id and add a policy_id index on each
policy_rule_* table (the columns named policy_id in tables like
policy_rule_action, policy_rule_condition, etc.). Locate the workspaceId column
declaration in the policy table (workspaceId / uuid("workspace_id")...) and add
an index there, and locate each policy_rule_* table's policyId /
uuid("policy_id") column and add an index for it; name indexes consistently
(e.g., idx_policy_workspace_id and idx_policy_rule_<table>_policy_id) and use
the schema's index API (the same pattern used elsewhere in the project) so
queries filtering by workspace_id or policy_id use the index.
---
Duplicate comments:
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go`:
- Around line 50-54: The current Get/Items behavior does a soft-fail when
r.loadRules(uid) returns an error and then returns a PolicyToOapi(row, rules)
with incomplete rules; change this so rule load errors are not ignored: in the
Get method (where r.loadRules(uid) is called) propagate the error instead of
logging and returning a partial policy (i.e., return an error result/false), and
in Items either skip entries whose r.loadRules(uid) fails or fail the entire
Items call consistently; ensure callers of Get/Items can observe the error and
that PolicyToOapi is only called with a successfully loaded rules slice.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/store/policy.go`:
- Around line 24-26: Add a Go doc comment above the exported method
Policies.SetRepo describing its purpose and parameters: explain that SetRepo
assigns the provided repository.PolicyRepo implementation to the Policies
instance (i.e., it sets the backing policy repository) and note any important
behavior/expectations (e.g., that repo must be non-nil if applicable). Place the
comment immediately above the func (Policies).SetRepo declaration and use the
standard Go doc style starting with "SetRepo ..." referencing Policies and
repository.PolicyRepo.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.go`:
- Line 69: Add Go doc comments for the exported functions PolicyToOapi and
ToPolicyUpsertParams: place a sentence starting with the function name (e.g.,
"PolicyToOapi converts ...") directly above each function, briefly describing
what the function does, its key inputs (e.g., that PolicyToOapi accepts a
db.Policy and RuleRows and returns an *oapi.Policy) and its return value; do the
same for ToPolicyUpsertParams describing the parameters and the upsert parameter
struct it produces. Ensure the comments follow Go doc style (start with the
function name) and are concise one- or two-line descriptions.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go`:
- Around line 16-35: Add Go doc comments for the exported types and constructor:
document the RuleRows struct with a brief description of what collection of
policy rule row slices it represents, document the Repo struct explaining its
role and stored fields (ctx and workspaceID), and document NewRepo describing it
constructs and returns a new *Repo for the given context and workspace ID; place
comments immediately above RuleRows, Repo, and NewRepo using standard Go doc
comment style (// RuleRows ... , // Repo ... , // NewRepo ...).
ℹ️ 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 (7)
apps/workspace-engine/pkg/workspace/store/policy.goapps/workspace-engine/pkg/workspace/store/repository/db/policies/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.gopackages/db/drizzle/meta/0148_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/policy.tspackages/db/src/schema/reconcile.ts
Comment on lines
+45
to
+47
| row, err := db.GetQueries(r.ctx).GetPolicyByID(r.ctx, uid) | ||
| if err != nil { | ||
| return nil, false |
Contributor
There was a problem hiding this comment.
Enforce workspace ownership in Get/Remove (cross-tenant access risk).
Repo is workspace-scoped, but these calls are keyed only by policy ID. A known policy ID from another workspace can be read/deleted if reachable through this repository path.
🔒 Minimal guardrail in current code
func (r *Repo) Get(id string) (*oapi.Policy, bool) {
+ wsID, err := uuid.Parse(r.workspaceID)
+ if err != nil {
+ log.Warn("Failed to parse workspace id", "id", r.workspaceID, "error", err)
+ return nil, false
+ }
uid, err := uuid.Parse(id)
if err != nil {
...
}
row, err := db.GetQueries(r.ctx).GetPolicyByID(r.ctx, uid)
if err != nil {
return nil, false
}
+ if row.WorkspaceID != wsID {
+ return nil, false
+ }
...
}
func (r *Repo) Remove(id string) error {
+ wsID, err := uuid.Parse(r.workspaceID)
+ if err != nil {
+ return fmt.Errorf("parse workspace id: %w", err)
+ }
uid, err := uuid.Parse(id)
if err != nil {
return fmt.Errorf("parse id: %w", err)
}
+ row, err := db.GetQueries(r.ctx).GetPolicyByID(r.ctx, uid)
+ if err != nil {
+ return err
+ }
+ if row.WorkspaceID != wsID {
+ return nil
+ }
return db.GetQueries(r.ctx).DeletePolicy(r.ctx, uid)
}Long-term, prefer workspace-scoped SQL (...WHERE id = $1 AND workspace_id = $2) to avoid TOCTOU.
Also applies to: 97-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go`
around lines 45 - 47, Repo methods Get and Remove fetch policies by ID only,
allowing cross-workspace access; restrict by workspace ownership by updating the
calls to enforce workspace_id: either change the underlying query used by
GetPolicyByID/DeletePolicyByID to accept workspace_id and use WHERE id=$1 AND
workspace_id=$2, or after fetching in Repo.Get/Repo.Remove verify the returned
row's WorkspaceID equals r.workspaceID and treat mismatches as not found; update
references to GetPolicyByID and any DeletePolicyByID/RemovePolicyByID calls
accordingly and return (nil, false) or no-op delete when workspace IDs differ.
Comment on lines
+223
to
+224
| createdAt := parseTimestamptz(rule.CreatedAt) | ||
|
|
Contributor
There was a problem hiding this comment.
Surface invalid CreatedAt parse errors instead of silently dropping them.
parseTimestamptz currently turns malformed timestamps into zero/invalid values, which hides bad input and can mutate write behavior.
🛠️ Suggested change
-func parseTimestamptz(s string) pgtype.Timestamptz {
+func parseTimestamptz(s string) (pgtype.Timestamptz, error) {
if s == "" {
- return pgtype.Timestamptz{}
+ return pgtype.Timestamptz{}, nil
}
t, err := time.Parse(time.RFC3339, s)
if err != nil {
- return pgtype.Timestamptz{}
+ return pgtype.Timestamptz{}, fmt.Errorf("parse created_at: %w", err)
}
- return pgtype.Timestamptz{Time: t, Valid: true}
+ return pgtype.Timestamptz{Time: t, Valid: true}, nil
}Then handle the returned error at call sites in insertRulesWithQueries.
Also applies to: 389-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/policies/repo.go`
around lines 223 - 224, The call to parseTimestamptz currently swallows
malformed timestamps; change parseTimestamptz to return (time, error) and update
call sites in insertRulesWithQueries (and the other occurrences around the later
block at lines ~389-397) to check and return the parse error instead of using a
zero/invalid time; specifically, update the parsing call (createdAt :=
parseTimestamptz(rule.CreatedAt)) to capture (createdAt, err :=
parseTimestamptz(...)) and propagate or wrap the error from
insertRulesWithQueries so malformed CreatedAt values are surfaced to the caller.
Comment on lines
+29
to
+31
| workspaceId: uuid("workspace_id") | ||
| .notNull() | ||
| .references(() => workspace.id, { onDelete: "cascade" }), |
Contributor
There was a problem hiding this comment.
Add indexes for workspace/policy foreign-key lookup paths.
The current schema defines foreign keys but no explicit indexes on policy.workspace_id and each policy_rule_*.policy_id. The new repository hot paths query exactly these columns, so this will degrade quickly with table growth.
💡 Suggested schema direction
export const policy = pgTable("policy", {
...
-});
+}, (t) => [
+ index("policy_workspace_id_idx").on(t.workspaceId),
+]);
export const policyRuleAnyApproval = pgTable("policy_rule_any_approval", {
...
-});
+}, (t) => [index("policy_rule_any_approval_policy_id_idx").on(t.policyId)]);Apply the same policy_id index pattern to each policy_rule_* table.
Also applies to: 53-55, 76-78, 100-102, 127-129, 155-157, 177-179, 202-204, 224-226, 248-250, 272-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/schema/policy.ts` around lines 29 - 31, Add explicit indexes
for the foreign-key lookup columns: add an index on policy.workspace_id and add
a policy_id index on each policy_rule_* table (the columns named policy_id in
tables like policy_rule_action, policy_rule_condition, etc.). Locate the
workspaceId column declaration in the policy table (workspaceId /
uuid("workspace_id")...) and add an index there, and locate each policy_rule_*
table's policyId / uuid("policy_id") column and add an index for it; name
indexes consistently (e.g., idx_policy_workspace_id and
idx_policy_rule_<table>_policy_id) and use the schema's index API (the same
pattern used elsewhere in the project) so queries filtering by workspace_id or
policy_id use the index.
This was referenced Mar 5, 2026
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.
Mintlify
0 threads from 0 users in Mintlify
Summary by CodeRabbit
New Features
Chores
Tests