From 3163a91d1a6d45927c4dbe6939a135ef3ce554b4 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Sun, 20 Sep 2026 02:19:43 +0800 Subject: [PATCH] # Cron edit/duplicate must round-trip a parseable schedule; bad input returns 400 (#7352) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path The WebUI populates the editable schedule field from `schedule_display`, which is the human-readable label `once at 2026-08-28 16:00` for one-shot jobs. `saveCronForm()` then submits that exact string back to `POST /api/crons/update`. The Agent's `cron.jobs.parse_schedule()` rejects the display form with `ValueError` (it only accepts the canonical ISO timestamp). The route's earlier `try/except ValueError` block covered input normalization but not the `update_job()` call, so the parser exception escaped as HTTP 500. There are two independent failures to fix: 1. **Frontend** ships the unsafe display label as editable input. Both `openCronEdit()` (`static/panels.js:1593`) and `duplicateCurrentCron()` (`static/panels.js:1532`) use the same `schedule_display || schedule.expression` fallback. The cleanest fix is a shared helper that returns the canonical Agent-parseable value by schedule kind, matching the Agent's own schema: - `once` → `schedule.run_at` - recurring cron → `schedule.expr` (current schema; `expression` accepted for legacy payloads) - interval / legacy → the stored value, with `schedule_display` kept as a final fallback only when it isn't the `once at …` label. 2. **Backend** converts `ValueError` from `update_job()` into HTTP 400 with the parser message, so the WebUI can show a real validation toast instead of an opaque Internal Server Error. The 404 path (job not found) is preserved. ## What Changed - **`static/panels.js`** — add `_cronScheduleForEdit(job)` helper and use it in both `openCronEdit()` and `duplicateCurrentCron()`. The helper returns `schedule.run_at` for one-shot jobs, `schedule.expr` (or legacy `schedule.expression`) for recurring cron, and falls back to `schedule_display` only when it is not the `once at …` presentation form. `saveCronForm()` is unchanged — it still submits `schEl.value`, which is now the canonical field instead of the display label. - **`api/routes.py::_handle_cron_update`** — wrap the `update_job()` call in `try/except ValueError` and return HTTP 400 with the parser message. The earlier normalization try block stays untouched; the 404 check (`if not job`) sits outside the new try block so a missing job still returns 404. - **`tests/test_cron_edit_round_trip.py`** — new regression module with seven tests covering the helper contract (helper exists, both call sites use it, `saveCronForm` still submits the field by name), the route's 400-on-`ValueError` behavior, the preserved 404 path, and the happy-path 200. ## Why - Editing a one-shot cron job's scheduled time has been returning HTTP 500 since the WebUI's initial public release (per #7352 historical note), because the editable field round-trips the `once at …` display label straight to the Agent parser. - The backend regression is independent: even with a correct frontend, any user-typed invalid schedule text (or a programmatic caller) used to produce a 500 instead of a validation response. Both halves are required to deliver a "no more 500" user experience. ## Verification ``` $ .venv/bin/pytest \ tests/test_cron_edit_round_trip.py \ tests/test_cron_no_agent_edit.py \ tests/test_cron_model_override.py \ tests/test_cron_model_provider_picker.py \ tests/test_issue617_cron_profile_selector.py \ -v --no-header … ============================== 27 passed in 4.84s ============================== ``` Seven new tests cover both layers; twenty surrounding cron tests (no-agent edit, model override, model/provider picker, profile selector) stay green. ## Contract | Scenario | Before | After | | --- | --- | --- | | Open one-shot job for edit, change time, save | 500 (display label round-trips to parser) | 200, persisted with new `run_at` | | Open recurring cron job for edit | 200 (legacy `schedule.expression` chain) | 200, unchanged canonical field (`schedule.expr` first, `expression` as legacy fallback) | | Duplicate a one-shot job | 500 on first save | 200, populated with `schedule.run_at` from the source job | | Submit a `once at …` schedule value (programmatic / pre-fix frontend) | 500 | 400 with the parser message | | Submit any other unparseable schedule | 500 | 400 with the parser message | | Update a job that does not exist | 404 | 404 (preserved) | | Update a job with a valid canonical schedule | 200 | 200 (preserved) | ## Risks - **Frontend helper change**: I switched the editable field source for both edit and duplicate in one commit. `saveCronForm()` is untouched, so anything already rendered with the legacy chain (rare in practice — the helper is only called from these two functions) keeps working. The regression `test_save_cron_form_still_submits_schedule_value` pins the submit field name. - **New `try/except ValueError` in `_handle_cron_update`**: the earlier normalization try block (lines 24986–24998) is left exactly as-is. The new try block sits directly around `update_job()` only, so a `ValueError` from input parsing still surfaces before any persistence side effect. The 404 check is outside the new try, so a missing-job path cannot be misreported as 400. Both `test_cron_update_preserves_404_when_job_missing` and `test_cron_update_valid_schedule_still_succeeds` pin those contracts. - **Legacy `schedule.expression` payloads**: accepted as a back-compat fallback in `_cronScheduleForEdit` and in the route (the Agent schema is `expr`, but the parser still handles `expression` per the existing contract). If you prefer to drop the legacy fallback, the helper can be trimmed to `sched.expr` only. ## Model Used - Generated with `MiniMax-M3` via `happy5318/hermes-webui` worktree `/tmp/webui-worktrees/pr7352-cron-edit`, branch `fix/cron-edit-round-trip` (off `origin/master`). ## Related - Closes #7352 - Surface code: - `static/panels.js:1510` `duplicateCurrentCron` (uses helper) - `static/panels.js:1585` `openCronEdit` (uses helper) - `static/panels.js:1905` `saveCronForm` (submit path, unchanged) - `api/routes.py:24979` `_handle_cron_update` (new try/except) - `cron/jobs.py:785` `parse_schedule` (rejects the display form; Agent-side, not modified in this PR) - `cron/jobs.py:2199` `update_job` (the call that raises) - Test surface: new `tests/test_cron_edit_round_trip.py` --- api/routes.py | 12 +- static/panels.js | 32 +++- tests/test_cron_edit_round_trip.py | 240 +++++++++++++++++++++++++++++ 3 files changed, 281 insertions(+), 3 deletions(-) create mode 100644 tests/test_cron_edit_round_trip.py diff --git a/api/routes.py b/api/routes.py index b827c53f..599b02ee 100644 --- a/api/routes.py +++ b/api/routes.py @@ -25011,7 +25011,17 @@ def _handle_cron_update(handler, body): updates[k] = v except ValueError as e: return bad(handler, str(e)) - job = update_job(body["job_id"], updates) + # #7352: ``update_job`` re-parses the updated schedule through + # ``cron.jobs.parse_schedule``, which raises ``ValueError`` for + # display-form input like ``"once at 2026-08-28 16:05"`` (or any other + # user-typed garbage). That exception previously escaped to a 500 + # because the earlier normalization try block didn't cover it. Return + # 400 with the parser message so the WebUI can surface a real + # validation error instead of an opaque Internal Server Error. + try: + job = update_job(body["job_id"], updates) + except ValueError as e: + return bad(handler, str(e), 400) if not job: return bad(handler, "Job not found", 404) return j(handler, {"ok": True, "job": _cron_job_for_api(job)}) diff --git a/static/panels.js b/static/panels.js index 7d4e174a..5c87bc0d 100644 --- a/static/panels.js +++ b/static/panels.js @@ -1507,6 +1507,34 @@ function editCurrentCron(){ if (!_currentCronDetail) return; openCronEdit(_currentCronDetail); } +function _cronScheduleForEdit(job){ + // #7352: when the user opens an existing job for edit or duplicate, the + // editable field must hold the canonical Agent-parseable schedule, not + // the human-readable ``schedule_display`` ("once at ..."). The Agent + // parser at cron/jobs.py rejects the display form with ValueError and + // the resulting round-trip has been failing with HTTP 500 since the + // initial WebUI release. + if(!job) return ''; + const sched = job.schedule; + if(sched && typeof sched === 'object'){ + // One-shot: schedule_display is purely presentation; run_at is what the + // parser accepts. + if(sched.kind === 'once' && sched.run_at) return sched.run_at; + // Recurring cron: the current Agent schema is ``expr``; some legacy + // payloads still expose ``expression`` — accept either. + if(sched.expr) return sched.expr; + if(sched.expression) return sched.expression; + if(sched.run_at) return sched.run_at; + } + // Final fallback: only use schedule_display if it isn't the "once at ..." + // presentation label (which the parser rejects). For any other text + // (e.g. interval/every-30m) the display form is also a valid input. + if(job.schedule_display && !/^\s*once at\s+/i.test(job.schedule_display)){ + return job.schedule_display; + } + return ''; +} + function duplicateCurrentCron(){ if (!_currentCronDetail) return; const job = _currentCronDetail; @@ -1529,7 +1557,7 @@ function duplicateCurrentCron(){ } _renderCronForm({ name: dupName, - schedule: job.schedule_display || (job.schedule && job.schedule.expression) || '', + schedule: _cronScheduleForEdit(job), prompt: job.prompt || '', deliver: job.deliver || 'local', profile: job.profile || '', @@ -1590,7 +1618,7 @@ function openCronEdit(job){ _cronSelectedSkills = Array.isArray(job.skills) ? [...job.skills] : []; _renderCronForm({ name: job.name || '', - schedule: job.schedule_display || (job.schedule && job.schedule.expression) || '', + schedule: _cronScheduleForEdit(job), prompt: job.prompt || '', deliver: job.deliver || 'local', profile: job.profile || '', diff --git a/tests/test_cron_edit_round_trip.py b/tests/test_cron_edit_round_trip.py new file mode 100644 index 00000000..39400fb6 --- /dev/null +++ b/tests/test_cron_edit_round_trip.py @@ -0,0 +1,240 @@ +"""Regression coverage for issue #7352: cron edit/duplicate uses +``schedule_display`` ("once at ...") as editable input, which the Agent +parser at ``cron.jobs.parse_schedule`` rejects with ``ValueError`` and +the resulting round-trip has been failing with HTTP 500 since the +WebUI's initial public release. + +Two layers are fixed in this regression: + +1. ``static/panels.js`` adds ``_cronScheduleForEdit`` and uses it in + both ``openCronEdit`` and ``duplicateCurrentCron`` so the editable + field holds the canonical Agent-parseable value (one-shot + ``schedule.run_at``; recurring ``schedule.expr``; interval keeps its + existing text). + +2. ``api/routes.py::_handle_cron_update`` now wraps the ``update_job`` + call in ``try/except ValueError`` and returns HTTP 400 with the + parser message instead of letting the exception escape as a 500. +""" +from __future__ import annotations + +import io +import json +import sys +import types +from pathlib import Path + +REPO = Path(__file__).resolve().parents[1] +PANELS_JS = (REPO / "static" / "panels.js").read_text(encoding="utf-8") + + +class _JSONHandler: + def __init__(self): + self.status = None + self.headers = {} + self.response_headers = [] + self.wfile = io.BytesIO() + + def send_response(self, status): + self.status = status + + def send_header(self, key, value): + self.response_headers.append((key, value)) + + def end_headers(self): + pass + + +def _payload(handler): + return json.loads(handler.wfile.getvalue().decode("utf-8")) + + +def _function_body(name: str) -> str: + marker = f"function {name}(" + start = PANELS_JS.find(marker) + assert start != -1, f"{name} not found" + paren = PANELS_JS.find("(", start) + assert paren != -1, f"{name} params not found" + depth = 0 + for idx in range(paren, len(PANELS_JS)): + ch = PANELS_JS[idx] + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if depth == 0: + brace = PANELS_JS.find("{", idx) + break + else: + raise AssertionError(f"{name} params did not terminate") + assert brace != -1, f"{name} body not found" + depth = 0 + for idx in range(brace, len(PANELS_JS)): + ch = PANELS_JS[idx] + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + if depth == 0: + return PANELS_JS[brace + 1 : idx] + raise AssertionError(f"{name} body did not terminate") + + +# --- Frontend: _cronScheduleForEdit helper exists and is referenced ---------- + + +def test_cron_schedule_for_edit_helper_exists(): + """A canonical-by-kind schedule picker is required so that opening + an existing one-shot job for edit/duplicate populates the editable + field with the Agent-parseable ``run_at`` rather than the + presentation-only ``schedule_display`` label.""" + body = _function_body("_cronScheduleForEdit") + assert body, "_cronScheduleForEdit must exist" + # One-shot: use run_at, never the display label. + assert "kind === 'once'" in body + assert "run_at" in body + # Recurring cron: prefer the current Agent schema field ``expr``; + # accept legacy ``expression`` for back-compat. + assert "sched.expr" in body + assert "sched.expression" in body + # Final fallback: only use schedule_display if it isn't the unsafe + # ``once at ...`` presentation form. + assert "schedule_display" in body + assert "once at" in body + assert "return ''" in body + + +def test_open_cron_edit_uses_helper(): + """openCronEdit must call _cronScheduleForEdit, not the legacy + ``schedule_display || schedule.expression`` chain that round-trips + the unsafe display label back to the Agent parser.""" + body = _function_body("openCronEdit") + assert "schedule: _cronScheduleForEdit(job)" in body + # Legacy chain must be gone from this function. + assert "job.schedule_display" not in body + assert "job.schedule.expression" not in body + + +def test_duplicate_current_cron_uses_helper(): + """duplicateCurrentCron has the same root cause as openCronEdit; the + fix must cover both call sites or duplicating a one-shot job + would also fail.""" + body = _function_body("duplicateCurrentCron") + assert "schedule: _cronScheduleForEdit(job)" in body + assert "job.schedule_display" not in body + + +def test_save_cron_form_still_submits_schedule_value(): + """Regression guard: the saved-field name stays ``schedule`` and + the helper must not have leaked into the submit path (which already + reads ``schEl.value`` and submits it unchanged).""" + body = _function_body("saveCronForm") + assert "const schedule=schEl.value.trim();" in body + assert "schedule," in body + + +# --- Backend: _handle_cron_update converts ValueError to HTTP 400 -------------- + + +def _install_cron_jobs_stub(monkeypatch, *, update_fn): + cron_pkg = types.ModuleType("cron") + cron_pkg.__path__ = [] + cron_jobs = types.ModuleType("cron.jobs") + cron_jobs.update_job = update_fn + monkeypatch.setitem(sys.modules, "cron", cron_pkg) + monkeypatch.setitem(sys.modules, "cron.jobs", cron_jobs) + + +def test_cron_update_value_error_returns_400(monkeypatch): + """#7352: when the Agent parser rejects the submitted schedule with + ValueError (the "once at ..." case before the frontend fix, or any + user-typed garbage), the route must return 400 with the parser + message instead of letting the exception escape as 500.""" + import api.routes as routes + + def _raise(job_id, updates): + raise ValueError( + "Invalid schedule 'once at 2026-08-28 16:05'. Use:\n" + " - Timestamp: '2026-02-03T14:00:00' (one-shot at time)" + ) + + _install_cron_jobs_stub(monkeypatch, update_fn=_raise) + + handler = _JSONHandler() + routes._handle_cron_update( + handler, + { + "job_id": "test-job", + "schedule": "once at 2026-08-28 16:05", + }, + ) + + assert handler.status == 400, ( + "schedule validation failures must surface as 400, not 500 " + "(#7352)" + ) + payload = _payload(handler) + assert "error" in payload + assert "Invalid schedule" in payload["error"], ( + "parser message must propagate so the WebUI can show a useful " + "validation toast" + ) + assert "once at 2026-08-28 16:05" in payload["error"] + + +def test_cron_update_preserves_404_when_job_missing(monkeypatch): + """The new ValueError handler must not swallow the not-found path: + when ``update_job`` returns ``None`` (no such job), the route must + still respond 404.""" + import api.routes as routes + + def _missing(job_id, updates): + return None + + _install_cron_jobs_stub(monkeypatch, update_fn=_missing) + + handler = _JSONHandler() + routes._handle_cron_update( + handler, + {"job_id": "ghost", "schedule": "0 9 * * *"}, + ) + + assert handler.status == 404 + assert "not found" in _payload(handler).get("error", "").lower() + + +def test_cron_update_valid_schedule_still_succeeds(monkeypatch): + """Happy path: a parseable schedule (the post-fix frontend submits + ``run_at`` rather than ``schedule_display``) reaches ``update_job`` + and the route returns 200 with the job payload.""" + import api.routes as routes + + calls = [] + job = { + "id": "test-job", + "schedule": { + "kind": "once", + "run_at": "2026-08-28T16:05:00-05:00", + "display": "once at 2026-08-28 16:05", + }, + "schedule_display": "once at 2026-08-28 16:05", + } + + def _ok(job_id, updates): + calls.append((job_id, updates)) + return {**job, **updates} + + _install_cron_jobs_stub(monkeypatch, update_fn=_ok) + + handler = _JSONHandler() + routes._handle_cron_update( + handler, + { + "job_id": "test-job", + "schedule": "2026-08-28T16:05:00-05:00", # canonical ISO + }, + ) + + assert handler.status == 200 + assert calls and calls[0][0] == "test-job" + assert calls[0][1]["schedule"] == "2026-08-28T16:05:00-05:00"