fix(cron): stop the job editor rounding an interval it can represent exactly

[leonlaiyc]

Problem / Motivation

The cron job editor picks the interval unit by magnitude — the largest unit
that is <= the interval — and then rounds:

const intUnit = secs >= 86400 ? 'days' : secs >= 3600 ? 'hours' : 'minutes'
const intVal  = Math.max(1, Math.round(/* secs / that unit */))

For a 90-minute job, secs = 5400 is >= 3600, so the unit is hours and
Math.round(1.5) is 2. buildBody then re-serialises intVal * 3600.

So opening a 90-minute job and saving any unrelated field — renaming it,
toggling silent — silently rewrites its schedule to 2 hours. The user is never
told; the form simply displayed the wrong interval and then persisted what it
displayed.

The same shape one unit up: a 36-hour job (129600) selects days,
Math.round(1.5) is 2, and it persists as 2 days.

Why it matters

This is silent corruption of a schedule the user configured, triggered by an edit
that has nothing to do with scheduling. A job set to run every 90 minutes quietly
runs every 120, and nothing in the UI reports the change — the form shows "2
hours" as if that were what was stored.

90 minutes is exactly representable in the units this form already offers.
Nothing about the interval was un-representable; the magnitude test discarded the
representation that worked before rounding ever ran.

What changed (motivation → approach → change)

Symptom — an interval the form can represent exactly is rounded to a
neighbouring value, and the rounded value is written back on the next save.

Root cause — unit selection asks "which unit is this interval bigger than?"
when the question that preserves the value is "which unit divides it?".

Change — pick the largest unit that divides secs evenly:

const evenUnit = secs % 86400 === 0 ? 'days'
  : secs % 3600 === 0 ? 'hours'
  : secs % 60 === 0 ? 'minutes'
  : null
const intUnit = evenUnit ?? (/* unchanged magnitude choice */)

Largest-that-divides, not smallest-that-divides: a minutes-only rule would also
round-trip correctly but would show a daily job as "1440 minutes". Exactness is
necessary, not sufficient — both properties are pinned by tests.

What is deliberately NOT changed. Where nothing divides evenly — a 90-second
job — the pre-existing nearest-magnitude behaviour is kept. The form offers no
sub-minute unit, so that schedule cannot be represented here at all; widening the
unit set is a separate product question, and changing it under cover of this fix
would be a second, undeclared change. There is a test asserting that path still
behaves exactly as it does today, so the boundary is enforced rather than merely
stated.

One production file, one hunk.

Tests

website/src/test/JobForm.interval.test.ts (new), driving the two exported
functions the editor actually uses:

• keeps a 90-minute job in minutes instead of rounding it to 2 hours
• keeps a 36-hour job in hours instead of rounding it to 2 days — the same
  defect at the next unit boundary, so the fix is not special-cased to one value.
• A parameterised round-trip table — the defect stated directly:
  buildBody(parseJobDefaults(job)) must return the interval it was given, for
  90 minutes, 36 hours, 150 minutes, 1 hour, 2 hours, 1 day, 30 minutes, 1 week.
  This is what "saving an unrelated field must not rewrite the schedule" means in
  code.
• still prefers the largest EXACT unit, not merely the smallest one — guards the
  1440-minutes regression a naive fix would introduce.
• leaves a sub-minute schedule on the pre-existing nearest-unit behaviour —
  pins the declared non-change above.
• never produces an interval below the input control minimum of 1 — the number
  input is min={1}, so the clamp must survive.

Red-before against the unmodified main component: 5 failed / 8 passed,
with the corruption stated numerically —

AssertionError: expected 7200 to be 5400     // 90 minutes → 2 hours
AssertionError: expected 172800 to be 129600 // 36 hours   → 2 days
AssertionError: expected 10800 to be 9000    // 150 minutes → 3 hours

Green-after: 13 passed.

Gates: tsc -b ✓, eslint ✓ on both changed files.

Manual verification

N/A — unit coverage sufficient. The round-trip table exercises the exact
parse→serialise path the editor runs on save, which is where the corruption
happens; a manual click-through would confirm one value of the eight the table
covers.

Screenshots / video

Both captures render the real JobForm component with the app's own stylesheet,
mounted against one fixture job — every_secs: 5400, i.e. every 90 minutes. Only the
component under test differs between them.

Before — the editor shows 2 hours, and saving persists 7200s:

After — the editor shows 90 minutes, and saving persists 5400s:

No layout, component, or theme change — the same two controls render; the values
in them are what the fix corrects.

Related Issues

No linked issue. This is the residual named in #8644's First Principles
review, which fixed the cron-expression side of #8469 and counted this as the one
remaining unfixed sibling of the same root cause in the same function:

parseJobDefaults rounds interval jobs into a lossy unit (every_secs: 5400
parses as intUnit: 'hours', intVal: Math.round(1.5) = 2), and buildBody
re-serialises it, so saving an unrelated field silently rewrites a 90-minute job
to 2 hours — exactly the #8469 shape. […] Interval has no verbatim fallback
mode, so the fix differs (pick the largest unit that divides secs evenly).

That is the approach implemented here.

Pattern harvest

Rule candidate: review-prompt

Pattern: a lossy parse feeding a re-serialising save. Neither half is a defect
alone — a display rounding is harmless if nothing writes it back, and a serialiser
is correct if its input is exact. The corruption exists only because the editor
round-trips: parse → (edit an unrelated field) → serialise publishes the parse's
approximation as the user's stored value. When reviewing an edit form, ask whether
serialise(parse(x)) === x for every x the backend can store, and pin it as a
table — that assertion catches this whole class, while a per-field test does not,
because every individual field looks right.

This is the second instance in one function (#8469 was the cron-expression half),
which is what makes it a pattern rather than a bug.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — the in-source comment states why the unit is chosen by divisibility and what is deliberately left alone
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of de754032d9dc5bd5b1bee3b1dc2a1d71037b3f4c via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix at the right layer: unit selection by divisibility, with the round-trip invariant pinned as a table and the declared non-change test-enforced.

[DESIGN-REVIEWED] de75403

[github-actions]

UX Review (Fable 5, fork) — ✅ PASS

UX-level review of de754032d9dc5bd5b1bee3b1dc2a1d71037b3f4c via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The diff is a targeted fix with tests and two before/after screenshots (PR-added, not in the base tree). No new strings, controls, or layout — it removes a silent-schedule-rewrite defect. One residual case remains: sub-minute jobs (creatable only outside this form) still display rounded and persist the rounded value silently on save, which the PR deliberately scopes out but leaves as the same silent-corruption class for that rare input.

UX-Verdict: PASS

Removes a silent schedule rewrite: a 90-minute job now displays and round-trips as 90 minutes instead of quietly becoming 2 hours.

Suggestions

• The preserved fallback (intUnit = evenUnit ?? …magnitude…) still lets a CLI-created 90-second job display as "2 minutes" and silently persist as 120s on save; render a one-line inline note by the interval input when the parse was lossy ("Stored as 90s; saving will change it to 2 minutes") so the rewrite is at least announced.

[UX-REVIEWED] de75403

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed de754032d9dc5bd5b1bee3b1dc2a1d71037b3f4c via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] de75403

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of de754032d9dc5bd5b1bee3b1dc2a1d71037b3f4c via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks are done: the change is a single cause-level hunk plus tests, the exports it relies on already exist in base (JobForm.tsx:501), the screenshots follow the documented temp-screenshots/ convention, and the only sibling of the root cause (sub-minute intervals) is declared and test-pinned. Final review:

First-Principles-Verdict: PASS

Unit selection now asks "which unit divides secs" instead of "which is smaller", killing the silent 90-min→2-h rewrite at its cause; every item earns its place.

What this change ships

Intent: stop the job editor from silently rewriting an interval it can represent exactly when the user saves an unrelated field — a FIX.

1. A 90-minute job now displays as 90 minutes, not 2 hours — justified, the fix.
2. Saving an unrelated field no longer persists the rounded interval — justified, same mechanism.
3. Sub-minute intervals keep the old nearest-unit rounding, now test-pinned — declared non-change.
4. New round-trip test table over parseJobDefaults/buildBody — justified; uses the existing export (JobForm.tsx:501), no new surface.
5. Two before/after screenshots under temp-screenshots/ — justified, documented repo convention (temp-screenshots/README.md).

Watch

• The declared residual is still silent corruption, not just display rounding: a 90-second job (creatable via CLI/agent; the editor's regex even parses every 90s) round-trips to 120s on an unrelated save. Sibling count: 1 (the sub-minute branch; WeekGrid.tsx:34 is display-only, no write-back — grepped every_secs). The general fix — carry secs verbatim through form state until the interval controls are touched, the interval analogue of fix(cron): keep multi-time cron expressions in cron mode in the job editor (#8469) #8644's verbatim cron mode — is genuinely larger; accepted as deferred, but it removes the whole class this PR's own "pattern harvest" names.

[FIRST-PRINCIPLES-REVIEWED] de75403

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed de754032d9dc5bd5b1bee3b1dc2a1d71037b3f4c via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate hinges on a secs === 0 input. The backend rejects non-positive intervals (onboarding_import.py:1796 every_secs <= 0 → rejected), the UI input control has min={1}, and a zero-second interval is degenerate on both the old and new code paths. Criterion (a) — a concrete input that occurs in practice — cannot be re-derived; the candidate itself rates it "low" and could not confirm the backend ever emits it. It fails falsification.

The changed logic is correct for all realistic positive secs: it selects the largest exactly-dividing unit and round-trips the value, with the pre-existing nearest-magnitude fallback preserved for sub-minute inputs. No new grounded defect surfaces.

No findings.

[OPUS-REVIEWED] de75403

[iamwhatever]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: interval unit selection in the cron job editor picked the largest unit <= the interval instead of the largest that divides it evenly, so a 90-minute job round-tripped to 2 hours on any unrelated save -- root cause is one expression in parseJobDefaults, fixed with an even-divisor test and a regression test. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.