mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-12 08:51:53 +00:00
fix(cron-recipes): pre-release hardening — honest cadences, strict slot names, surface-aware UX
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.
This commit is contained in:
parent
e976faac7a
commit
e8b757845d
14 changed files with 185 additions and 28 deletions
|
|
@ -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))}"
|
||||
)
|
||||
|
|
|
|||
1
cron/scripts/__init__.py
Normal file
1
cron/scripts/__init__.py
Normal file
|
|
@ -0,0 +1 @@
|
|||
"""Scripts shipped with the cron subsystem (runnable via ``python3 -m cron.scripts.<name>``)."""
|
||||
|
|
@ -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."
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)}"
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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"):
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue