Merge PR #3125: fix: harden cron tool contract

fix: harden cron tool contract

nanobot/agent/tools/cron.py

@@ -5,41 +5,71 @@ from datetime import datetime
 from typing import Any
 
 from nanobot.agent.tools.base import Tool, tool_parameters
-from nanobot.agent.tools.schema import BooleanSchema, IntegerSchema, StringSchema, tool_parameters_schema
+from nanobot.agent.tools.schema import (
+    BooleanSchema,
+    IntegerSchema,
+    StringSchema,
+    tool_parameters_schema,
+)
 from nanobot.cron.service import CronService
 from nanobot.cron.types import CronJob, CronJobState, CronSchedule
 
-
+_CRON_PARAMETERS = tool_parameters_schema(
-@tool_parameters(
+    action=StringSchema("Action to perform", enum=["add", "list", "remove"]),
-    tool_parameters_schema(
+    name=StringSchema(
-        action=StringSchema("Action to perform", enum=["add", "list", "remove"]),
+        "Optional short human-readable label for the job "
-        name=StringSchema(
+        "(e.g., 'weather-monitor', 'daily-standup'). Defaults to first 30 chars of message."
-            "Optional short human-readable label for the job "
+    ),
-            "(e.g., 'weather-monitor', 'daily-standup'). Defaults to first 30 chars of message."
+    message=StringSchema(
-        ),
+        "REQUIRED when action='add'. Instruction for the agent to execute when the job triggers "
-        message=StringSchema(
+        "(e.g., 'Send a reminder to WeChat: xxx' or 'Check system status and report'). "
-            "REQUIRED when action='add'. Instruction for the agent to execute when the job triggers "
+        "Not used for action='list' or action='remove'."
-            "(e.g., 'Send a reminder to WeChat: xxx' or 'Check system status and report'). "
+    ),
-            "Not used for action='list' or action='remove'."
+    every_seconds=IntegerSchema(0, description="Interval in seconds (for recurring tasks)"),
-        ),
+    cron_expr=StringSchema("Cron expression like '0 9 * * *' (for scheduled tasks)"),
-        every_seconds=IntegerSchema(0, description="Interval in seconds (for recurring tasks)"),
+    tz=StringSchema(
-        cron_expr=StringSchema("Cron expression like '0 9 * * *' (for scheduled tasks)"),
+        "Optional IANA timezone for cron expressions (e.g. 'America/Vancouver'). "
-        tz=StringSchema(
+        "When omitted with cron_expr, the tool's default timezone applies."
-            "Optional IANA timezone for cron expressions (e.g. 'America/Vancouver'). "
+    ),
-            "When omitted with cron_expr, the tool's default timezone applies."
+    at=StringSchema(
-        ),
+        "ISO datetime for one-time execution (e.g. '2026-02-12T10:30:00'). "
-        at=StringSchema(
+        "Naive values use the tool's default timezone."
-            "ISO datetime for one-time execution (e.g. '2026-02-12T10:30:00'). "
+    ),
-            "Naive values use the tool's default timezone."
+    deliver=BooleanSchema(
-        ),
+        description="Whether to deliver the execution result to the user channel (default true)",
-        deliver=BooleanSchema(
+        default=True,
-            description="Whether to deliver the execution result to the user channel (default true)",
+    ),
-            default=True,
+    job_id=StringSchema("REQUIRED when action='remove'. Job ID to remove (obtain via action='list')."),
-        ),
+    required=["action"],
-        job_id=StringSchema("REQUIRED when action='remove'. Job ID to remove (obtain via action='list')."),
+    description=(
-        required=["action"],
+        "Action-specific parameters: add requires a non-empty message plus one schedule "
-    )
+        "(every_seconds, cron_expr, or at); remove requires job_id; list only needs action."
+    ),
 )
+_CRON_PARAMETERS["oneOf"] = [
+    {
+        "properties": {
+            "action": {"enum": ["add"]},
+            "message": {"type": "string", "minLength": 1},
+        },
+        "required": ["action", "message"],
+    },
+    {
+        "properties": {
+            "action": {"enum": ["list"]},
+        },
+        "required": ["action"],
+    },
+    {
+        "properties": {
+            "action": {"enum": ["remove"]},
+        },
+        "required": ["action", "job_id"],
+    },
+]
+
+
+@tool_parameters(_CRON_PARAMETERS)
 class CronTool(Tool):
     """Tool to schedule reminders and recurring tasks."""
 
@@ -95,6 +125,15 @@ class CronTool(Tool):
             f"If tz is omitted, cron expressions and naive ISO times default to {self._default_timezone}."
         )
 
+    def validate_params(self, params: dict[str, Any]) -> list[str]:
+        errors = super().validate_params(params)
+        action = params.get("action")
+        if action == "add" and not str(params.get("message") or "").strip():
+            errors.append("message is required when action='add'")
+        if action == "remove" and not str(params.get("job_id") or "").strip():
+            errors.append("job_id is required when action='remove'")
+        return errors
+
     async def execute(
         self,
         action: str,
@@ -130,8 +169,8 @@ class CronTool(Tool):
     ) -> str:
         if not message:
             return (
-                "Error: cron action='add' requires a non-empty 'message' "
+                "Error: cron action='add' requires a non-empty 'message' parameter "
-                "parameter describing what to do when the job triggers "
+                "describing what to do when the job triggers "
                 "(e.g. the reminder text). Retry including message=\"...\"."
             )
         if not self._channel or not self._chat_id:

tests/cron/test_cron_tool_list.py

@@ -7,7 +7,6 @@ import pytest
 from nanobot.agent.tools.cron import CronTool
 from nanobot.cron.service import CronService
 from nanobot.cron.types import CronJob, CronJobState, CronPayload, CronSchedule
-from tests.test_openai_api import pytest_plugins
 
 
 def _make_tool(tmp_path) -> CronTool:
@@ -346,6 +345,47 @@ def test_add_job_can_disable_delivery(tmp_path) -> None:
     assert job.payload.deliver is False
 
 
+def test_cron_schema_advertises_action_specific_requirements(tmp_path) -> None:
+    tool = _make_tool(tmp_path)
+
+    assert tool.parameters["required"] == ["action"]
+    assert tool.parameters["oneOf"] == [
+        {
+            "properties": {
+                "action": {"enum": ["add"]},
+                "message": {"type": "string", "minLength": 1},
+            },
+            "required": ["action", "message"],
+        },
+        {
+            "properties": {"action": {"enum": ["list"]}},
+            "required": ["action"],
+        },
+        {
+            "properties": {"action": {"enum": ["remove"]}},
+            "required": ["action", "job_id"],
+        },
+    ]
+
+
+def test_validate_params_requires_message_only_for_add(tmp_path) -> None:
+    tool = _make_tool(tmp_path)
+
+    assert "message is required when action='add'" in tool.validate_params({"action": "add"})
+    assert tool.validate_params({"action": "list"}) == []
+    assert "job_id is required when action='remove'" in tool.validate_params({"action": "remove"})
+
+
+def test_add_job_empty_message_returns_actionable_error(tmp_path) -> None:
+    tool = _make_tool(tmp_path)
+    tool.set_context("telegram", "chat-1")
+
+    result = tool._add_job(None, "", 60, None, None, None)
+
+    assert "action='add' requires a non-empty 'message'" in result
+    assert "Retry including message=" in result
+
+
 def test_list_excludes_disabled_jobs(tmp_path) -> None:
     tool = _make_tool(tmp_path)
     job = tool._cron.add_job(