From aaae80292aa79fc6df9f7b49ec104d2b11b16ad9 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 20 Sep 2026 03:14:42 +0000 Subject: [PATCH] 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 --- static/panels.js | 14 ++++- tests/test_cron_edit_round_trip.py | 94 ++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 2 deletions(-) diff --git a/static/panels.js b/static/panels.js index 5c87bc0d..dd5090c3 100644 --- a/static/panels.js +++ b/static/panels.js @@ -1516,10 +1516,20 @@ function _cronScheduleForEdit(job){ // initial WebUI release. if(!job) return ''; const sched = job.schedule; + // Only the "once at ..." label is unparseable; every other schedule_display + // form the Agent emits (`every monday 9am`, `every 30m`, ...) round-trips + // through parse_schedule() AND is what the user actually typed. Prefer it + // over the canonical `expr`, otherwise editing or duplicating a + // natural-language recurring job silently rewrites it to raw cron + // (`every monday 9am` -> `0 9 * * 1`) — the Agent rebuilds schedule_display + // from whatever we submit, so the rewrite sticks. + const display = job.schedule_display; + const displayIsParseable = Boolean(display) && !/^\s*once at\s+/i.test(display); 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; + if(displayIsParseable) return display; // Recurring cron: the current Agent schema is ``expr``; some legacy // payloads still expose ``expression`` — accept either. if(sched.expr) return sched.expr; @@ -1529,8 +1539,8 @@ function _cronScheduleForEdit(job){ // 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; + if(displayIsParseable){ + return display; } return ''; } diff --git a/tests/test_cron_edit_round_trip.py b/tests/test_cron_edit_round_trip.py index 39400fb6..4c839f27 100644 --- a/tests/test_cron_edit_round_trip.py +++ b/tests/test_cron_edit_round_trip.py @@ -238,3 +238,97 @@ def test_cron_update_valid_schedule_still_succeeds(monkeypatch): assert handler.status == 200 assert calls and calls[0][0] == "test-job" assert calls[0][1]["schedule"] == "2026-08-28T16:05:00-05:00" + + +# --------------------------------------------------------------------------- +# Behavioral round-trip coverage (gate follow-up). +# +# The assertions above pin the SOURCE TEXT of `_cronScheduleForEdit`. That is +# not enough: the first implementation satisfied every one of them while still +# rewriting a natural-language recurring schedule. `every monday 9am` has a +# canonical `sched.expr` of `0 9 * * 1`, and because `expr` was consulted +# before `schedule_display`, opening such a job for edit (or duplicating it) +# replaced the user's wording with raw cron. The Agent rebuilds +# `schedule_display` from whatever the WebUI submits, so the rewrite sticks and +# the user silently loses the phrasing they typed. +# +# These tests EXECUTE the real helper out of panels.js instead of reading it. +# --------------------------------------------------------------------------- + +import subprocess + + +def _cron_schedule_for_edit_source() -> str: + """Slice the complete `_cronScheduleForEdit` declaration out of panels.js.""" + marker = "function _cronScheduleForEdit(" + start = PANELS_JS.find(marker) + assert start != -1, "_cronScheduleForEdit not found in panels.js" + paren = PANELS_JS.find("(", start) + depth = 0 + brace = -1 + 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 + assert brace != -1, "_cronScheduleForEdit 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[start:idx + 1] + raise AssertionError("_cronScheduleForEdit body did not terminate") + + +def _run_cron_schedule_for_edit(job: dict) -> str: + """Execute the real `_cronScheduleForEdit` from panels.js against `job`.""" + script = ( + f"{_cron_schedule_for_edit_source()}\n" + f"const job = {json.dumps(job)};\n" + "process.stdout.write(String(_cronScheduleForEdit(job)));\n" + ) + result = subprocess.run( + ["node", "-e", script], check=True, capture_output=True, text=True + ) + return result.stdout + + +def test_natural_language_recurring_schedule_survives_edit_unchanged(): + """`every monday 9am` must round-trip as typed, not as `0 9 * * 1`. + + Red before the fix: `sched.expr` was preferred over `schedule_display`. + """ + job = { + "schedule_display": "every monday 9am", + "schedule": {"kind": "cron", "expr": "0 9 * * 1"}, + } + assert _run_cron_schedule_for_edit(job) == "every monday 9am" + + +def test_one_shot_still_uses_canonical_run_at_not_the_display_label(): + """The original #7352 fix must not regress: `once at ...` is unparseable.""" + job = { + "schedule_display": "once at 2026-08-28 16:00", + "schedule": {"kind": "once", "run_at": "2026-08-28T16:00:00"}, + } + assert _run_cron_schedule_for_edit(job) == "2026-08-28T16:00:00" + + +def test_raw_cron_expression_without_display_falls_back_to_expr(): + """A job carrying only `expr` still yields the canonical expression.""" + job = {"schedule": {"kind": "cron", "expr": "*/15 * * * *"}} + assert _run_cron_schedule_for_edit(job) == "*/15 * * * *" + + +def test_legacy_expression_field_still_accepted(): + """Back-compat: older payloads expose `expression` rather than `expr`.""" + job = {"schedule": {"kind": "cron", "expression": "0 0 * * *"}} + assert _run_cron_schedule_for_edit(job) == "0 0 * * *"