From 681c774f329e2f59877ad47d2e2e4d1e6f1a8bf1 Mon Sep 17 00:00:00 2001 From: rayhpeng Date: Fri, 31 Jul 2026 16:50:08 +0800 Subject: [PATCH] fix(schedule): validate the cron expression itself, not just its arity ScheduleSpec.__post_init__ only counted fields, so five fields of garbage ("x x x x x", out-of-range values) constructed successfully and surfaced later in next_after as a croniter exception outside the ScheduleError family -- turning a 422-mappable input error into an unclassified 500. Probe the normalized expression with croniter at construction and wrap the failure in InvalidScheduleError. Co-Authored-By: Claude Fable 5 --- .../deerflow/domain/schedule/model/spec.py | 13 +++++++++++-- backend/tests/test_schedule_domain.py | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/backend/packages/harness/deerflow/domain/schedule/model/spec.py b/backend/packages/harness/deerflow/domain/schedule/model/spec.py index 4fe54ff93..560ba36be 100644 --- a/backend/packages/harness/deerflow/domain/schedule/model/spec.py +++ b/backend/packages/harness/deerflow/domain/schedule/model/spec.py @@ -4,7 +4,7 @@ from dataclasses import dataclass from datetime import UTC, datetime from zoneinfo import ZoneInfo, ZoneInfoNotFoundError -from croniter import croniter +from croniter import CroniterError, croniter from deerflow.domain.schedule.exceptions import InvalidScheduleError from deerflow.domain.schedule.model.enums import ScheduleType @@ -75,7 +75,16 @@ class ScheduleSpec: fields = [part for part in self.cron.split() if part] if len(fields) != CRON_FIELD_COUNT: raise InvalidScheduleError(f"Cron expression must contain exactly {CRON_FIELD_COUNT} fields") - object.__setattr__(self, "cron", " ".join(fields)) + normalized = " ".join(fields) + # Counting fields is not parsing: five fields of garbage would + # otherwise construct fine and surface later, deep in next_after, + # as a croniter error outside the ScheduleError family the router + # maps -- an input error turned unclassified 500. + try: + croniter(normalized) + except CroniterError as exc: + raise InvalidScheduleError(f"Invalid cron expression: {normalized}") from exc + object.__setattr__(self, "cron", normalized) if self.schedule_type is ScheduleType.ONCE: if self.run_at is None: diff --git a/backend/tests/test_schedule_domain.py b/backend/tests/test_schedule_domain.py index 302b6eed3..fa2661020 100644 --- a/backend/tests/test_schedule_domain.py +++ b/backend/tests/test_schedule_domain.py @@ -94,6 +94,21 @@ class TestScheduleSpecInvariants: def test_cron_whitespace_is_normalized_on_construction(self): assert ScheduleSpec(ScheduleType.CRON, "UTC", cron=" 0 9 * * * ").cron == "0 9 * * *" + @pytest.mark.parametrize("expr", ["x x x x x", "60 * * * *", "* * * * 99"]) + def test_a_five_field_expression_the_parser_rejects_is_invalid_at_construction(self, expr): + """Counting fields is not parsing: garbage with five fields used to + construct successfully and only blow up later, deep in `next_after`, + as a croniter exception outside the ScheduleError family -- turning a + 422-mappable input error into an unclassified 500.""" + with pytest.raises(InvalidScheduleError, match="Invalid cron expression"): + ScheduleSpec.cron_schedule(expr, "UTC") + + @pytest.mark.parametrize("expr", ["*/5 * * * *", "0 9 * * mon-fri", "0 0 1,15 * *"]) + def test_real_cron_vocabulary_still_constructs(self, expr): + """The validity probe must not over-reject: steps, ranges, lists, and + named weekdays are everyday croniter vocabulary.""" + assert ScheduleSpec.cron_schedule(expr, "UTC").cron == expr + def test_naive_run_at_is_localized_to_the_schedule_timezone(self): spec = ScheduleSpec.once_at(datetime(2026, 8, 1, 9, 0), "Asia/Shanghai") # noqa: DTZ001 -- naive input is the subject assert spec.run_at.utcoffset() == timedelta(hours=8)