fix(cron): coerce string repeat values on the UPDATE path too
The create-path coercion (salvaged from #78928) left update_job and the cronjob tool's update handler comparing/storing raw strings: repeat= 'forever' via update raised TypeError in the tool path and stored the raw string via update_job, breaking the next mark_job_run ('str' has no .get). Extract normalize_repeat_value as the shared chokepoint (shape from #77366 by @andrexibiza, with garbage-rejection semantics) and route create_job, update_job, and the tool update handler through it. Completed counters are preserved across repeat updates. Class: #66824 #64520 #7142 #71987 #95706, update half of #77366.
This commit is contained in:
76
cron/jobs.py
76
cron/jobs.py
@@ -774,6 +774,36 @@ def ensure_dirs():
|
||||
# Schedule Parsing
|
||||
# =============================================================================
|
||||
|
||||
def normalize_repeat_value(repeat: Any) -> Optional[int]:
|
||||
"""Coerce a repeat value from any entry point into ``Optional[int]``.
|
||||
|
||||
The tool schema exposes ``repeat`` as an integer, but agents and users
|
||||
legitimately pass the user-facing strings ``'forever'``/``'once'`` or
|
||||
numeric strings (``'3'``). Uncoerced strings previously died with
|
||||
``'<=' not supported between instances of 'str' and 'int'`` at create
|
||||
(#66824/#64520/#7142/#71987/#95706) and were stored raw by update paths,
|
||||
breaking ``mark_job_run`` later. Semantics: ``'forever'``-family -> None
|
||||
(infinite), ``'once'``-family -> 1, numeric -> int, 0/negative -> None,
|
||||
anything else -> ValueError (never store garbage).
|
||||
"""
|
||||
if repeat is None:
|
||||
return None
|
||||
if isinstance(repeat, str):
|
||||
repeat_str = repeat.strip().lower()
|
||||
if repeat_str in ("forever", "infinite", "inf", "none", ""):
|
||||
return None
|
||||
if repeat_str in ("once", "one", "1x"):
|
||||
return 1
|
||||
try:
|
||||
repeat = int(repeat_str)
|
||||
except ValueError:
|
||||
raise ValueError(
|
||||
f"Invalid repeat value {repeat!r}: use an integer, "
|
||||
f"'forever', or 'once'."
|
||||
)
|
||||
return None if repeat <= 0 else int(repeat)
|
||||
|
||||
|
||||
def parse_duration(s: str) -> int:
|
||||
"""
|
||||
Parse duration string into minutes.
|
||||
@@ -2248,29 +2278,10 @@ def create_job(
|
||||
parsed_schedule = parse_schedule(schedule)
|
||||
|
||||
# Normalize repeat: treat 0 or negative values as None (infinite).
|
||||
# Also coerce the documented string forms ('forever' -> None,
|
||||
# 'once' -> 1, numeric strings -> int) — the tool schema exposes repeat
|
||||
# as an integer but agents legitimately pass the user-facing strings
|
||||
# 'forever'/'once', which previously died with
|
||||
# "'<=' not supported between instances of 'str' and 'int'"
|
||||
# (#66824/#64520/#7142). Coerce here so every entry point (tool, CLI,
|
||||
# API, dashboard) inherits the fix.
|
||||
if isinstance(repeat, str):
|
||||
repeat_str = repeat.strip().lower()
|
||||
if repeat_str in ("forever", "infinite", "inf", "none", ""):
|
||||
repeat = None
|
||||
elif repeat_str in ("once", "one", "1x"):
|
||||
repeat = 1
|
||||
else:
|
||||
try:
|
||||
repeat = int(repeat_str)
|
||||
except ValueError:
|
||||
raise ValueError(
|
||||
f"Invalid repeat value {repeat!r}: use an integer, "
|
||||
f"'forever', or 'once'."
|
||||
)
|
||||
if repeat is not None and repeat <= 0:
|
||||
repeat = None
|
||||
# String forms ('forever'/'once'/numeric) coerce via
|
||||
# normalize_repeat_value — the shared chokepoint with update paths
|
||||
# (#66824/#64520/#7142/#71987/#95706).
|
||||
repeat = normalize_repeat_value(repeat)
|
||||
|
||||
# Auto-set repeat=1 for one-shot schedules if not specified
|
||||
if parsed_schedule["kind"] == "once" and repeat is None:
|
||||
@@ -2526,6 +2537,25 @@ def update_job(job_id: str, updates: Dict[str, Any]) -> Optional[Dict[str, Any]]
|
||||
updates["reasoning_effort"]
|
||||
)
|
||||
|
||||
# Normalize repeat the same way create_job does. Callers pass
|
||||
# either the stored dict shape ({"times": N, "completed": M}) or
|
||||
# a bare value ("forever", "once", 3, "3"); bare values coerce
|
||||
# through normalize_repeat_value and preserve the completed
|
||||
# counter. A raw string stored here previously broke
|
||||
# mark_job_run ('str' has no .get) and repeat accounting.
|
||||
if "repeat" in updates:
|
||||
_rp = updates["repeat"]
|
||||
if isinstance(_rp, dict):
|
||||
_rp = dict(_rp)
|
||||
_rp["times"] = normalize_repeat_value(_rp.get("times"))
|
||||
_rp.setdefault("completed", (job.get("repeat") or {}).get("completed", 0))
|
||||
updates["repeat"] = _rp
|
||||
else:
|
||||
updates["repeat"] = {
|
||||
"times": normalize_repeat_value(_rp),
|
||||
"completed": (job.get("repeat") or {}).get("completed", 0),
|
||||
}
|
||||
|
||||
previous_inference_axes = _normalized_inference_axes(job)
|
||||
updated = _apply_skill_fields({**job, **updates})
|
||||
|
||||
|
||||
@@ -412,6 +412,32 @@ class TestJobCRUD:
|
||||
with pytest.raises(ValueError, match="Invalid repeat"):
|
||||
create_job(prompt="Bad", schedule="every 1h", repeat="banana")
|
||||
|
||||
def test_update_repeat_string_forms_coerced(self, tmp_cron_dir):
|
||||
"""The UPDATE path must coerce repeat the same way create does.
|
||||
|
||||
Before this fix, update_job({"repeat": "forever"}) stored the raw
|
||||
string, and the next mark_job_run died with
|
||||
"'str' object has no attribute 'get'". Same class as the create-path
|
||||
TypeError (#66824/#64520/#7142/#71987/#95706) — the bare-value and
|
||||
dict shapes both route through normalize_repeat_value now.
|
||||
"""
|
||||
from cron.jobs import mark_job_run, update_job
|
||||
|
||||
job = create_job(prompt="t", schedule="every 1h", repeat=2)
|
||||
mark_job_run(job["id"], success=True) # completed=1
|
||||
|
||||
updated = update_job(job["id"], {"repeat": "forever"})
|
||||
assert updated["repeat"]["times"] is None
|
||||
assert updated["repeat"]["completed"] == 1 # counter preserved
|
||||
mark_job_run(job["id"], success=True) # must not raise
|
||||
|
||||
updated = update_job(job["id"], {"repeat": {"times": "3"}})
|
||||
assert updated["repeat"]["times"] == 3
|
||||
assert updated["repeat"]["completed"] == 2
|
||||
|
||||
with pytest.raises(ValueError, match="Invalid repeat"):
|
||||
update_job(job["id"], {"repeat": "banana"})
|
||||
|
||||
def test_rejects_stale_past_one_shot_at_creation(self, tmp_cron_dir, monkeypatch):
|
||||
now = datetime(2026, 3, 18, 4, 30, 0, tzinfo=timezone.utc)
|
||||
monkeypatch.setattr("cron.jobs._hermes_now", lambda: now)
|
||||
|
||||
@@ -1917,8 +1917,12 @@ def cronjob(
|
||||
)
|
||||
updates["no_agent"] = target_no_agent
|
||||
if repeat is not None:
|
||||
# Normalize: treat 0 or negative as None (infinite)
|
||||
normalized_repeat = None if repeat <= 0 else repeat
|
||||
# Coerce string forms ('forever'/'once'/'3') and 0/negative
|
||||
# via the shared chokepoint — a bare `repeat <= 0` here
|
||||
# raised TypeError for string repeats on the UPDATE path
|
||||
# (create was fixed first; same class).
|
||||
from cron.jobs import normalize_repeat_value
|
||||
normalized_repeat = normalize_repeat_value(repeat)
|
||||
repeat_state = dict(job.get("repeat") or {})
|
||||
repeat_state["times"] = normalized_repeat
|
||||
updates["repeat"] = repeat_state
|
||||
|
||||
Reference in New Issue
Block a user