chore: grad rollouts rule go #706
Conversation
WalkthroughIntroduces gradual rollout policy evaluation functionality. Adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GradualRolloutEvaluator
participant Store
participant TimeFunc
participant HashFunc
Client->>GradualRolloutEvaluator: Evaluate(ctx, env, version, target)
GradualRolloutEvaluator->>Store: Fetch approval records
Store-->>GradualRolloutEvaluator: User approvals
GradualRolloutEvaluator->>GradualRolloutEvaluator: Compute rollout start time
alt Start time not reached
GradualRolloutEvaluator-->>Client: Pending (not started)
else Start time reached
GradualRolloutEvaluator->>Store: Fetch all targets for environment
Store-->>GradualRolloutEvaluator: Targets
loop For each target
GradualRolloutEvaluator->>HashFunc: Hash target
HashFunc-->>GradualRolloutEvaluator: Hash value
end
GradualRolloutEvaluator->>GradualRolloutEvaluator: Sort targets by hash
GradualRolloutEvaluator->>GradualRolloutEvaluator: Find current target position
GradualRolloutEvaluator->>TimeFunc: Get current time
TimeFunc-->>GradualRolloutEvaluator: Now
GradualRolloutEvaluator->>GradualRolloutEvaluator: Calculate offset from position
alt Current target reached
GradualRolloutEvaluator-->>Client: Allowed
else Target not yet reached
GradualRolloutEvaluator-->>Client: Pending (wait for offset time)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
apps/workspace-engine/oapi/openapi.json (2)
373-393: Document units and tighten bounds for timeScaleInterval.Please add a description clarifying the unit (minutes?) and intended semantics for 0. Consider enforcing minimum 1 if 0 would collapse the rollout into an immediate “all at once”.
Apply something like:
"timeScaleInterval": { - "format": "int32", - "minimum": 0, - "type": "integer" + "format": "int32", + "minimum": 1, + "type": "integer", + "description": "Time gap between successive targets (minutes). Use 1+ to stagger. If 0 is allowed, document that it yields immediate rollout to all targets." }
715-717: Clarify rule exclusivity in PolicyRule.With gradualRollout added, should PolicyRule allow multiple rule kinds at once (anyApproval/environmentProgression/gradualRollout), or be mutually exclusive? If exclusive, model with oneOf to prevent invalid combinations.
apps/workspace-engine/oapi/spec/schemas/policy.jsonnet (2)
51-52: Consider oneOf for rule exclusivity.If a PolicyRule is meant to carry only one rule kind, use an OpenAPI oneOf across anyApproval, environmentProgression, and gradualRollout to enforce it at schema level. Otherwise, document the interaction when multiple are set.
55-62: Describe units and validate lower bound for timeScaleInterval.Add a description (minutes?) and decide whether 0 is valid. If 0 is not meaningful, set minimum: 1. This will propagate to generated code and docs.
- timeScaleInterval: { type: 'integer', format: 'int32', minimum: 0 }, + timeScaleInterval: { + type: 'integer', + format: 'int32', + minimum: 1, + description: 'Time gap between successive targets (minutes).' +},apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
226-232: Propagate field docs from OpenAPI.Add descriptions for GradualRolloutRule fields (especially timeScaleInterval units) in the source OpenAPI; re-generate to include them here. This improves IDE hovers and API clarity.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
55-62: Name is verbose; consider a concise type name.EnvironmentAndVersionAndTargetScopedEvaluator is hard to read. Consider TargetScopedVersionEvaluator or EnvVersionTargetEvaluator for brevity. Interface signature is otherwise clear.
If you want a rename, I can generate a repo-wide mechanical refactor script.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go (3)
91-103: Deterministic tie-breaker for hash collisions.Extremely rare, but add a secondary key (e.g., target ID) to keep total order deterministic.
- sort.Slice(targetsWithHashes, func(i, j int) bool { - return targetsWithHashes[i].hash < targetsWithHashes[j].hash - }) + sort.Slice(targetsWithHashes, func(i, j int) bool { + if targetsWithHashes[i].hash == targetsWithHashes[j].hash { + return targetsWithHashes[i].target.Key() < targetsWithHashes[j].target.Key() + } + return targetsWithHashes[i].hash < targetsWithHashes[j].hash + })
114-116: Clarify/guard timeScaleInterval=0 behavior.If 0 means “no staggering,” document it. Otherwise, clamp to 1 to avoid surprising “all at once”.
func (e *GradualRolloutEvaluator) getDeploymentOffset(rolloutPosition int32, timeScaleInterval int32) time.Duration { - return time.Duration(rolloutPosition) * time.Duration(timeScaleInterval) * time.Minute + interval := timeScaleInterval + if interval < 1 { + interval = 1 + } + return time.Duration(rolloutPosition) * time.Duration(interval) * time.Minute }
128-143: Inject a clock for testability and consistent comparisons.Use a now() func field to avoid flakiness around boundary comparisons and to simplify unit tests.
type GradualRolloutEvaluator struct { store *store.Store rule *oapi.GradualRolloutRule hashingFn func(targetID, versionID string) uint64 + now func() time.Time } func NewGradualRolloutEvaluator(store *store.Store, rule *oapi.GradualRolloutRule) *GradualRolloutEvaluator { return &GradualRolloutEvaluator{ store: store, rule: rule, hashingFn: fnvHashingFn, + now: time.Now, } } - if time.Now().Before(*rolloutStartTime) { + if e.now().Before(*rolloutStartTime) { ... - if time.Now().Before(deploymentTime) { + if e.now().Before(deploymentTime) { ...
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/workspace-engine/oapi/openapi.json(2 hunks)apps/workspace-engine/oapi/spec/schemas/policy.jsonnet(1 hunks)apps/workspace-engine/pkg/oapi/oapi.gen.go(2 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go(1 hunks)apps/workspace-engine/pkg/workspace/store/user_approval_records.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.goapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/workspace-engine/oapi/openapi.json
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Environment(185-192)DeploymentVersion(158-168)ReleaseTarget(416-420)RuleEvaluation(472-487)
apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
UserApprovalRecord(514-521)Status(57-57)CreatedAt(49-49)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go (4)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
EnvironmentAndVersionAndTargetScopedEvaluator(55-62)apps/workspace-engine/pkg/oapi/oapi.gen.go (7)
GradualRolloutRule(227-231)Environment(185-192)DeploymentVersion(158-168)ReleaseTarget(416-420)CreatedAt(49-49)Id(51-51)RuleEvaluation(472-487)apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
UserApprovalRecords(11-14)apps/workspace-engine/pkg/workspace/releasemanager/policy/results/rule.go (4)
NewDeniedResult(24-26)NewPendingResult(17-21)ActionTypeWait(9-9)NewAllowedResult(29-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: tests
- GitHub Check: workspace-engine-tests
- GitHub Check: Typecheck
- GitHub Check: Format
- GitHub Check: Lint
🔇 Additional comments (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
340-340: PolicyRule.GradualRollout addition looks correct.JSON tag and optional pointer align with the schema, and maintain backward compatibility.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go (1)
105-112: Key() helpers are properly defined and stable in oapi package.Both
ReleaseTarget.Key()andUserApprovalRecord.Key()are implemented inpkg/oapi/oapi.go(lines 68–70 and 76–78 respectively). They use simple, deterministic string concatenation of immutable ID fields with no state-dependent logic, making them stable across process restarts. The codebase uses them consistently throughout for map keying, caching, and comparisons, confirming their reliability.apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
60-71: RFC3339 string comparison is safe and appropriate for the current implementation.The codebase consistently uses
time.Now().Format(time.RFC3339)for all CreatedAt assignments. RFC3339 timestamps are inherently lexicographically sortable by design (ISO 8601 compliance), so the string comparison inGetApprovalRecords()is correct and efficient. The suggested refactor would add unnecessary overhead by parsing timestamps on every comparison without providing actual safety gains. No code paths exist that set CreatedAt without RFC3339 formatting.
| var fnvHashingFn = func(targetID, versionID string) uint64 { | ||
| h := fnv.New64a() | ||
| h.Write([]byte(targetID + versionID)) | ||
| return h.Sum64() | ||
| } |
There was a problem hiding this comment.
Avoid accidental concatenation collisions; add a delimiter.
targetID + versionID can collide for pairs like ("ab","c") vs ("a","bc"). Add a separator to be safe.
- h.Write([]byte(targetID + versionID))
+ h.Write([]byte(targetID))
+ h.Write([]byte{0}) // delimiter
+ h.Write([]byte(versionID))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var fnvHashingFn = func(targetID, versionID string) uint64 { | |
| h := fnv.New64a() | |
| h.Write([]byte(targetID + versionID)) | |
| return h.Sum64() | |
| } | |
| var fnvHashingFn = func(targetID, versionID string) uint64 { | |
| h := fnv.New64a() | |
| h.Write([]byte(targetID)) | |
| h.Write([]byte{0}) // delimiter | |
| h.Write([]byte(versionID)) | |
| return h.Sum64() | |
| } |
🤖 Prompt for AI Agents
In
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go
around lines 18 to 22, the current hash concatenates targetID and versionID
directly which can cause collisions (e.g., "ab"+"c" == "a"+"bc"); change the
hashing input to include an explicit delimiter between the IDs (use a character
unlikely to appear in IDs such as '\x00' or a fixed separator like ':'), e.g.,
write targetID, then the delimiter, then versionID into the FNV hasher so the
combined input is unambiguous and collision-resistant.
...kspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go
Show resolved
Hide resolved
...kspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go
Show resolved
Hide resolved
adityachoudhari26
force-pushed
the
gradual-rollouts-go
branch
from
b827227 to
f7e15fd
Compare
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go (3)
18-22: Add a delimiter in hashing to avoid accidental collisions.Concatenation can collide for ("ab","c") vs ("a","bc"). Use an explicit separator.
var fnvHashingFn = func(targetID, versionID string) uint64 { h := fnv.New64a() - h.Write([]byte(targetID + versionID)) + h.Write([]byte(targetID)) + h.Write([]byte{0}) // delimiter + h.Write([]byte(versionID)) return h.Sum64() }
46-52: Skip disabled policies when computing required approvals.Disabled policies shouldn’t gate rollout start.
- for _, policy := range policiesForTarget { + for _, policy := range policiesForTarget { + if !policy.Enabled { + continue + } for _, rule := range policy.Rules { if rule.AnyApproval != nil && rule.AnyApproval.MinApprovals > maxMinApprovals { maxMinApprovals = rule.AnyApproval.MinApprovals } } }
124-131: Return Pending(ActionTypeApproval) instead of Denied when approvals not met or not started.“Denied” implies irrecoverable; here we’re waiting on approvals or time.
- if rolloutStartTime == nil { - return results.NewDeniedResult("Rollout has not started yet"), nil - } + if rolloutStartTime == nil { + return results.NewPendingResult(results.ActionTypeApproval, "Waiting for required approvals to start rollout"), nil + } if time.Now().Before(*rolloutStartTime) { return results.NewPendingResult(results.ActionTypeWait, "Rollout has not started yet"), nil }#!/bin/bash # Verify ActionTypeApproval constant exists rg -nC2 'ActionType.*Approval|Approval.*ActionType' apps/workspace-engine/pkg/workspace/releasemanager/policy -g '!**/test/**'
🧹 Nitpick comments (4)
apps/workspace-engine/oapi/spec/schemas/policy.jsonnet (1)
55-71: Document units and semantics for timeScaleInterval; consider minimum.Clarify that timeScaleInterval is in minutes and whether 0 means “no delay/simultaneous.” If 0 is valid, document it; otherwise raise minimum to 1.
Apply this doc-only diff (safe either way):
- timeScaleInterval: { type: 'integer', format: 'int32', minimum: 0 }, + timeScaleInterval: { + type: 'integer', format: 'int32', minimum: 0, + description: 'Minutes to wait between successive rollout targets; 0 means simultaneous rollout across targets', + },Do we intend to allow 0? If not, set minimum to 1 and update description accordingly.
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
55-62: Add a doc comment to match surrounding interfaces and clarify scope.Keep consistency and help readers understand when to use this evaluator.
- type EnvironmentAndVersionAndTargetScopedEvaluator interface { +// EnvironmentAndVersionAndTargetScopedEvaluator evaluates rules that need +// environment, version, and release-target context (e.g., gradual rollouts). +type EnvironmentAndVersionAndTargetScopedEvaluator interface {apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go (2)
100-103: Deterministic secondary key on hash ties.Add a stable tie‑breaker to keep ordering deterministic even on rare hash collisions.
- sort.Slice(targetsWithHashes, func(i, j int) bool { - return targetsWithHashes[i].hash < targetsWithHashes[j].hash - }) + sort.SliceStable(targetsWithHashes, func(i, j int) bool { + if targetsWithHashes[i].hash == targetsWithHashes[j].hash { + return targetsWithHashes[i].target.Key() < targetsWithHashes[j].target.Key() + } + return targetsWithHashes[i].hash < targetsWithHashes[j].hash + })
137-145: Optional: inject a clock for testability and deterministic evaluations.Allows unit tests to control “now,” and supports future time‑source abstractions.
@@ type GradualRolloutEvaluator struct { store *store.Store rule *oapi.GradualRolloutRule hashingFn func(targetID, versionID string) uint64 + now func() time.Time } @@ func NewGradualRolloutEvaluator(store *store.Store, rule *oapi.GradualRolloutRule) *GradualRolloutEvaluator { return &GradualRolloutEvaluator{ store: store, rule: rule, hashingFn: fnvHashingFn, + now: time.Now, } } @@ - if time.Now().Before(*rolloutStartTime) { + if e.now().Before(*rolloutStartTime) { return results.NewPendingResult(results.ActionTypeWait, "Rollout has not started yet"), nil } @@ - if time.Now().Before(deploymentTime) { + if e.now().Before(deploymentTime) { reason := fmt.Sprintf("Rollout will start at %s for this release target", deploymentTime.Format(time.RFC3339)) return results.NewPendingResult(results.ActionTypeWait, reason), nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/workspace-engine/oapi/openapi.json(2 hunks)apps/workspace-engine/oapi/spec/schemas/policy.jsonnet(1 hunks)apps/workspace-engine/pkg/oapi/oapi.gen.go(2 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go(1 hunks)apps/workspace-engine/pkg/workspace/store/user_approval_records.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/workspace-engine/oapi/openapi.json
- apps/workspace-engine/pkg/oapi/oapi.gen.go
🧰 Additional context used
📓 Path-based instructions (1)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradual_rollout.go (4)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
EnvironmentAndVersionAndTargetScopedEvaluator(55-62)apps/workspace-engine/pkg/oapi/oapi.gen.go (7)
GradualRolloutRule(227-231)Environment(185-192)DeploymentVersion(158-168)ReleaseTarget(416-420)CreatedAt(49-49)Id(51-51)RuleEvaluation(472-487)apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
UserApprovalRecords(11-14)apps/workspace-engine/pkg/workspace/releasemanager/policy/results/rule.go (4)
NewDeniedResult(24-26)NewPendingResult(17-21)ActionTypeWait(9-9)NewAllowedResult(29-33)
apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
UserApprovalRecord(514-521)Status(57-57)ApprovalStatusApproved(19-19)CreatedAt(49-49)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Environment(185-192)DeploymentVersion(158-168)ReleaseTarget(416-420)RuleEvaluation(472-487)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Typecheck
- GitHub Check: Format
- GitHub Check: Lint
- GitHub Check: tests
- GitHub Check: workspace-engine-tests
🔇 Additional comments (1)
apps/workspace-engine/oapi/spec/schemas/policy.jsonnet (1)
51-53: LGTM on adding optional gradualRollout to PolicyRule.No issues; compatible with existing rule set.
|
|
||
| import ( | ||
| "context" | ||
| "sort" |
There was a problem hiding this comment.
🧩 Analysis chain
Sort by parsed RFC3339 time, not string, to avoid mis‑ordering across time zones.
String compare can produce incorrect chronology when offsets differ, which shifts the computed rollout start time.
Apply this diff:
@@
-import (
+import (
"context"
- "sort"
+ "sort"
+ "time"
"workspace-engine/pkg/changeset"
"workspace-engine/pkg/oapi"
"workspace-engine/pkg/workspace/store/repository"
)
@@
func (u *UserApprovalRecords) GetApprovalRecords(versionId, environmentId string) []*oapi.UserApprovalRecord {
records := make([]*oapi.UserApprovalRecord, 0)
for record := range u.repo.UserApprovalRecords.IterBuffered() {
if record.Val.VersionId == versionId && record.Val.EnvironmentId == environmentId && record.Val.Status == oapi.ApprovalStatusApproved {
records = append(records, record.Val)
}
}
- sort.Slice(records, func(i, j int) bool {
- return records[i].CreatedAt < records[j].CreatedAt
- })
+ sort.Slice(records, func(i, j int) bool {
+ ti, ei := time.Parse(time.RFC3339, records[i].CreatedAt)
+ tj, ej := time.Parse(time.RFC3339, records[j].CreatedAt)
+ if ei == nil && ej == nil {
+ return ti.Before(tj)
+ }
+ // Fallback to string compare if parse fails
+ return records[i].CreatedAt < records[j].CreatedAt
+ })
return records
}If CreatedAt is guaranteed RFC3339-Z everywhere, we can drop the fallback and sort directly by time. Otherwise, consider updating the schema to set format: date-time on UserApprovalRecord.createdAt for consistency.
Also applies to: 60-71
🏁 Script executed:
#!/bin/bash
# Examine the actual file and lines in question
echo "=== File content around lines 1-80 ==="
cat apps/workspace-engine/pkg/workspace/store/user_approval_records.go | head -80
echo -e "\n=== Check for CreatedAt field usage in file ==="
rg -n "CreatedAt" apps/workspace-engine/pkg/workspace/store/user_approval_records.go -C3Length of output: 2454
🏁 Script executed:
#!/bin/bash
# Check the UserApprovalRecord schema/definition
echo "=== Find UserApprovalRecord definition ==="
rg -n "type UserApprovalRecord" apps/workspace-engine/ -A10 --type=go
echo -e "\n=== Check oapi definitions ==="
fd "oapi" apps/workspace-engine/ --type=go | head -5Length of output: 2144
🏁 Script executed:
#!/bin/bash
# Check how CreatedAt is set/populated
echo "=== How CreatedAt is set ==="
rg -n "CreatedAt.*=" apps/workspace-engine/pkg/workspace/store/ --type=go -B2 -A2 | head -50
echo -e "\n=== Check for RFC3339 usage in codebase ==="
rg -n "RFC3339|time.Parse|time.Time" apps/workspace-engine/pkg/workspace/store/ --type=go | head -20Length of output: 1205
🏁 Script executed:
#!/bin/bash
# Check the environment-version-rollout rule mentioned in learnings
echo "=== Check rollout_start_time.go for similar sorting ==="
cat apps/workspace-engine/pkg/engine/policy/rules/environment-version-rollout/rollout_start_time.go | grep -A20 -B5 "sort\|CreatedAt\|ApprovedAt" | head -60Length of output: 183
🏁 Script executed:
#!/bin/bash
# Find where UserApprovalRecord is created/populated with CreatedAt
echo "=== Search for UserApprovalRecord creation/population ==="
rg -n "UserApprovalRecord\s*\{" apps/workspace-engine/ --type=go -A10 | head -80
echo -e "\n=== Search for .CreatedAt assignments involving UserApprovalRecord ==="
rg -n "userApprovalRecord.*CreatedAt|UserApprovalRecord.*CreatedAt" apps/workspace-engine/ --type=go -B3 -A3Length of output: 8995
🏁 Script executed:
#!/bin/bash
# Check RFC3339 string comparison behavior in theory
# If timestamps are RFC3339, check if string comparison works correctly
echo "=== Check if RFC3339 format allows lexicographic sorting ==="
python3 << 'EOF'
# RFC3339 format: 2006-01-02T15:04:05-07:00
# Test if string comparison works for different timezone offsets
test_cases = [
("2025-01-01T12:00:00+00:00", "2025-01-01T12:00:00+01:00", "Same UTC, different offset"),
("2025-01-01T12:00:00Z", "2025-01-01T13:00:00+01:00", "Different UTC times, both valid RFC3339"),
("2025-01-01T11:00:00-05:00", "2025-01-01T16:00:00Z", "Different representations of same instant"),
]
for t1, t2, desc in test_cases:
print(f"{desc}")
print(f" t1: {t1}, t2: {t2}")
print(f" String comparison (t1 < t2): {t1 < t2}")
print(f" Expected chronological order (actual UTC times): 2025-01-01T12:00:00Z is same as both")
print()
EOFLength of output: 723
🏁 Script executed:
#!/bin/bash
# Find where userApprovalRecord records are created in the actual flow
echo "=== Search for approval record creation flow ==="
rg -n "new.*UserApprovalRecord\|&oapi\.UserApprovalRecord" apps/workspace-engine/ --type=go -B5 -A5 | head -100Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check the format of CreatedAt when it's set - look at handlers or API endpoints
echo "=== Check for time formatting in approval-related handlers ==="
rg -n "time.Now\(\).Format\|CreatedAt.*time\|approval" apps/workspace-engine/ --type=go -i | grep -i "approval\|created" | head -30Length of output: 62
🏁 Script executed:
#!/bin/bash
# Find the handler for UserApprovalRecordCreate event
echo "=== Search for UserApprovalRecordCreate handler ==="
rg -n "UserApprovalRecordCreate" apps/workspace-engine/ --type=go -B5 -A10 | head -100
echo -e "\n=== Search in handler directory ==="
fd "handler" apps/workspace-engine/ -type f | head -20Length of output: 9497
🏁 Script executed:
#!/bin/bash
# Look for where UserApprovalRecord is populated initially
echo "=== Search for assignment to CreatedAt in handlers or event processing ==="
rg -n "handler\." apps/workspace-engine/ --type=go | grep -i "approval" | head -20Length of output: 2691
🏁 Script executed:
#!/bin/bash
# Check if there's an API endpoint or event handler that creates approval records
echo "=== Search for event handlers directory ==="
find apps/workspace-engine -type d -name "*handler*" | head -10
echo -e "\n=== Check for CreateUserApprovalRecord or similar ==="
rg -n "CreateUserApprovalRecord\|user.*approval\|approval.*event" apps/workspace-engine/ --type=go -i | head -30Length of output: 139
🏁 Script executed:
#!/bin/bash
# Look at the database schema to understand CreatedAt type/default
echo "=== Search for SQL/migration files with CreatedAt ==="
find apps/workspace-engine -type f \( -name "*.sql" -o -name "*.go" \) | xargs grep -l "user_approval" 2>/dev/null | head -10
echo -e "\n=== Check db.go or schema-related files ==="
rg -n "USER_APPROVAL_RECORD" apps/workspace-engine/pkg/db/ --type=go -B2 -A5 | head -50Length of output: 4285
🏁 Script executed:
#!/bin/bash
# Examine the event handler implementation
echo "=== Find UserApprovalRecordCreate handler constant/registration ==="
cat apps/workspace-engine/pkg/events/handler/constants.go 2>/dev/null || \
cat apps/workspace-engine/pkg/events/handler/*.go | head -100
echo -e "\n=== List handler files ==="
ls -la apps/workspace-engine/pkg/events/handler/Length of output: 4953
🏁 Script executed:
#!/bin/bash
# Search for where CreatedAt is set with time.Now()
echo "=== Search for where timestamp is created/set for CreatedAt ==="
rg -n "time\.Now\(\)" apps/workspace-engine/pkg/events/ --type=go -B3 -A3
echo -e "\n=== Search in oapi persistence for CreatedAt ==="
rg -n "CreatedAt" apps/workspace-engine/pkg/oapi/persistence.go -B3 -A3Length of output: 4011
🏁 Script executed:
#!/bin/bash
# Look for database migrations to see if created_at has DEFAULT
echo "=== Search for SQL migration files ==="
find apps/workspace-engine -type f -name "*.sql" | head -10
echo -e "\n=== Check if migrations exist ==="
find apps/workspace-engine -type d -name "migrations" -o -name "schema" | head -5Length of output: 73
🏁 Script executed:
#!/bin/bash
# Check where PushEvent stores the approval record
echo "=== Find PushEvent implementation ==="
rg -n "func.*PushEvent\|UserApprovalRecordCreate" apps/workspace-engine/pkg/events/ --type=go -B2 -A8 | head -80Length of output: 37
🏁 Script executed:
#!/bin/bash
# Find where the approval record's created_at comes from - look in main handler logic
echo "=== Look for approval record processing/storage ==="
rg -n "UserApprovalRecords.*Upsert\|approval.*Upsert" apps/workspace-engine/ --type=go -B5 -A5 | head -100Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look in the userapprovalrecords handler directory
echo "=== Check userapprovalrecords handler files ==="
ls -la apps/workspace-engine/pkg/events/handler/userapprovalrecords/
echo -e "\n=== Content of userapprovalrecords handler ==="
cat apps/workspace-engine/pkg/events/handler/userapprovalrecords/*.goLength of output: 1459
🏁 Script executed:
#!/bin/bash
# Find where createdAt timestamp is set when creating approval record
echo "=== Search for JSON unmarshaling or event processing of UserApprovalRecord ==="
rg -n "json.Unmarshal\|UserApprovalRecord" apps/workspace-engine/pkg/events/handler/userapprovalrecords/ --type=go -B5 -A10
echo -e "\n=== Check how event data flows to Upsert ==="
rg -n "Upsert.*UserApprovalRecord\|\.Upsert.*approval" apps/workspace-engine/ --type=go -B3 -A3 | head -50Length of output: 124
🏁 Script executed:
#!/bin/bash
# Look for where CreatedAt is set or defaulted
echo "=== Find all .CreatedAt assignments ==="
rg -n "\.CreatedAt\s*=" apps/workspace-engine/ --type=go | grep -v test | head -30
echo -e "\n=== Check if database generates timestamps ==="
rg -n "CURRENT_TIMESTAMP\|DEFAULT\|now\(\)" apps/workspace-engine/ --type=go -i | head -20Length of output: 948
🏁 Script executed:
#!/bin/bash
# Check the actual Go struct field tags to see if database handles it
echo "=== Check UserApprovalRecord struct in oapi.gen.go ==="
sed -n '514,530p' apps/workspace-engine/pkg/oapi/oapi.gen.go
echo -e "\n=== See more of UserApprovalRecord context ==="
sed -n '500,550p' apps/workspace-engine/pkg/oapi/oapi.gen.goLength of output: 2344
🏁 Script executed:
#!/bin/bash
# Final verification: Check if CreatedAt could come from API with different timezones
echo "=== Search for API endpoint that creates approval records ==="
rg -n "approval\|Approval" apps/workspace-engine/ --type=go | grep -i "post\|create\|endpoint\|api" | head -20
echo -e "\n=== Check if there's timestamp normalization anywhere ==="
rg -n "RFC3339\|Format.*time\|Parse.*time" apps/workspace-engine/ --type=go | grep -v test | head -20Length of output: 120
🏁 Script executed:
#!/bin/bash
# Verify the fix is syntactically sound
echo "=== Verify RFC3339 parsing logic ==="
python3 << 'EOF'
import time as time_module
# Test the proposed fix logic
test_timestamps = [
"2025-01-01T12:00:00+00:00", # UTC
"2025-01-01T12:00:00Z", # UTC with Z
"2025-01-01T12:00:00-05:00", # EST
"2025-01-01T12:00:00+01:00", # CET
"invalid-timestamp", # Invalid
]
print("Testing RFC3339 parsing correctness:")
for ts in test_timestamps:
try:
# Simulate Go's time.Parse behavior
from datetime import datetime
# Python's fromisoformat handles most RFC3339
dt = datetime.fromisoformat(ts.replace('Z', '+00:00'))
print(f" ✓ '{ts}' → parses to UTC: {dt}")
except Exception as e:
print(f" ✗ '{ts}' → parse error (fallback to string compare)")
print("\nString comparison vs parsed time comparison:")
t1 = "2025-01-01T12:00:00Z"
t2 = "2025-01-01T12:00:00+01:00"
print(f" String: '{t1}' < '{t2}' = {t1 < t2} (WRONG - represent same instant)")
print(f" Parsed would show both as same time correctly")
EOFLength of output: 633
Sort CreatedAt by parsed time, not string, to handle RFC3339 timestamps with varying timezone offsets.
String comparison of RFC3339 timestamps produces incorrect ordering when different timezone offsets are used, affecting the rollout start time calculation. For example, "2025-01-01T12:00:00Z" and "2025-01-01T12:00:00+01:00" represent the same instant but string comparison treats them incorrectly.
Apply the suggested diff to parse RFC3339 strings before comparing:
@@
-import (
+import (
"context"
- "sort"
+ "sort"
+ "time"
"workspace-engine/pkg/changeset"
"workspace-engine/pkg/oapi"
"workspace-engine/pkg/workspace/store/repository"
)
@@
func (u *UserApprovalRecords) GetApprovalRecords(versionId, environmentId string) []*oapi.UserApprovalRecord {
records := make([]*oapi.UserApprovalRecord, 0)
for record := range u.repo.UserApprovalRecords.IterBuffered() {
if record.Val.VersionId == versionId && record.Val.EnvironmentId == environmentId && record.Val.Status == oapi.ApprovalStatusApproved {
records = append(records, record.Val)
}
}
- sort.Slice(records, func(i, j int) bool {
- return records[i].CreatedAt < records[j].CreatedAt
- })
+ sort.Slice(records, func(i, j int) bool {
+ ti, ei := time.Parse(time.RFC3339, records[i].CreatedAt)
+ tj, ej := time.Parse(time.RFC3339, records[j].CreatedAt)
+ if ei == nil && ej == nil {
+ return ti.Before(tj)
+ }
+ // Fallback to string compare if parse fails
+ return records[i].CreatedAt < records[j].CreatedAt
+ })
return records
}The fix includes a defensive fallback for malformed timestamps while ensuring correct chronological ordering across timezone offsets.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/store/user_approval_records.go around
line 5, replace the current string-based sort of CreatedAt with a time-aware
comparator: parse each record's CreatedAt using time.Parse(time.RFC3339) (import
time if not present), compare the parsed times with t1.Before(t2) to determine
order, and include a defensive fallback when parsing fails (e.g., treat
unparseable timestamps as zero time or fall back to string comparison) so
malformed values don't panic and RFC3339 timestamps with different timezone
offsets sort correctly; ensure the sort import remains.
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (1)
136-138: Clarify action type when approvals are not satisfied.When
rolloutStartTimeis nil, it indicates that approval requirements haven't been met. The current implementation returnsActionTypeWaitwith message "Rollout has not started yet", but a more specific action type would better indicate that user approvals are blocking progress. Consider whether a distinct action type for approval-related waits would improve clarity for callers.</comment_end -->
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go (1)
99-259: Consider extracting common test setup.Lines 105-129, 186-210, 267-291, and 375-401 share nearly identical environment, deployment, resource, and evaluator setup. Extracting a helper like
setupTestEvaluator(t, systemID, numResources, timeScaleInterval, currentTime)would reduce duplication and make test intent clearer.</comment_end -->
Also applies to: 261-493
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go(1 hunks)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#658
File: apps/workspace-engine/pkg/engine/policy/rules/environment-version-rollout/rollout_start_time.go:65-80
Timestamp: 2025-08-19T22:58:15.170Z
Learning: In the environment-version-rollout rule's rollout start time logic, the code intentionally returns the latest ApprovedAt time (first non-nil record from records ordered by updatedAt) rather than the earliest. This is because the goal is to find the latest approval that caused all the rules to pass. While the complete logic for determining "the latest that caused all rules to pass" would be too complex, this approximation works well given that the UI prevents approving if all rules are already passing.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
📚 Learning: 2025-08-19T22:58:15.170Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#658
File: apps/workspace-engine/pkg/engine/policy/rules/environment-version-rollout/rollout_start_time.go:65-80
Timestamp: 2025-08-19T22:58:15.170Z
Learning: In the environment-version-rollout rule's rollout start time logic, the code intentionally returns the latest ApprovedAt time (first non-nil record from records ordered by updatedAt) rather than the earliest. This is because the goal is to find the latest approval that caused all the rules to pass. While the complete logic for determining "the latest that caused all rules to pass" would be too complex, this approximation works well given that the UI prevents approving if all rules are already passing.
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*_test.go : Follow the existing test structure used in *_test.go files
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
🧬 Code graph analysis (2)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (4)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/types.go (1)
EnvironmentAndVersionAndTargetScopedEvaluator(55-62)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
GradualRolloutRule(227-231)Id(51-51)RuleEvaluation(472-487)apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
UserApprovalRecords(11-14)apps/workspace-engine/pkg/workspace/releasemanager/policy/results/rule.go (4)
NewPendingResult(17-21)ActionTypeWait(9-9)NewDeniedResult(24-26)NewAllowedResult(29-33)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go (4)
apps/workspace-engine/pkg/oapi/oapi.gen.go (11)
JsonSelector(292-294)Id(51-51)CreatedAt(49-49)GradualRolloutRule(227-231)Wait(92-92)PolicyTargetSelector(346-351)PolicyRule(336-343)AnyApprovalRule(96-98)UserApprovalRecord(514-521)Status(57-57)ApprovalStatusApproved(19-19)apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (1)
GradualRolloutEvaluator(24-29)apps/workspace-engine/pkg/workspace/releasemanager/policy/results/rule.go (1)
ActionType(5-5)apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
UserApprovalRecords(11-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: workspace-engine-tests
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: tests
🔇 Additional comments (2)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go (1)
99-493: Comprehensive test coverage!The four test cases effectively validate the gradual rollout logic:
- Linear progression with time-based intervals
- Immediate rollout when interval is zero
- Blocking when approval requirements aren't met
- Progression gated by approval timestamps
The assertions correctly verify position-based deployment timing and approval-based start time calculation.
</comment_end -->
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go (1)
65-76: ****The sorting is not implicit.
GetApprovalRecords(inuser_approval_records.go:67-69) explicitly sorts records byCreatedAtin ascending order usingsort.Slice. The code at line 70 correctly accessesapprovalRecords[maxMinApprovals-1]to get the first record satisfying the minimum requirement. No documentation or verification is needed—the contract is clear from the implementation.Likely an incorrect or invalid review comment.
| rolloutPosition, err := e.getRolloutPositionForTarget(ctx, environment, version, releaseTarget) | ||
| if err != nil { | ||
| return results.NewDeniedResult("Failed to get rollout position"), err | ||
| } |
There was a problem hiding this comment.
Reconsider error handling pattern.
Returning both a Denied result and an error creates ambiguity for callers—should they check the error or inspect the result? Typically, when an error occurs during evaluation, you'd return (nil, error) to signal that evaluation couldn't complete, reserving non-nil results for successful evaluations only.
Apply this pattern:
rolloutPosition, err := e.getRolloutPositionForTarget(ctx, environment, version, releaseTarget)
if err != nil {
- return results.NewDeniedResult("Failed to get rollout position"), err
+ return nil, fmt.Errorf("failed to get rollout position: %w", err)
}</comment_end -->
🤖 Prompt for AI Agents
In
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout.go
around lines 144–147, the code currently returns a Denied result plus an error
when getRolloutPositionForTarget fails; change this to return (nil, err) so
callers can rely on the error to indicate failure and non-nil results only for
successful evaluations. Concretely, remove creation/return of
results.NewDeniedResult on error and return nil with the propagated error;
ensure any upstream callers expect a nil result when an error is returned
(adjust tests or call sites if they currently inspect the result instead of the
error).
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
Summary by CodeRabbit
New Features
Tests