fix(cron): validate field value ranges in isValidCron

[nolanchic]

Problem

isValidCron only checked the shape of each cron sub-field (a regex), not the value ranges. Out-of-range expressions were accepted as valid, stored as cron tasks, and then silently never fired — a footgun with no feedback to the user.

isValidCron("99 99 * * *")  → true   // minute > 59, hour > 23 — impossible, never fires
isValidCron("0 0 32 * *")   → true   // no 32nd day of month — never fires
isValidCron("0 0 1 13 *")   → true   // no 13th month — never fires
isValidCron("0 24 * * *")   → true   // hour = 24 is invalid (valid is 0-23)

Fix

Add per-field value-range validation, in addition to the existing shape check:

field	range
minute	0–59
hour	0–23
day-of-month	1–31
month	1–12
day-of-week	0–7 (0 and 7 both = Sunday, per POSIX)

Every literal number in a value (N), range (N-M), or ranged-step (N-M/S) must fall within its field's range. Step values (*/S and the /S suffix) must be positive integers (rejects e.g. 0-59/0). Bare * wildcards remain always-valid.

The validation is field-position aware, so a literal that's legal in one field but not another is handled correctly (e.g. 32 is fine as a year-style value but rejected in DOM).

Behavior change

Out-of-range cron expressions are now rejected at letta cron add --cron "…" with the existing invalid-expression error, instead of being accepted and silently never firing. All previously-valid expressions remain valid (verified against the existing test suite + new boundary tests).

Tests

Extended isValidCron in src/cron/parse-interval.test.ts:

• new "rejects out-of-range values that would never fire" test: minute>59, hour>23, impossible DOM (0 and 32), impossible month (0 and 13), DOW>7, out-of-range inside ranges (0-60, 1-32 in DOM), step must be > 0 (0-59/0), out-of-range inside a comma list rejects the whole field
• new "accepts valid boundary values for each field" test: min/max boundaries, DOW 0 and 7 (both Sunday), full ranges 0-59 0-23 1-31 1-12 0-7

Verification

• bun test src/cron/ — 95 pass
• bun run typecheck — clean
• bun run check:boundaries, check:cycles, check:exported-functions — clean
• biome — clean on changed files

AI disclosure

Authored with AI assistance (Claude Code) and reviewed by a human before submission.

[just-cameron]

Nice catch. I think this should probably use an existing cron parser instead of extending the custom validator one case at a time.

The current patch still leaves validation and matching with different semantics. Example: this PR accepts day-of-week 7 as Sunday, but cronMatchesTime compares against Date.getDay() values 0-6, so 0 0 * * 7 becomes valid but still never fires. Same footgun, just one layer deeper. Ranges containing 7 have the same problem, and reversed ranges like 59-0 * * * * still validate even though the matcher can never satisfy them.

I would rather make a library the source of truth for cron semantics, probably cron-parser:

• keep our product dialect gate before parsing: exactly 5 fields, and numeric syntax only if we do not want to expand support to MON, JAN, ?, L, etc.
• implement isValidCron by calling the parser after that dialect gate.
• ideally implement cronMatchesTime via the parser's next occurrence calculation, so validation and execution share the same semantics.

Shape would be roughly:

import { CronExpressionParser } from "cron-parser";

const SUPPORTED_CRON_RE = /^[\d\s*,*/-]+$/;

export function isValidCron(expr: string): boolean {
  const trimmed = expr.trim();
  const fields = trimmed.split(/\s+/);

  if (fields.length !== 5) return false;
  if (!SUPPORTED_CRON_RE.test(trimmed)) return false;

  try {
    CronExpressionParser.parse(trimmed, { strict: false });
    return true;
  } catch {
    return false;
  }
}

Then cronMatchesTime can ask for the next occurrence after the previous millisecond and compare it to the current minute. That also gives us timezone and DST semantics from a maintained library rather than our own partial implementation.

If we want to keep this PR smaller, I would at least fix DOW 7 matching and reversed ranges here, and change the PR description to say this is field-range validation only, not robust cron validation.

Written by Cameron ◯ Letta Code

[nolanchic]

Thanks for the review @just-cameron — agreed on all counts. The DOW 7 / reversed-range gaps are exactly the "validation and matching with different semantics" problem, and patching them one case at a time is the wrong direction.

I'll rework this to put cron-parser behind both isValidCron and cronMatchesTime so they share one source of truth, keeping the product dialect gate (5 fields, numeric syntax only) in front of the parser. I'll also drop the hand-rolled fieldMatches step/range logic in favor of the parser's next-occurrence calculation for matching, which gives us timezone/DST for free.

Will push the rework to this PR.

[nolanchic]

Reworked per your feedback — cron-parser is now the source of truth for both validation and matching (commit f8f3d82).

isValidCron: keeps the product dialect gate (exactly 5 fields, numeric syntax only via /^[\d\s*,*/-]+$/) then delegates to CronExpressionParser.parse(expr, { strict: false }). So out-of-range values, reversed ranges (59-0), and zero steps (0-59/0) are all rejected by the library instead of by hand-rolled per-field checks.

cronMatchesTime: now asks cron-parser for the next occurrence after the previous minute and checks it lands in the target minute. This closes the DOW-7 gap you called out — 0 0 * * 7 now both validates and matches Sunday (previously valid-but-never-fired). Reversed ranges can't sneak through either, since the same parser is used for both. Timezone/DST now come from the library; an invalid timezone falls back to local (validated via Intl.DateTimeFormat before handing to cron-parser) to preserve the previous behavior.

Drops ~170 lines of hand-rolled fieldMatches / getTimeComponents / range math.

Tests (src/cron/parse-interval.test.ts, 96 pass across src/cron/):

• new: DOW 7 matches Sunday just like 0 (the exact gap you flagged); reversed ranges (59-0, 31-1) rejected at validation
• existing: all prior valid/invalid isValidCron and cronMatchesTime cases still pass unchanged, including the invalid-timezone-falls-back-to-local case

bun run typecheck, biome, layer/cycles/exported-functions checks all clean. New dep: cron-parser ^5.6.1.

I did not migrate cronMatchesTime's callers to use the parser's iterator directly (e.g. scheduler.ts still calls cronMatchesTime(now)), to keep this PR scoped to "fix validation/matching semantics". Happy to follow up on that if you'd rather have the scheduler drive off next() directly.