diff --git a/cron/scheduler.py b/cron/scheduler.py index 3b38a20336..d41c7ed860 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -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: diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index 23565511cf..b5754bd8c7 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -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.""" diff --git a/tests/tools/test_cronjob_tools.py b/tests/tools/test_cronjob_tools.py index 38fc12cc8c..ab6f8eef08 100644 --- a/tests/tools/test_cronjob_tools.py +++ b/tests/tools/test_cronjob_tools.py @@ -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" diff --git a/tools/cronjob_tools.py b/tools/cronjob_tools.py index 994c313623..53e778a7db 100644 --- a/tools/cronjob_tools.py +++ b/tools/cronjob_tools.py @@ -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