From e8b757845de948762eced5b4457259fbcb8205b3 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 10 Jun 2026 05:25:35 -0700 Subject: [PATCH] =?UTF-8?q?fix(cron-recipes):=20pre-release=20hardening=20?= =?UTF-8?q?=E2=80=94=20honest=20cadences,=20strict=20slot=20names,=20surfa?= =?UTF-8?q?ce-aware=20UX?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for the Cron Recipes stack before release: - hydration-move: */90 in the cron minute field silently wraps to hourly (croniter-verified) — 90/120-minute options never fired at their stated cadence. Replaced with an hour-field step (0 9-17/2 * * 1-5) and an interval_hours slot whose options (1/2/3h) all fire as labeled. - fill_recipe: reject unknown slot names. A typo'd 'tiem=07:15' used to silently create the job at the 08:00 default; now it 422s on the dashboard form and errors on the slash/deep-link paths with the valid slot list. - deliver slot: non-strict enum (options are suggestions, scheduler validates downstream) so slack/whatsapp/etc. users aren't locked out; GET /api/cron/recipes rewrites its options from cron_delivery_targets() so the dashboard form only offers configured platforms; help text no longer claims dashboard-created jobs deliver to 'the chat you set this up from' (the endpoint strips origin — they go to the home channel). - gateway: success/accept messages no longer point at /cron (cli_only); surface-aware hint instead. Conversational fill now sends the 'Setting up X — I'll ask you a couple of things…' ack before the agent turn, matching the CLI experience. - important-mail catalog entry: reference the urgency classifier by module path (python3 -m cron.scripts.classify_items) instead of baking an absolute host path into the job prompt — stale after relocation and nonexistent on remote terminal backends. cron/scripts is now a real package and ships in the wheel (pyproject packages.find). - export_recipe: interval schedules round-trip again — parse_schedule stores 'minutes' but the renderer only read 'seconds', so every interval job exported as the silent '0 9 * * *' fallback. - skills_hub install: say so when a recipe suggestion is dropped (latched dedup or pending cap) instead of printing nothing. Targeted tests: 58 cron/recipe + 261 web_server pass; E2E-validated all 14 recipes fill+parse, hydration cadences via croniter, typo rejection on slash + endpoint paths, surface-aware hints, and interval export round-trip. --- cron/recipe_catalog.py | 41 +++++++++++++++++++++++++------ cron/scripts/__init__.py | 1 + cron/suggestion_catalog.py | 6 +++-- gateway/run.py | 16 ++++++++++-- hermes_cli/cron_recipe_cmd.py | 16 ++++++++++-- hermes_cli/skills_hub.py | 14 +++++++++++ hermes_cli/suggestions_cmd.py | 12 +++++++-- hermes_cli/web_server.py | 26 ++++++++++++++++++-- pyproject.toml | 2 +- tests/cron/test_recipe_catalog.py | 29 +++++++++++++++++++++- tests/cron/test_suggestions.py | 7 +++++- tests/tools/test_cron_recipes.py | 19 ++++++++++++++ tools/recipes.py | 22 +++++++++++------ web/src/lib/api.ts | 2 ++ 14 files changed, 185 insertions(+), 28 deletions(-) create mode 100644 cron/scripts/__init__.py diff --git a/cron/recipe_catalog.py b/cron/recipe_catalog.py index 0e7a3e1280a..c96b05a7d8e 100644 --- a/cron/recipe_catalog.py +++ b/cron/recipe_catalog.py @@ -68,6 +68,11 @@ class RecipeSlot: options: tuple = () # for type="enum": allowed values optional: bool = False help: str = "" + # When False, ``options`` are suggestions rather than a closed set — + # any value is accepted (e.g. the deliver slot, where the real set of + # valid platforms depends on the user's configured gateways and is + # validated downstream by the cron scheduler). + strict: bool = True def __post_init__(self) -> None: if self.type not in _SLOT_TYPES: @@ -105,7 +110,10 @@ _TIME = lambda default="08:00": RecipeSlot( # noqa: E731 - concise factory _DELIVER = RecipeSlot( name="deliver", type="enum", label="Where to deliver?", default="origin", options=("origin", "local", "telegram", "discord", "email"), - help="origin = the chat you set this up from; local = save only, no message", + optional=False, strict=False, + help="origin = the chat you set this up from (or your configured home " + "channel when created from the dashboard); local = save only, no message; " + "or any connected platform name", ) @@ -324,7 +332,10 @@ CATALOG: List[CronRecipe] = [ description="A periodic nudge during the day to drink water, stand up, " "and stretch.", category="general", - schedule_template="*/{interval_min} {start_hour}-{end_hour} * * 1-5", + # NOTE: cron minute-field steps (*/90) wrap per hour — */90 and */120 + # both degrade to hourly. Use an hour-field step instead so the chosen + # cadence is what actually fires. + schedule_template="0 {start_hour}-{end_hour}/{interval_hours} * * 1-5", prompt_template=( "Send the user a brief, friendly nudge to drink some water, stand " "up, and stretch for a moment. Vary the wording each time so it " @@ -332,9 +343,9 @@ CATALOG: List[CronRecipe] = [ ), slots=[ RecipeSlot( - name="interval_min", type="enum", label="How often?", - default="90", options=("60", "90", "120"), - help="minutes between nudges", + name="interval_hours", type="enum", label="How often?", + default="1", options=("1", "2", "3"), + help="hours between nudges", ), RecipeSlot( name="start_hour", type="enum", label="Start hour", @@ -494,6 +505,7 @@ def recipe_form_schema(recipe: CronRecipe) -> Dict[str, Any]: "default": s.default, "options": list(s.options), "optional": s.optional, + "strict": s.strict, "help": s.help, } for s in recipe.slots @@ -543,6 +555,11 @@ def _humanize_schedule(recipe: CronRecipe) -> str: iv = next((s for s in recipe.slots if s.name == "interval_min"), None) every = (iv.default if iv else None) or sched.split("/")[1].split()[0] return f"every {every} minutes" + if "{interval_hours}" in sched: + iv = next((s for s in recipe.slots if s.name == "interval_hours"), None) + every = str((iv.default if iv else None) or "1") + scope = "weekdays, " if "* * 1-5" in sched else "" + return f"{scope}every hour" if every == "1" else f"{scope}every {every} hours" time_slot = next((s for s in recipe.slots if s.type == "time"), None) when = time_slot.default if time_slot else None if "* * 1-5" in sched: @@ -651,9 +668,17 @@ def fill_recipe( Missing required (non-optional) slots raise RecipeFillError naming the slot, so a form can show field errors and the agent knows what to ask. - Enum values are checked against their options. The result is passed - straight to ``create_job`` — no second schema. + Unknown slot names are rejected (a typo'd ``tiem=07:15`` must not silently + create a job with the default time). Enum values are checked against their + options. The result is passed straight to ``create_job`` — no second schema. """ + known = {s.name for s in recipe.slots} + unknown = sorted(set(values) - known) + if unknown: + raise RecipeFillError( + f"unknown slot{'s' if len(unknown) > 1 else ''}: " + f"{', '.join(unknown)} — valid: {', '.join(s.name for s in recipe.slots)}" + ) resolved: Dict[str, Any] = {} for s in recipe.slots: raw = values.get(s.name, s.default) @@ -661,7 +686,7 @@ def fill_recipe( if s.optional: continue raise RecipeFillError(f"missing required value: {s.name} ({s.label})") - if s.type == "enum" and s.options and str(raw) not in {str(o) for o in s.options}: + if s.type == "enum" and s.strict and s.options and str(raw) not in {str(o) for o in s.options}: raise RecipeFillError( f"{s.name}={raw!r} not allowed — one of {', '.join(map(str, s.options))}" ) diff --git a/cron/scripts/__init__.py b/cron/scripts/__init__.py new file mode 100644 index 00000000000..8dc1a5c3c51 --- /dev/null +++ b/cron/scripts/__init__.py @@ -0,0 +1 @@ +"""Scripts shipped with the cron subsystem (runnable via ``python3 -m cron.scripts.``).""" diff --git a/cron/suggestion_catalog.py b/cron/suggestion_catalog.py index e297bc440a0..deacf1e1349 100644 --- a/cron/suggestion_catalog.py +++ b/cron/suggestion_catalog.py @@ -71,8 +71,10 @@ CATALOG: List[CatalogEntry] = [ "For each candidate, judge urgency against this rule: surface " "only mail that needs a reply today, is from a manager/family " "member, or mentions a deadline. Pipe candidates through the " - f"urgency classifier at {classify_items_script_path()} " - "(--threshold 7) and deliver ONLY what it returns. If nothing " + "urgency classifier (run `python3 -m cron.scripts.classify_items " + "--threshold 7 --criteria ...` from the hermes-agent install — " + "resolve the script path at run time, do not assume a fixed " + "location) and deliver ONLY what it returns. If nothing " "clears the bar, respond with [SILENT] so the user is not " "pinged. Requires a connected mail source; if none is " "configured, explain how to connect one and then stop." diff --git a/gateway/run.py b/gateway/run.py index e161d8ed13b..cd91f3d3730 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -7183,6 +7183,18 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew # user for each slot value conversationally and then calls the # cronjob tool (the /steer fall-through pattern). The seed # enters as a normal user turn, preserving role alternation. + # Send the "Setting up X…" ack first so the user gets the same + # immediate feedback CLI users see, instead of silence until + # the agent's first question. + _ack = getattr(_recipe_result, "text", "") or "" + if _ack: + try: + adapter = self.adapters.get(source.platform) + if adapter: + _ack_meta = self._thread_metadata_for_source(source) + await adapter.send(str(source.chat_id), _ack, metadata=_ack_meta) + except Exception: + logger.debug("cron-recipe ack send failed", exc_info=True) try: event.text = _recipe_seed except Exception: @@ -9281,7 +9293,7 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew try: from hermes_cli.suggestions_cmd import handle_suggestions_command - return handle_suggestions_command(args, origin=origin) + return handle_suggestions_command(args, origin=origin, surface="gateway") except Exception as e: logger.debug("suggestions command failed: %s", e) return f"Suggestions command failed: {e}" @@ -9314,7 +9326,7 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew try: from hermes_cli.cron_recipe_cmd import handle_cron_recipe_command - return handle_cron_recipe_command(args, origin=origin) + return handle_cron_recipe_command(args, origin=origin, surface="gateway") except Exception as e: logger.debug("cron-recipe command failed: %s", e) from hermes_cli.cron_recipe_cmd import RecipeCommandResult diff --git a/hermes_cli/cron_recipe_cmd.py b/hermes_cli/cron_recipe_cmd.py index 0f97c69ff28..4927c9d8d17 100644 --- a/hermes_cli/cron_recipe_cmd.py +++ b/hermes_cli/cron_recipe_cmd.py @@ -234,10 +234,20 @@ def _fmt_no_match(query: str) -> str: return msg +def _manage_hint(surface: str) -> str: + """Post-create management hint. /cron is a CLI-only slash command; on + gateway platforms the user manages jobs by asking the agent (cronjob tool) + or from the dashboard.""" + if surface == "cli": + return "Manage it with /cron." + return "Ask me to list, pause, or remove it any time." + + def handle_cron_recipe_command( args: str, *, origin: Optional[Dict[str, Any]] = None, + surface: str = "cli", ) -> RecipeCommandResult: """Dispatch a ``/cron-recipe`` invocation. @@ -246,7 +256,9 @@ def handle_cron_recipe_command( command is fully handled and only ``text`` is shown. ``args`` is everything after ``/cron-recipe``. ``origin`` lets a directly - created job deliver back to the chat it was set up from. + created job deliver back to the chat it was set up from. ``surface`` + (``"cli"`` | ``"gateway"``) picks the right wording for follow-up hints — + ``/cron`` only exists on the CLI. """ try: from cron.recipe_catalog import fill_recipe, RecipeFillError @@ -302,5 +314,5 @@ def handle_cron_recipe_command( return RecipeCommandResult( f"Scheduled '{recipe.title}'" + (f" ({sched})" if sched else "") - + f", delivering to {spec.get('deliver', 'origin')}. Manage it with /cron." + + f", delivering to {spec.get('deliver', 'origin')}. {_manage_hint(surface)}" ) diff --git a/hermes_cli/skills_hub.py b/hermes_cli/skills_hub.py index 2b7546962f6..f6f70b288d4 100644 --- a/hermes_cli/skills_hub.py +++ b/hermes_cli/skills_hub.py @@ -715,6 +715,20 @@ def do_install(identifier: str, category: str = "", force: bool = False, "[dim]Added to your suggestions — run[/] [bold]/suggestions[/] " "[dim]to schedule or dismiss it.[/]\n" ) + else: + # Dropped: already offered/dismissed (latched) or the pending + # list is at its cap. Say so instead of silently doing nothing — + # the user can still schedule it by hand. + c.print( + f"[bold cyan]Recipe:[/] '{bundle.name}' is an automation " + f"(schedule [bold]{spec.schedule}[/]), but it wasn't added to " + "your suggestions (already offered/dismissed, or the pending " + "list is full — run [bold]/suggestions[/] to review)." + ) + c.print( + "[dim]You can still schedule it any time by asking the agent " + "or via[/] [bold]hermes cron add[/][dim].[/]\n" + ) except Exception: # pragma: no cover - recipe detection is best-effort pass diff --git a/hermes_cli/suggestions_cmd.py b/hermes_cli/suggestions_cmd.py index a0f785016a5..aa336e37a19 100644 --- a/hermes_cli/suggestions_cmd.py +++ b/hermes_cli/suggestions_cmd.py @@ -67,13 +67,16 @@ def handle_suggestions_command( args: str, *, origin: Optional[Dict[str, Any]] = None, + surface: str = "cli", ) -> str: """Dispatch a ``/suggestions`` invocation. Returns text to show the user. ``args`` is everything after ``/suggestions`` (already stripped of the command word). ``origin`` is the platform/chat dict so an accepted job's "origin" delivery routes back to where the user accepted; when omitted it - is resolved from the session environment. + is resolved from the session environment. ``surface`` (``"cli"`` | + ``"gateway"``) picks the wording for follow-up hints — ``/cron`` only + exists on the CLI. """ if origin is None: origin = _resolve_origin() @@ -99,10 +102,15 @@ def handle_suggestions_command( return f"No pending suggestion matches '{rest}'. Run /suggestions to list them." sched = job.get("schedule_display") or (job.get("job_spec", {}) or {}).get("schedule", "") name = job.get("name", "automation") + manage = ( + "Manage it with /cron." + if surface == "cli" + else "Ask me to list, pause, or remove it any time." + ) return ( f"Scheduled '{name}'" + (f" ({sched})" if sched else "") - + ". Manage it with /cron." + + f". {manage}" ) if sub in ("dismiss", "no", "reject"): diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 05586231820..ffafc93d4bc 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -6790,11 +6790,33 @@ class CronRecipeInstantiate(BaseModel): @app.get("/api/cron/recipes") async def list_cron_recipes(): - """Return the recipe catalog as form schemas for the dashboard gallery.""" + """Return the recipe catalog as form schemas for the dashboard gallery. + + The ``deliver`` slot's options are rewritten from the user's actually + configured gateway platforms (plus the universal origin/local/all), so the + form never offers a platform that isn't connected. + """ try: from cron.recipe_catalog import CATALOG, recipe_catalog_entry - return {"recipes": [recipe_catalog_entry(r) for r in CATALOG]} + deliver_options = None + try: + from cron.scheduler import cron_delivery_targets + + platforms = [t["id"] for t in cron_delivery_targets() if t.get("id")] + deliver_options = ["origin", "local", *platforms] + except Exception: + _log.debug("cron_delivery_targets unavailable; using static deliver options", exc_info=True) + + entries = [] + for r in CATALOG: + entry = recipe_catalog_entry(r) + if deliver_options: + for f in entry.get("fields", []): + if f.get("name") == "deliver": + f["options"] = deliver_options + entries.append(entry) + return {"recipes": entries} except Exception as e: _log.exception("GET /api/cron/recipes failed") raise HTTPException(status_code=500, detail=str(e)) diff --git a/pyproject.toml b/pyproject.toml index e5bf882d87a..e191932c285 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -319,7 +319,7 @@ plugins = [ ] [tool.setuptools.packages.find] -include = ["agent", "agent.*", "tools", "tools.*", "hermes_cli", "hermes_cli.*", "gateway", "gateway.*", "tui_gateway", "tui_gateway.*", "cron", "acp_adapter", "plugins", "plugins.*", "providers", "providers.*"] +include = ["agent", "agent.*", "tools", "tools.*", "hermes_cli", "hermes_cli.*", "gateway", "gateway.*", "tui_gateway", "tui_gateway.*", "cron", "cron.*", "acp_adapter", "plugins", "plugins.*", "providers", "providers.*"] [tool.pytest.ini_options] testpaths = ["tests"] diff --git a/tests/cron/test_recipe_catalog.py b/tests/cron/test_recipe_catalog.py index 7ccc8f79aad..017d6bd2ae1 100644 --- a/tests/cron/test_recipe_catalog.py +++ b/tests/cron/test_recipe_catalog.py @@ -80,7 +80,34 @@ class TestValidation: def test_bad_enum_rejected_and_names_slot(self): with pytest.raises(RecipeFillError, match="not allowed"): - fill_recipe(get_recipe("morning-brief"), {"time": "08:00", "deliver": "pigeon"}) + fill_recipe(get_recipe("news-digest"), {"count": "42"}) + + def test_deliver_slot_accepts_any_platform(self): + # deliver is a non-strict enum: its options are suggestions, the real + # set of valid platforms depends on the user's configured gateways and + # is validated downstream by the cron scheduler. + spec = fill_recipe(get_recipe("morning-brief"), {"time": "08:00", "deliver": "slack"}) + assert spec["deliver"] == "slack" + + def test_unknown_slot_name_rejected(self): + # A typo'd slot must NOT silently create a job with the default value. + with pytest.raises(RecipeFillError, match="unknown slot"): + fill_recipe(get_recipe("morning-brief"), {"tiem": "07:15"}) + + def test_hydration_hourly_step_actually_fires_at_chosen_cadence(self): + # Regression: a minute-field step (*/90) silently wraps to hourly. + # The hour-field step form must produce the cadence the user picked. + croniter = pytest.importorskip("croniter").croniter + from datetime import datetime + + spec = fill_recipe(get_recipe("hydration-move"), {"interval_hours": "2"}) + it = croniter(spec["schedule"], datetime(2026, 6, 10, 8, 0)) + first_three = [it.get_next(datetime) for _ in range(3)] + gaps = { + (b - a).total_seconds() + for a, b in zip(first_three, first_three[1:]) + } + assert gaps == {7200.0}, f"expected 2h gaps, got {spec['schedule']} -> {first_three}" def test_text_slot_renders_into_prompt(self): spec = fill_recipe( diff --git a/tests/cron/test_suggestions.py b/tests/cron/test_suggestions.py index b8db8f54dac..179c2956623 100644 --- a/tests/cron/test_suggestions.py +++ b/tests/cron/test_suggestions.py @@ -127,7 +127,12 @@ class TestCatalog: from cron.suggestion_catalog import CATALOG, classify_items_script_path monitor = next(e for e in CATALOG if e.key == "catalog:important-mail-monitor") - assert classify_items_script_path() in monitor.job_spec["prompt"] + # The prompt must reference the classifier by module path (resolvable + # at run time on any backend), never by a baked-in absolute path — + # absolute paths go stale after relocation and don't exist on remote + # terminal backends (Docker/Modal). + assert "cron.scripts.classify_items" in monitor.job_spec["prompt"] + assert classify_items_script_path() not in monitor.job_spec["prompt"] assert Path(classify_items_script_path()).name == "classify_items.py" diff --git a/tests/tools/test_cron_recipes.py b/tests/tools/test_cron_recipes.py index 7d9f89442f2..439b9604913 100644 --- a/tests/tools/test_cron_recipes.py +++ b/tests/tools/test_cron_recipes.py @@ -167,3 +167,22 @@ class TestExportRecipe: md = export_recipe(job, "body") assert "recipe" in md assert "automation" in md + + def test_export_interval_job_without_display(self): + # Regression: parse_schedule stores interval periods as "minutes" — + # exporting a job with only the parsed schedule dict must round-trip + # the real interval, not fall back to the daily default. + job = { + "name": "poller", + "schedule": {"kind": "interval", "minutes": 30}, + "skills": ["poller"], + } + md = export_recipe(job, "body") + spec = parse_recipe(md) + assert spec is not None + assert spec.schedule == "every 30m" + + job["schedule"] = {"kind": "interval", "minutes": 120} + spec = parse_recipe(export_recipe(job, "body")) + assert spec is not None + assert spec.schedule == "every 2h" diff --git a/tools/recipes.py b/tools/recipes.py index 014720801e3..1b95f192802 100644 --- a/tools/recipes.py +++ b/tools/recipes.py @@ -307,11 +307,19 @@ def _schedule_to_string(schedule: Any) -> str: kind = schedule.get("kind") if kind == "cron" and schedule.get("expr"): return str(schedule["expr"]) - if kind == "interval" and schedule.get("seconds"): - secs = int(schedule["seconds"]) - if secs % 3600 == 0: - return f"every {secs // 3600}h" - if secs % 60 == 0: - return f"every {secs // 60}m" - return f"every {secs}s" + if kind == "interval": + # parse_schedule stores interval periods as "minutes"; tolerate a + # legacy/foreign "seconds" form too. + if schedule.get("minutes"): + mins = int(schedule["minutes"]) + if mins % 60 == 0: + return f"every {mins // 60}h" + return f"every {mins}m" + if schedule.get("seconds"): + secs = int(schedule["seconds"]) + if secs % 3600 == 0: + return f"every {secs // 3600}h" + if secs % 60 == 0: + return f"every {secs // 60}m" + return f"every {secs}s" return "0 9 * * *" # safe daily fallback diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 4f9b7461151..f306a43a485 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -1845,6 +1845,8 @@ export interface CronRecipeField { default: string | null; options: string[]; optional: boolean; + /** When false, options are suggestions — any value is accepted. */ + strict?: boolean; help: string; }