Repository navigation
fix(cron): refuse seconds-bearing expressions at validation - #24084
aniruddhaadak80 wants to merge 1 commit into
Conversation
croniter reads a six-field expression that carries a seconds field, but is_due never fires a job twice in the same UTC minute, so the second occurrence such a schedule advertises is suppressed forever: the job lists a next run that can never happen. Validation is the one funnel every writer passes through (fields.build_job on create, fields.apply_job_update on update), so refuse the shape there and the advertised schedule agrees with the execution contract again. Five-field expressions and the one-field @ aliases keep validating.
github-actions
Bot
added
the
fork
github-actions
Bot
added
readiness: action required
UX Review (Opus 5.5, fork) — ⏭️ skippedRevision |
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the validator now accepts only the five-field shape the minute-grain executor can run, and both writers go through it. Notes
[DESIGN-REVIEWED] ed506b0 Description read[DESCRIPTION-READ] ee351006063e7bda7017e81e3aaa5871fcd0d010aa3446cb2b639b69b6ca4533 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
Problem / Motivation
Goal: stop accepting a cron schedule that lists a next run the scheduler can never carry out.
A six-field cron expression (
* * * * * *, seconds in the first field) validates today, although the documented shape and every surface the product exposes say five fields (min hour dom month dow). The observable symptom: create such a job, and the listing advertises a next run one second out — then the due check suppresses that occurrence and every later one inside the minute, so the job effectively runs one minute-grain behind what it advertised, forever. Issue #24044's repro: at a fixed UTC minute boundary,validate_cron_expr("...")andis_due(job, boundary)both pass,compute_next_run_ts(job, boundary)returnsboundary + 1, andis_due(job, boundary + 1)returns False.Why it matters
A schedule the service cannot honor is persisted as if it were honored, and the contradiction is silent: the UI and API keep naming runs the runner refuses. Correctness of the advertised next run is the contract users plan around (a
kirocrew cron listentry says "runs at 12:00:01 next"; it next runs at 12:01). The refusal is also the cheap place to fail: validation already funnels every writer.Not a goal
@daily,@hourly, ...), interval (every_secs) or one-shot (at_ts) schedules.What changed (motivation → approach → change)
Observed symptom → root cause → change:
validate_cron_expr(src/kiro_crew/cron_service/schedule.py) returnscroniter.is_valid(expr)verbatim, and croniter accepts six-field expressions — so the validator's reach exceeds whatis_due's one-run-per-UTC-minute gate (schedule.py,is_due: the same-minute refuse, plus the 120 s same-occurrence bound) can ever deliver.validate_cron_exprnow refuses any width croniter reads beyond five fields. Five-field expressions and croniter's one-field@aliases (which carry no seconds field) keep validating. The docstring records why, so a future loosening re-reads the contract before it lands.No user-facing copy changes: the refusal reuses the existing
Invalid cron expression: <expr>message every writer already reports, and the MCP tool description, changelog catalog and docs already document the five-field syntax (src/kiro_crew/mcp_cron_tool_list.py,src/kiro_crew/change_card_catalog_kinds.py:739refuses non-five-field strings on its own path,src/kiro_crew/docs/cron-and-scheduling.md).Backwards compatibility
Breaking: creating or updating a job with a six-field cron expression no longer persists — it is refused with
Invalid cron expression(dashboard reports its existinginvalid_croncode). That is the point: every such schedule was already advertising runs the runner suppressed. Writer sweep: the writers (MCPcron_add/cron_update, dashboard create/PATCH, CLI) all build or update throughfields.build_job/fields.apply_job_update, so one predicate covers them all;change_card_catalog_kinds.pyalready wrote five-field-only on its path.Reader: src/kiro_crew/cron_service/store.py
_job_from_record-- entry: store decode -- does not call the predicate, so a six-field record already on disk keeps loading and keeps its unchanged one-run-per-minute behavior; nothing is dropped or rewritten.Removes nothing: no stored job, key, field or capability is deleted; only the acceptance of one previously-accepted JSON payload shape at write time narrows.
Tests
test/test_cron.py::TestValidateCronExprgains three tests:test_alias_valid—@daily/@yearly/@midnightstill validate.test_seconds_field_rejected— six-field shapes (* * * * * *,0 0 * * * *) no longer validate.test_seconds_expr_refused_at_create_and_update— both writer owners refuse the shape, so the guard cannot be bypassed by a caller that skips the bare predicate.Proof: PROVEN test_seconds_field_rejected, test_seconds_expr_refused_at_create_and_update (head 5c46e1701e) — both fail with the production hunks reverted (
DID NOT RAISE ValueError), pass with them applied.Local suite: test_cron.py, test_cron_refactor_contract.py, test_cron_service_bindings.py all green on Windows;
scripts/check_black_formatting.py,scripts/check_subprocess_encoding.py,isort --check-only,flake8on the touched files,mypy src/kiro_crew,check_comment_history.py,check_brand_name.pyandtest_flake_pattern_ratchet.pyall pass.Manual verification
N/A — unit coverage sufficient: the change is a pure predicate, and the writer funnels are exercised by the create/update test without a gateway. The dashboard refusal path (
invalid_cron) is the pre-existing behavior forValueErrors from these owners.Related Issues
Fixes #24044
Sibling sweep
Swept:
git grep -n "5 field|five-field|min hour dom month dow" -- src docs websiteSibling: src/kiro_crew/change_card_catalog_kinds.py:739 -- already refuses non-five-field
cron_exprstrings on its own path -- not affected.Sibling: src/kiro_crew/mcp_cron_tool_list.py:108 -- documents "Standard 5-field cron expression" at the MCP surface -- not affected.
Sibling: src/kiro_crew/dashboard/handlers/cron_jobs.py:431 -- treats a schedule string as cron only when five fields, parses every other shape as an interval, and never forwards a six-field string as a cron expression -- not affected.
Sibling: docs/system-specs/modules/learn-cron-dashboard.md:453, src/kiro_crew/docs/cron-and-scheduling.md:58 -- already document the five-field syntax -- not affected, no doc change needed.
Siblings: none beyond these -- the seconds-acceptance lived only in
validate_cron_expr.Pattern harvest
Rule candidate: review-prompt
Pattern: a validator accepts a form the executor independently refuses — validation and execution must share one contract, and a third-party parser's capability (croniter reads seconds) is not the product's contract.
Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
Accepted under the project's Apache-2.0 license; see the Licensing section of CONTRIBUTING.md.