feat: add cron expression validation to manifest validator#2149
feat: add cron expression validation to manifest validator#2149
Conversation
Enhances validateScheduledTriggers to parse and validate inline cron expressions from scheduled:<name>:<cron-expr> triggers. Enforces a 15-minute minimum interval, 20-schedule-per-manifest cap, and warns on schedules that fire less frequently than once per 365 days.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughEnhanced validation for scheduled saga triggers in the control plane domain validator. Added checks for: tenant-wide scheduled trigger count limits, cron expression parsing and validity, minimum interval enforcement between cron occurrences, and warnings for infrequent schedules. Includes new helper function and validation method plus comprehensive test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/control-plane/internal/validator/domain_validator.go (1)
398-414:⚠️ Potential issue | 🟡 MinorReject an explicit empty cron suffix.
Line 399 collapses
scheduled:<name>andscheduled:<name>:into the same(name, "")shape, and Line 412 treats both as “no inline cron provided.” That silently accepts a malformed trigger instead of surfacing a validation error. Preserve whether the second separator was present and fail when it is present but empty.💡 One way to preserve that distinction
- name, cronExpr := parsScheduledTrigger(remainder) + name, cronExpr, hasCron := parseScheduledTrigger(remainder) if firstIdx, exists := seen[name]; exists { addError(result, ValidationError{ Severity: SeverityError, Path: fmt.Sprintf("sagas[%d].trigger", i), @@ - if cronExpr != "" { + if hasCron { + if cronExpr == "" { + addError(result, ValidationError{ + Severity: SeverityError, + Path: fmt.Sprintf("sagas[%d].trigger", i), + Code: "INVALID_CRON_EXPRESSION", + Message: "cron expression cannot be empty", + ResourceType: "saga", + ResourceID: saga.GetName(), + }) + continue + } v.validateCronExpression(cronExpr, fmt.Sprintf("sagas[%d].trigger", i), saga.GetName(), result) } } } -// parsScheduledTrigger splits "name" or "name:cron expr" into (name, cronExpr). +// parseScheduledTrigger splits "name" or "name:cron expr" into (name, cronExpr, hasCron). // The cron expression may contain spaces, so only the first colon is used as separator. -func parsScheduledTrigger(remainder string) (name, cronExpr string) { +func parseScheduledTrigger(remainder string) (name, cronExpr string, hasCron bool) { idx := strings.Index(remainder, ":") if idx < 0 { - return remainder, "" + return remainder, "", false } - return remainder[:idx], remainder[idx+1:] + return remainder[:idx], remainder[idx+1:], true }Also applies to: 427-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/validator/domain_validator.go` around lines 398 - 414, parsScheduledTrigger currently collapses "scheduled:name" and "scheduled:name:" into the same (name, "") so a trailing colon (explicit empty cron) is silently accepted; update the validation after calling parsScheduledTrigger to detect when the trigger had a separator but an empty cron (you can change parsScheduledTrigger to return a third bool like separatorPresent or have it return nil vs "" differently), and when separatorPresent && cronExpr == "" call addError with a ValidationError (SeverityError, Path fmt.Sprintf("sagas[%d].trigger", i), Code like "EMPTY_SCHEDULED_CRON", and a helpful Message) instead of calling validateCronExpression; keep the existing duplicate-name check using seen and only call validateCronExpression when cronExpr != "" and not explicitly empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/control-plane/internal/validator/domain_validator.go`:
- Around line 452-467: The current check uses a single sampled gap from now
(using schedule.Next) which can miss shorter intervals; change the logic in
domain_validator.go so that instead of computing only t1 := schedule.Next(now)
and t2 := schedule.Next(t1), you pick a fixed anchor (e.g., a constant UTC time)
and iterate schedule.Next repeatedly to collect many consecutive occurrences,
compute the minimum difference between any adjacent occurrences, and compare
that minimum to minCronInterval; update the code paths that call
addError/ValidationError (using expr, sagaName, path, result) to report when
this computed minimum is < minCronInterval.
---
Outside diff comments:
In `@services/control-plane/internal/validator/domain_validator.go`:
- Around line 398-414: parsScheduledTrigger currently collapses "scheduled:name"
and "scheduled:name:" into the same (name, "") so a trailing colon (explicit
empty cron) is silently accepted; update the validation after calling
parsScheduledTrigger to detect when the trigger had a separator but an empty
cron (you can change parsScheduledTrigger to return a third bool like
separatorPresent or have it return nil vs "" differently), and when
separatorPresent && cronExpr == "" call addError with a ValidationError
(SeverityError, Path fmt.Sprintf("sagas[%d].trigger", i), Code like
"EMPTY_SCHEDULED_CRON", and a helpful Message) instead of calling
validateCronExpression; keep the existing duplicate-name check using seen and
only call validateCronExpression when cronExpr != "" and not explicitly empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65ad5fa1-abd2-4a6b-9d98-83c697cacde7
📒 Files selected for processing (2)
services/control-plane/internal/validator/domain_validator.goservices/control-plane/internal/validator/domain_validator_test.go
Claude Code ReviewCommit: SummaryClean, well-scoped addition of cron expression validation to the manifest validator. The second commit addressed all prior review feedback: interval sampling now uses a fixed anchor with 10 consecutive samples (catching non-uniform gaps), and the function name typo is fixed. Test coverage is thorough — 7 new test cases covering valid expressions, invalid syntax, interval floors, the cap limit, empty-cron edge case, and the infrequent-schedule warning. No domain-level concerns. Risk Assessment
FindingsNo open findings. Previously Flagged
Bot Review Notes
|
- Rename parsScheduledTrigger -> parseScheduledTrigger (typo) - Return hasCron bool to distinguish scheduled:name from scheduled:name: so an explicit empty cron suffix is rejected with INVALID_CRON_EXPRESSION - Use fixed anchor and 10-sample minimum for interval check instead of sampling only 2 occurrences from now, avoiding time-dependent results
bjcoombs
dismissed
coderabbitai[bot]’s stale review
Addressed both issues: empty cron suffix now errors, interval check uses fixed anchor with 10-sample minimum
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary
validateScheduledTriggersto parse inline cron expressions fromscheduled:<name>:<cron-expr>triggers usingrobfig/cron/v3(already a dependency)Changes Made
services/control-plane/internal/validator/domain_validator.gomaxScheduledTriggersPerTenant,minCronInterval,warnCronIntervalconstantscronParserpackage-level var usingcron.NewParser(Minute|Hour|Dom|Month|Dow)to match schedulerparsScheduledTriggerto splitname:cron-exprby first colonvalidateCronExpressionmethod covering syntax check + interval checksvalidateScheduledTriggersto invoke new validations and enforce the 20-schedule capTesting
All 9 new tests pass alongside the existing test suite:
TestValidateScheduledTriggers_ValidCronExpression- valid hourly cron passesTestValidateScheduledTriggers_NoCronExpression_Valid-scheduled:<name>without cron is validTestValidateScheduledTriggers_InvalidCronSyntax- invalid cron expression errors withINVALID_CRON_EXPRESSIONTestValidateScheduledTriggers_IntervalTooShort- every-minute cron errors withCRON_INTERVAL_TOO_SHORTTestValidateScheduledTriggers_IntervalExactly15Minutes_Valid- exactly 15-minute interval passesTestValidateScheduledTriggers_TooManySchedules- 21 schedules errors withTOO_MANY_SCHEDULED_TRIGGERSTestValidateScheduledTriggers_InfrequentScheduleWarns- annual cron warns withCRON_VERY_INFREQUENT