feat: add cron-doctor — diagnose cron death-traps before they ship#757
Conversation
Adds a new DevOps skill that catches the five silent cron failure modes: impossible dates (never fires), OR-semantics (fires too often), midnight spikes, uneven step drift, and leap-year February 29. Includes a zero-dependency validation engine (cron-engine.js, 638 lines) with a CLI wrapper, plus a SKILL.md following the canonical template. Source: takeaseatventure/devops-skills (MIT)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f35298f7ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| script: | ||
|
|
||
| ```bash | ||
| 0 0 * * 1 [ "$(date +%d)" = "01" -o "$(date +%d)" = "15" ] && your-command |
There was a problem hiding this comment.
In a real Vixie-style crontab command, unescaped % characters are treated specially, so this copy-paste fix using date +%d is split before date receives the format string. For the crontab scenario this skill explicitly targets, users trying to implement the documented Monday-and-1st/15th guard will get a broken command unless the % signs are escaped.
Useful? React with 👍 / 👎.
| if (r.description) console.log('description: ' + r.description); | ||
| if (r.warnings && r.warnings.length) { | ||
| console.log('warnings:'); | ||
| r.warnings.forEach((w) => console.log(' - ' + w)); |
There was a problem hiding this comment.
When validate() emits a warning, such as the OR-semantics warning for 0 0 1,15 * 1, this concatenates the warning object directly and the documented CLI prints - [object Object]. That hides the actual trap message from users running node scripts/cli.js validate ..., while observations and suggestions already print their .message fields correctly.
Useful? React with 👍 / 👎.
| const n = parseInt(token, 10); | ||
| if (!isNaN(n)) return n; |
There was a problem hiding this comment.
Because parseInt accepts a numeric prefix, malformed fields such as 0 0 1x * * are considered valid and scheduled as day 1 instead of being rejected. Since this engine is advertised as syntax validation before shipping cron expressions, typos with trailing characters can silently pass validation and produce a different schedule.
Useful? React with 👍 / 👎.
| d.setSeconds(0, 0); | ||
| d = new Date(d.getTime() + 60000); | ||
|
|
||
| let maxScan = 600000; // ~416 days ceiling |
There was a problem hiding this comment.
For sparse schedules whose next occurrence is more than about 416 days away, this ceiling is exhausted before any match; for example, 0 0 29 2 * from June 26, 2026 should next fire on February 29, 2028, but nextRuns returns an empty list. That makes the CLI's next-run check misleading for the leap-day trap this skill is meant to explain.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (r.description) console.log('description: ' + r.description); | ||
| if (r.warnings && r.warnings.length) { | ||
| console.log('warnings:'); | ||
| r.warnings.forEach((w) => console.log(' - ' + w)); |
There was a problem hiding this comment.
🟡 CLI prints warnings as unreadable "[object Object]" instead of the actual warning text
Warnings are printed by concatenating the raw object to a string (' - ' + w at skills/cron-doctor/scripts/cli.js:47) instead of accessing its .message property, so every CLI warning displays as - [object Object].
Impact: Users running node cli.js validate see gibberish instead of the actual warning message.
Inconsistency with observation/suggestion handlers
The observations handler at skills/cron-doctor/scripts/cli.js:51 and the suggestions handler at skills/cron-doctor/scripts/cli.js:55 both correctly access .level and .message on each item. The warnings handler on line 47 skips this and prints the object directly. All three arrays contain objects with { level, message } shape, as seen in skills/cron-doctor/scripts/cron-engine.js:423-426 and similar push sites.
| r.warnings.forEach((w) => console.log(' - ' + w)); | |
| r.warnings.forEach((w) => console.log(' [' + (w.level || 'info') + '] ' + w.message)); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| function describeFieldMonth(field) { | ||
| if (pIsWildcard(field)) return { restricted: false, text: 'every month' }; | ||
| const vals = [...(field.values || [])].sort((a, b) => a - b); | ||
| return { restricted: true, text: 'in ' + listJoin(vals.map(v => capitalize(MONTH_NAMES[v - 1]))) }; |
There was a problem hiding this comment.
🟡 Month descriptions produce garbled "in in February" text due to duplicated preposition
The month text is built with an "in" prefix ('in ' + listJoin(...) at skills/cron-doctor/scripts/cron-engine.js:259), then callers prepend a second "in" when appending it (' in ' + monthDesc.text at line 221), so descriptions read "on day-of-month 30 in in Feb" instead of "on day-of-month 30 in Feb".
Impact: Human-readable descriptions for any month-restricted cron expression contain doubled "in in" making them look broken.
Three call sites produce the double preposition
The describeFieldMonth function at skills/cron-doctor/scripts/cron-engine.js:259 returns text starting with "in ". Lines 221, 224, and 227 all append ' in ' + monthDesc.text, producing "in in Feb". The SKILL.md example at skills/cron-doctor/SKILL.md:159 even documents this broken output: "At 00:00, on day-of-month 30 in in FEB". The fix is to remove the "in " prefix from describeFieldMonth's text and leave the callers' prefix intact, or vice versa.
| return { restricted: true, text: 'in ' + listJoin(vals.map(v => capitalize(MONTH_NAMES[v - 1]))) }; | |
| return { restricted: true, text: listJoin(vals.map(v => capitalize(MONTH_NAMES[v - 1]))) }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const domValues = [...(parsed.dom.values || [])]; | ||
| if (!parsed.domRestricted && parsed.month.values && ![...parsed.month.values].every(m => m === 2)) { | ||
| // skip | ||
| } else if (parsed.domRestricted && !parsed.dom.special && domValues.includes(31)) { |
There was a problem hiding this comment.
🔴 Impossible-date detection misses February 30th — the tool's own lead example
The impossible-date check only looks for day 31 (domValues.includes(31) at skills/cron-doctor/scripts/cron-engine.js:433), so the tool's primary documented death-trap example 0 0 30 2 * (February 30th, which never fires) produces no targeted impossible-date warning.
Impact: Users validating impossible dates like February 30th get no diagnostic explaining why the schedule never fires.
Validation gap for day 30 in February and other impossible combos
The SKILL.md at skills/cron-doctor/SKILL.md:80-85 uses 0 0 30 2 * as the lead example of death-trap #1. When this expression is validated, domValues is [30], so domValues.includes(31) is false and the entire impossible-date check is skipped. The separate February 29 check at skills/cron-doctor/scripts/cron-engine.js:480 also doesn't match because dom is 30, not 29.
The estimateFrequency function will still report "never (impossible schedule)" as an observation, but no specific warning names the root cause (impossible date). The check should be generalized to detect any day-of-month that exceeds the maximum days in the constrained month(s), e.g. day 30 in February (max 29), day 31 in April (max 30), etc.
Prompt for agents
The impossible-date detection in validate() at cron-engine.js:429-451 only checks for day 31 via domValues.includes(31). It needs to be generalized to detect any day-of-month value that exceeds the maximum number of days in the constrained months.
The max days per month are: Jan=31, Feb=29, Mar=31, Apr=30, May=31, Jun=30, Jul=31, Aug=31, Sep=30, Oct=31, Nov=30, Dec=31. For each constrained month, check whether any of the specified dom values exceed that month's maximum. For example, 0 0 30 2 * should warn that February has at most 29 days so day 30 is impossible.
This should replace or extend the existing domValues.includes(31) check. The February 29 leap-year check at line 476 can remain as a separate, more specific warning about leap-year behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| let description = ''; | ||
| if (count >= 525600) description = 'every minute'; | ||
| else if (count >= 500000) description = 'multiple times per minute'; |
There was a problem hiding this comment.
🟡 Frequency label says "multiple times per minute" for a schedule that can only fire once per minute
The frequency description labels a count between 500,000 and 525,599 as "multiple times per minute" (skills/cron-doctor/scripts/cron-engine.js:538), but standard cron can fire at most once per minute, so this label is factually wrong for any reachable count in that range.
Impact: Users see a misleading frequency description that implies a firing rate that is physically impossible for 5-field cron.
How the impossible label gets triggered
The estimateFrequency function at skills/cron-doctor/scripts/cron-engine.js:523-550 iterates minute-by-minute through a sample year, so count can be at most 525,600 (365×24×60). Line 537 catches count=525,600 as "every minute." Line 538 catches 500,000–525,599 as "multiple times per minute." But since each minute is only counted once, counts in this range actually mean "almost every minute" (e.g., 0-58 * * * * = 516,060 fires/year = once per minute except at :59). The label should say something like "nearly every minute" instead.
| else if (count >= 500000) description = 'multiple times per minute'; | |
| else if (count >= 500000) description = 'nearly every minute'; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!domAny && !dowAny) return domMatch || dowMatch; // OR semantics | ||
| return domMatch && dowMatch; |
There was a problem hiding this comment.
📝 Info: OR semantics for day-of-month + day-of-week correctly implemented in matches()
The matches function at skills/cron-doctor/scripts/cron-engine.js:336 correctly uses domMatch || dowMatch when both dom and dow are restricted, matching standard Vixie cron OR semantics. This is one of the skill's key "death traps" and is correctly handled in the engine despite being counter-intuitive. The corresponding validate() check at line 422 also correctly flags this as a warning.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function ordinal(n) { | ||
| const s = ['th', 'st', 'nd', 'rd']; | ||
| const v = n % 100; | ||
| return n + (s[(v - 20) % 10] || s[v] || s[0]); | ||
| } |
There was a problem hiding this comment.
📝 Info: The ordinal() helper uses JavaScript negative modulo but works for the actual input range
The ordinal function at skills/cron-doctor/scripts/cron-engine.js:591-595 uses (v - 20) % 10 which produces negative results in JavaScript for v < 20 (e.g., v=1 gives -19%10 = -9). Negative array indices return undefined in JS, which falls through to the s[v] fallback. This works correctly for inputs 1-5 (the only values used via # parsing), but would produce wrong results for values like 11, 12, 13 (which should get 'th' but actually do via the fallback chain). Since the function is only called from describeFieldDow for nth values 1-5, it works in practice.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function estimateFrequency(parsed) { | ||
| try { | ||
| // Count runs over a sample year | ||
| const start = new Date(2025, 0, 1, 0, 0, 0, 0); | ||
| const end = new Date(2026, 0, 1, 0, 0, 0, 0); | ||
| let count = 0; | ||
| let d = new Date(start.getTime()); | ||
| let maxScan = 540000; // ~375 days | ||
| while (d < end && maxScan-- > 0) { | ||
| if (matches(d, parsed)) count++; | ||
| d = new Date(d.getTime() + 60000); | ||
| } | ||
|
|
||
| let description = ''; | ||
| if (count >= 525600) description = 'every minute'; | ||
| else if (count >= 500000) description = 'multiple times per minute'; | ||
| else if (count >= 8000) description = 'hourly or more'; | ||
| else if (count >= 300) description = 'daily or more'; | ||
| else if (count >= 40) description = 'weekly or more'; | ||
| else if (count >= 8) description = 'monthly or more'; | ||
| else if (count >= 1) description = 'yearly or less'; | ||
| else description = 'never (impossible schedule)'; | ||
|
|
||
| return { description, runsPerYear: count }; | ||
| } catch (e) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: estimateFrequency scans ~375 days of minutes which is computationally expensive
The estimateFrequency function at skills/cron-doctor/scripts/cron-engine.js:523-550 iterates up to 540,000 times (one per minute), creating a new Date object each iteration. Similarly, nextRuns at line 301 has a 600,000 iteration ceiling. For a simple validation call this is significant computation (~525K iterations and Date allocations for the frequency estimate alone). This is acceptable for a CLI tool or occasional validation but could be problematic if validate() is called in a hot loop or on a server handling many requests. Not a bug per se, but worth noting for anyone considering using this engine at scale.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| // Parse + describe -> returns { text, error, parsed } | ||
| const d = describe('0 0 30 2 *'); | ||
| console.log(d.text); // "At 00:00, on day-of-month 30 in in FEB" |
There was a problem hiding this comment.
🚩 SKILL.md documents the buggy double-in output as if it were correct
Line 159 of the SKILL.md shows // "At 00:00, on day-of-month 30 in in FEB" as the expected output of describe('0 0 30 2 *'). This documents the actual (buggy) behavior of the engine rather than the intended behavior. When the double-in bug in describeFieldMonth is fixed, this example output in the docs will also need updating to remove the duplicate preposition.
Was this helpful? React with 👍 or 👎 to provide feedback.
| safe(() => { | ||
| const r = cron.validate(expr); | ||
| console.log('valid: ' + r.valid); | ||
| if (r.description) console.log('description: ' + r.description); |
There was a problem hiding this comment.
📝 Info: validate() returns description as a string, but CLI prints it directly which works
At skills/cron-doctor/scripts/cron-engine.js:515, the validate() return includes description: desc where desc is the string result of describeParsed(). In skills/cron-doctor/scripts/cli.js:44, r.description is logged directly via string concatenation. This works because desc is already a string. However, if describeParsed were ever changed to return an object, this would break silently. Currently correct but tightly coupled.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| let description = ''; | ||
| if (count >= 525600) description = 'every minute'; | ||
| else if (count >= 500000) description = 'multiple times per minute'; |
There was a problem hiding this comment.
📝 Info: Frequency estimate label "multiple times per minute" is unreachable/misleading for 5-field cron
At skills/cron-doctor/scripts/cron-engine.js:538, the frequency bucket count >= 500000 is labeled 'multiple times per minute'. In standard 5-field cron, the minimum granularity is 1 minute, so a job can fire at most once per minute (max 525,600/year). A count between 500,000 and 525,599 is reachable (e.g., every minute except certain months) but represents "nearly every minute," not "multiple times per minute." The label is factually misleading but only affects an unusual edge case in descriptive output.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function nextRuns(expr, fromDate, count) { | ||
| count = count || 10; | ||
| const p = parseCron(expr); | ||
| const runs = []; | ||
| let d = new Date(fromDate.getTime()); | ||
| d.setSeconds(0, 0); | ||
| d = new Date(d.getTime() + 60000); | ||
|
|
||
| let maxScan = 600000; // ~416 days ceiling | ||
| while (runs.length < count && maxScan-- > 0) { | ||
| if (matches(d, p)) { | ||
| runs.push(new Date(d.getTime())); | ||
| } | ||
| d = new Date(d.getTime() + 60000); | ||
| } | ||
| return runs; |
There was a problem hiding this comment.
📝 Info: nextRuns scanning ceiling may not find all results for sparse schedules
At skills/cron-doctor/scripts/cron-engine.js:301, maxScan is set to 600,000 minutes (~416 days). For a schedule like 0 0 29 2 * (fires only on Feb 29 in leap years), the next fire could be up to ~4 years away, which exceeds 416 days. The function would return fewer results than requested (or an empty array) without any error indication. This is a known limitation documented in the SKILL.md, and the estimateFrequency function has a similar ceiling at 540,000 minutes. Not flagged as a bug since the limitation is acknowledged, but callers should be aware that sparse schedules may return incomplete results.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const n = parseInt(token, 10); | ||
| if (!isNaN(n)) return n; |
There was a problem hiding this comment.
📝 Info: parseSingleNum accepts numeric strings that start with digits but have trailing text
At skills/cron-doctor/scripts/cron-engine.js:94, parseInt(token, 10) is used to parse numeric values. parseInt in JavaScript accepts leading digits and ignores trailing non-numeric characters (e.g., parseInt('5abc', 10) returns 5). This means tokens like '5abc' would be parsed as 5 instead of being rejected. In practice, this is unlikely to cause issues because cron field tokens are already split by commas and dashes, but it means the parser is more lenient than expected for malformed input.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Maintainer workflow refresh: closing and reopening to retrigger pull_request checks against the updated PR body and README source-credit fix. |
3 participants
What this adds
A new DevOps skill:
cron-doctor- catches the five silent cron failure modes that pass syntax validation but break in production.The five death-traps it catches
0 0 30 2 *) - valid syntax, never fires (Feb has no 30th)0 0 1,15 * 1) - the most misunderstood cron rule; fires ~6x/month instead of ~2x0 0 * * *) - every job competes at 00:00*/7 * * * *) - 60 isn't divisible by 7, so intervals drift0 0 29 2 *) - fires once every 4 yearsWhat's included
skills/cron-doctor/SKILL.md- full skill following the canonical templateskills/cron-doctor/scripts/cron-engine.js- zero-dependency validation engineskills/cron-doctor/scripts/cli.js- minimal CLI wrapperREADME.md- community source credit fortakeaseatventure/devops-skillsAttribution
takeaseatventure/devops-skills(MIT)Quality Bar Checklist ✅
All applicable items must be checked before merging.
docs/contributors/quality-bar.mdanddocs/contributors/security-guardrails.md.SKILL.mdfrontmatter is valid (checked withnpm run validate).risk:tag (none,safe,critical,offensive, orunknownfor legacy/unclassified content).## Limitations(or equivalent accepted constraints) section.risk: safeand read-only behavior are documented.SKILL.md, I checked theskill-reviewGitHub Actions result and addressed any actionable feedback.SKILL.mdor risky guidance, I manually reviewed the logic, safety, failure modes, andrisk:label instead of relying on automated checks alone.CATALOG.md,skills_index.json,data/*.json) in this PR.README.md.license:andlicense_source:for the upstream MIT source.