fix(cron): accept list-form deliver values so deliver=['telegram'] works (#17456)
The cron schema contracts deliver as a string ("local", "origin",
"telegram", "telegram:chat_id[:thread_id]", or comma-separated combos),
but MCP clients and scripts sometimes pass an array like ['telegram'].
Before this change, the list was written to jobs.json verbatim, and
the scheduler's str(deliver).split(',') then tried to resolve the
literal string "['telegram']" as a platform — returning None and
logging 'no delivery target resolved for deliver=[\'telegram\']'.
Fix on both ends:
- tools/cronjob_tools.py: normalize deliver at the API boundary on
create and update, so storage is always a string.
- cron/scheduler.py: normalize deliver in _resolve_delivery_targets,
so existing jobs.json entries with list-form deliver are handled
gracefully without requiring users to edit the file.
Closes #17139
This commit is contained in:
parent
7141cda967
commit
398945e7b1
@ -233,12 +233,32 @@ def _resolve_single_delivery_target(job: dict, deliver_value: str) -> Optional[d
|
||||
}
|
||||
|
||||
|
||||
def _normalize_deliver_value(deliver) -> str:
|
||||
"""Normalize a stored/submitted ``deliver`` value to its canonical string form.
|
||||
|
||||
The contract is that ``deliver`` is a string (``"local"``, ``"origin"``,
|
||||
``"telegram"``, ``"telegram:-1001:17"``, or comma-separated combinations).
|
||||
Historically some callers — MCP clients passing an array, direct edits of
|
||||
``jobs.json``, or stale code paths — have stored a list/tuple like
|
||||
``["telegram"]``. ``str(["telegram"])`` would serialize to the literal
|
||||
string ``"['telegram']"``, which is not a known platform and fails
|
||||
resolution silently. Flatten lists/tuples into a comma-separated string
|
||||
so both forms work. Returns ``"local"`` for anything falsy.
|
||||
"""
|
||||
if deliver is None or deliver == "":
|
||||
return "local"
|
||||
if isinstance(deliver, (list, tuple)):
|
||||
parts = [str(p).strip() for p in deliver if str(p).strip()]
|
||||
return ",".join(parts) if parts else "local"
|
||||
return str(deliver)
|
||||
|
||||
|
||||
def _resolve_delivery_targets(job: dict) -> List[dict]:
|
||||
"""Resolve all concrete auto-delivery targets for a cron job (supports comma-separated deliver)."""
|
||||
deliver = job.get("deliver", "local")
|
||||
deliver = _normalize_deliver_value(job.get("deliver", "local"))
|
||||
if deliver == "local":
|
||||
return []
|
||||
parts = [p.strip() for p in str(deliver).split(",") if p.strip()]
|
||||
parts = [p.strip() for p in deliver.split(",") if p.strip()]
|
||||
seen = set()
|
||||
targets = []
|
||||
for part in parts:
|
||||
|
||||
@ -279,6 +279,44 @@ class TestResolveDeliveryTarget:
|
||||
"thread_id": None,
|
||||
}
|
||||
|
||||
def test_list_form_deliver_is_normalized(self, monkeypatch):
|
||||
"""deliver=['telegram'] (Python list) should resolve like 'telegram' string.
|
||||
|
||||
Regression test for #17139: MCP clients / scripts that pass the deliver
|
||||
field as an array-shaped value used to fail with "no delivery target
|
||||
resolved for deliver=['telegram']" because ``str(['telegram'])`` was
|
||||
passed through to ``split(',')`` verbatim.
|
||||
"""
|
||||
monkeypatch.setenv("TELEGRAM_HOME_CHANNEL", "-4004")
|
||||
job = {
|
||||
"deliver": ["telegram"],
|
||||
"origin": None,
|
||||
}
|
||||
|
||||
assert _resolve_delivery_target(job) == {
|
||||
"platform": "telegram",
|
||||
"chat_id": "-4004",
|
||||
"thread_id": None,
|
||||
}
|
||||
|
||||
def test_list_form_multiple_platforms_normalized(self, monkeypatch):
|
||||
"""deliver=['telegram', 'discord'] resolves to multiple targets."""
|
||||
from cron.scheduler import _resolve_delivery_targets
|
||||
|
||||
monkeypatch.setenv("TELEGRAM_HOME_CHANNEL", "-111")
|
||||
monkeypatch.setenv("DISCORD_HOME_CHANNEL", "-222")
|
||||
job = {"deliver": ["telegram", "discord"], "origin": None}
|
||||
|
||||
targets = _resolve_delivery_targets(job)
|
||||
platforms = sorted(t["platform"] for t in targets)
|
||||
assert platforms == ["discord", "telegram"]
|
||||
|
||||
def test_empty_list_form_deliver_resolves_to_local(self):
|
||||
"""deliver=[] is treated as local (no delivery)."""
|
||||
from cron.scheduler import _resolve_delivery_targets
|
||||
|
||||
assert _resolve_delivery_targets({"deliver": []}) == []
|
||||
|
||||
|
||||
class TestDeliverResultWrapping:
|
||||
"""Verify that cron deliveries are wrapped with header/footer and no longer mirrored."""
|
||||
|
||||
@ -231,3 +231,60 @@ class TestUnifiedCronjobTool:
|
||||
assert updated["success"] is True
|
||||
assert updated["job"]["skills"] == []
|
||||
assert updated["job"]["skill"] is None
|
||||
|
||||
def test_create_normalizes_list_form_deliver(self):
|
||||
"""deliver=['telegram'] (list) is stored as the string 'telegram'.
|
||||
|
||||
Regression for #17139: MCP clients / scripts sometimes pass ``deliver``
|
||||
as an array. Prior to the fix, ``['telegram']`` was written verbatim
|
||||
to ``jobs.json`` and the scheduler then tried to resolve the literal
|
||||
string ``"['telegram']"`` as a platform, failing with
|
||||
"no delivery target resolved".
|
||||
"""
|
||||
from cron.jobs import get_job
|
||||
|
||||
created = json.loads(
|
||||
cronjob(
|
||||
action="create",
|
||||
prompt="Daily briefing",
|
||||
schedule="every 1h",
|
||||
deliver=["telegram"],
|
||||
)
|
||||
)
|
||||
assert created["success"] is True
|
||||
stored = get_job(created["job_id"])
|
||||
assert stored["deliver"] == "telegram"
|
||||
|
||||
def test_create_normalizes_multi_element_list_deliver(self):
|
||||
"""deliver=['telegram', 'discord'] is stored as 'telegram,discord'."""
|
||||
from cron.jobs import get_job
|
||||
|
||||
created = json.loads(
|
||||
cronjob(
|
||||
action="create",
|
||||
prompt="Daily briefing",
|
||||
schedule="every 1h",
|
||||
deliver=["telegram", "discord"],
|
||||
)
|
||||
)
|
||||
assert created["success"] is True
|
||||
stored = get_job(created["job_id"])
|
||||
assert stored["deliver"] == "telegram,discord"
|
||||
|
||||
def test_update_normalizes_list_form_deliver(self):
|
||||
"""update with deliver=['telegram'] stores the canonical string."""
|
||||
from cron.jobs import get_job
|
||||
|
||||
created = json.loads(
|
||||
cronjob(action="create", prompt="x", schedule="every 1h")
|
||||
)
|
||||
updated = json.loads(
|
||||
cronjob(
|
||||
action="update",
|
||||
job_id=created["job_id"],
|
||||
deliver=["telegram"],
|
||||
)
|
||||
)
|
||||
assert updated["success"] is True
|
||||
stored = get_job(created["job_id"])
|
||||
assert stored["deliver"] == "telegram"
|
||||
|
||||
@ -150,6 +150,27 @@ def _normalize_optional_job_value(value: Optional[Any], *, strip_trailing_slash:
|
||||
return text or None
|
||||
|
||||
|
||||
def _normalize_deliver_param(value: Any) -> Optional[str]:
|
||||
"""Normalize a user-supplied ``deliver`` value to the canonical string form.
|
||||
|
||||
The cron schema documents ``deliver`` as a string (``"local"``, ``"origin"``,
|
||||
``"telegram"``, ``"telegram:chat_id[:thread_id]"``, or comma-separated combos).
|
||||
Some callers — MCP clients passing arrays, scripts building the payload as a
|
||||
list — supply ``["telegram"]``. ``create_job``/``update_job`` store it as-is,
|
||||
and the scheduler's ``str(deliver).split(",")`` then serializes the list to
|
||||
the literal ``"['telegram']"`` which is not a known platform. Flatten lists
|
||||
/ tuples at the API boundary so storage is always a string. Returns ``None``
|
||||
for ``None``/empty so callers can treat it as "not supplied".
|
||||
"""
|
||||
if value is None:
|
||||
return None
|
||||
if isinstance(value, (list, tuple)):
|
||||
parts = [str(p).strip() for p in value if str(p).strip()]
|
||||
return ",".join(parts) if parts else None
|
||||
text = str(value).strip()
|
||||
return text or None
|
||||
|
||||
|
||||
def _validate_cron_script_path(script: Optional[str]) -> Optional[str]:
|
||||
"""Validate a cron job script path at the API boundary.
|
||||
|
||||
@ -283,7 +304,7 @@ def cronjob(
|
||||
schedule=schedule,
|
||||
name=name,
|
||||
repeat=repeat,
|
||||
deliver=deliver,
|
||||
deliver=_normalize_deliver_param(deliver),
|
||||
origin=_origin_from_env(),
|
||||
skills=canonical_skills,
|
||||
model=_normalize_optional_job_value(model),
|
||||
@ -364,7 +385,7 @@ def cronjob(
|
||||
if name is not None:
|
||||
updates["name"] = name
|
||||
if deliver is not None:
|
||||
updates["deliver"] = deliver
|
||||
updates["deliver"] = _normalize_deliver_param(deliver)
|
||||
if skills is not None or skill is not None:
|
||||
canonical_skills = _canonical_skills(skill, skills)
|
||||
updates["skills"] = canonical_skills
|
||||
|
||||
Loading…
Reference in New Issue
Block a user