fix(calendar): OR day-of-month and day-of-week when both restricted#644
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCron matching logic was updated so DaysOfMonth and DaysOfWeek use restriction flags: when both are restricted, the schedule matches if either DOM or DOW matches (OR semantics). Months remain an independent short-circuit. New unit tests exercise these scenarios with fixed 2026 UTC timestamps. ChangesCron Day-Matching Logic & Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/calendar/cron.go`:
- Around line 122-137: The cron DOM/DOW semantics change needs unit tests to
prevent regressions: add a table-driven test file
(internal/calendar/cron_test.go) that uses ParseCron to build schedules and
calls cs.matches(time) for the following cases: the OR case ("0 9 1 * 1")
verifying it fires on the 1st and on Mondays but not other days or wrong hours;
a DOM-only spec ("0 9 15 * *") confirming weekday is ignored; a DOW-only spec
("0 9 * * 1") confirming day-of-month is ignored; a wildcard spec ("0 9 * * *")
confirming it fires every day at 09:00; and a month-restricted case showing
month short-circuits regardless of day fields. Ensure each scenario is
table-driven and asserts cs.matches returns the expected boolean.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 26662df9-4e54-4330-ac90-042f7e1ad276
📒 Files selected for processing (1)
internal/calendar/cron.go
0c6c469 to
28ff949
Compare
Pin the new behavior so future regressions to AND semantics are caught: the canonical "0 9 1 * 1" OR case, DOM-only and DOW-only specs ignoring the unrestricted axis, an unrestricted-day wildcard, and a month-restricted spec confirming month still short-circuits regardless of day fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/calendar/cron_test.go`:
- Around line 11-35: TestMatches_DOMandDOW_ORWhenBothRestricted is missing a
case where both day-of-month and day-of-week match; update that test (after
ParseCron and before the loop) to include a case in the cases slice with a time
that is both the 1st of the month and a Monday at 09:00 UTC (so
sched.matches(...) should return true), referencing the existing test function
TestMatches_DOMandDOW_ORWhenBothRestricted and the sched.matches method to
ensure the OR-path where both DOM and DOW are true is explicitly exercised.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9f578637-39b4-4a42-9c8a-c2608ecd5786
📒 Files selected for processing (1)
internal/calendar/cron_test.go
|
@CharlieKerfoot TY 😄 |
Summary
Standard cron semantics treat day-of-month and day-of-week as a union when both fields are restricted, but
CronSchedule.matchesin internal/calendar/cron.go:119 ANDed them. A schedule like0 9 1 * 1fired only when the date was both the 1st and a Monday, instead of either.Fix
Split the day check: when only one of DOM/DOW is restricted, require that match; when both are restricted, accept the tick if either matches. Month check moved above the day logic so it still short-circuits cleanly.
Summary by CodeRabbit