master
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 nesquena-hermesandhappy5318
|
aaae80292a |
fix(cron): keep natural-language recurring schedules verbatim on edit/duplicate
Gate follow-up to #7649. _cronScheduleForEdit() consulted sched.expr before schedule_display, so opening or duplicating a job scheduled as `every monday 9am` populated the field with the canonical `0 9 * * 1`. The Agent rebuilds schedule_display from whatever the WebUI submits, so the rewrite stuck and the user silently lost their phrasing. Only the `once at ...` label is unparseable; every other schedule_display form round-trips through parse_schedule(), so prefer it and fall back to expr/expression. The existing tests asserted on the SOURCE TEXT of the helper and stayed green through this defect. Adds four tests that EXECUTE the real helper out of panels.js via node; the natural-language case is red before this fix. Co-authored-by: happy5318 <happy5318@users.noreply.github.com> |
||
|
happy5318
|
3163a91d1a |
# Cron edit/duplicate must round-trip a parseable schedule; bad input returns 400 (#7352)
## 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`
|