From 8759b25e072dcc201fa00c37cfbcf022d35ce6be Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 12 Sep 2026 22:56:23 -0700 Subject: [PATCH] fix(automations): isolate the scheduler tick and refuse oversized cron steps (#20152) Two defects that change nothing about when an existing schedule fires. #16303: evaluateDueRuns awaited each row with no catch, so one unreadable schedule skipped every later due automation in that tick. Each row is isolated now; a poison record writes one folded skipped_unavailable run explaining itself and the tick continues. A renderer send that throws is closed out as dispatch_failed rather than mislabelled as an unreadable schedule. #15895: step validation only checked integer >= 1, so a step wider than its field degraded silently to a single value and still passed validation. Oversized steps are refused at input time only, bounded by the count of distinct values a field holds, so day of week rejects */8 while */7 stays legal. Runtime parsing stays lenient so rows saved before the gate keep running the cadence they have. isValidAutomationSchedule now answers only 'acceptable as new input'; a new isRunnableAutomationSchedule answers 'can Orca still run this', and the editor uses it so a legacy row opens intact and can be renamed without re-authoring a schedule that is still firing. Verified: 34/34 corpus expressions fire identically to main. Fixes #16303 Fixes #15895 --- src/main/automations/dispatch-refusal.test.ts | 113 ++++++++++ src/main/automations/dispatch-refusal.ts | 70 +++++- src/main/automations/run-usage-collection.ts | 32 +++ src/main/automations/service.test.ts | 154 ++++++++++++++ src/main/automations/service.ts | 41 ++-- .../automations/AutomationEditorDialog.tsx | 3 + .../AutomationSchedulePicker.render.test.tsx | 1 + .../AutomationSchedulePicker.test.ts | 1 + .../automations/AutomationSchedulePicker.tsx | 15 +- .../automations/HermesCronOutputView.tsx | 4 +- .../automations/automation-edit-draft.ts | 10 +- ...automation-legacy-schedule-editing.test.ts | 118 +++++++++++ .../automations/automation-save-action.ts | 10 +- .../automation-schedule-input-gate.ts | 19 ++ .../external-automation-schedule-display.ts | 4 +- .../use-automation-editor-actions.ts | 1 + .../use-automations-page-local-state.ts | 1 + src/shared/automation-cron-field-parsing.ts | 133 ++++++++++++ .../automation-cron-input-validation.test.ts | 101 +++++++++ src/shared/automation-schedule-parsing.ts | 199 ++++++------------ 20 files changed, 861 insertions(+), 169 deletions(-) create mode 100644 src/main/automations/dispatch-refusal.test.ts create mode 100644 src/renderer/src/components/automations/automation-legacy-schedule-editing.test.ts create mode 100644 src/renderer/src/components/automations/automation-schedule-input-gate.ts create mode 100644 src/shared/automation-cron-field-parsing.ts create mode 100644 src/shared/automation-cron-input-validation.test.ts diff --git a/src/main/automations/dispatch-refusal.test.ts b/src/main/automations/dispatch-refusal.test.ts new file mode 100644 index 00000000000..233deb3e782 --- /dev/null +++ b/src/main/automations/dispatch-refusal.test.ts @@ -0,0 +1,113 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { Automation, AutomationRun } from '../../shared/automations-types' +import type { AutomationRunWriter } from './automation-run-writer' +import { UNEVALUABLE_SCHEDULE, recordUnevaluableAutomation } from './dispatch-refusal' + +const brokenAutomation: Automation = { + id: 'a1', + name: 'Broken schedule', + prompt: 'Check the repo', + precheck: null, + agentId: 'claude', + projectId: 'r1', + executionTargetType: 'local', + executionTargetId: 'local', + schedulerOwner: 'local_host_service', + workspaceMode: 'existing', + workspaceId: 'wt1', + baseBranch: null, + reuseSession: false, + timezone: 'UTC', + rrule: '0 9 32 * *', + dtstart: 0, + enabled: true, + nextRunAt: 1000, + missedRunPolicy: 'run_once_within_grace', + missedRunGraceMinutes: 720, + createdAt: 0, + updatedAt: 0 +} + +const makeRun = (id: string): AutomationRun => ({ + id, + automationId: brokenAutomation.id, + title: 'Broken schedule run', + scheduledFor: brokenAutomation.nextRunAt, + status: 'pending', + trigger: 'scheduled', + workspaceId: brokenAutomation.workspaceId, + sessionKind: 'terminal', + chatSessionId: null, + terminalSessionId: null, + terminalPaneKey: null, + terminalPtyId: null, + outputSnapshot: null, + precheckResult: null, + usage: null, + error: null, + startedAt: null, + dispatchedAt: null, + createdAt: 0 +}) + +/** Records what the writer was asked to do, with `repeatSkip` standing in for a fold or not. */ +function makeRunWriter(foldsRepeat: boolean): { + writer: AutomationRunWriter + created: string[] + updated: { status: string; error?: string | null }[] +} { + const created: string[] = [] + const updated: { status: string; error?: string | null }[] = [] + const writer: AutomationRunWriter = { + repeatSkip: () => (foldsRepeat ? makeRun('folded') : null), + createRun: () => { + const run = makeRun(`run-${created.length + 1}`) + created.push(run.id) + return run + }, + updateRun: (args) => { + updated.push({ status: args.status, error: args.error }) + return { ...makeRun(args.runId), status: args.status, error: args.error ?? null } + } + } + return { writer, created, updated } +} + +describe('recordUnevaluableAutomation', () => { + afterEach(() => { + vi.restoreAllMocks() + }) + + it('writes one run and logs once when the record is newly broken', () => { + const logged = vi.spyOn(console, 'error').mockImplementation(() => {}) + const { writer, created, updated } = makeRunWriter(false) + + recordUnevaluableAutomation({ + runs: writer, + automation: brokenAutomation, + error: new Error('Invalid cron day of month.') + }) + + expect(created).toEqual(['run-1']) + expect(updated).toEqual([{ status: 'skipped_unavailable', error: UNEVALUABLE_SCHEDULE }]) + expect(logged).toHaveBeenCalledTimes(1) + }) + + // The record is retried every tick on purpose, so a repaired schedule resumes on its own. + // The fold is what keeps that from writing a row, and logging, once per tick forever. + it('stays silent on a record it has already reported', () => { + const logged = vi.spyOn(console, 'error').mockImplementation(() => {}) + const { writer, created } = makeRunWriter(true) + + for (let tick = 0; tick < 5; tick += 1) { + recordUnevaluableAutomation({ + runs: writer, + automation: brokenAutomation, + error: new Error('Invalid cron day of month.') + }) + } + + expect(created).toEqual([]) + expect(logged).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/automations/dispatch-refusal.ts b/src/main/automations/dispatch-refusal.ts index 7d7fdc4d140..08fd400e8c8 100644 --- a/src/main/automations/dispatch-refusal.ts +++ b/src/main/automations/dispatch-refusal.ts @@ -5,13 +5,22 @@ * coalescing folds repeats only on byte-identical text, so a reason that varied * per occurrence would write a row each. */ +import type { WebContents } from 'electron' import type { Store } from '../persistence' -import type { Automation } from '../../shared/automations-types' +import type { + Automation, + AutomationDispatchRequest, + AutomationRun +} from '../../shared/automations-types' import { resolveAutomationRunTarget, type AutomationRunTargetResult } from './run-target-resolution' import type { AutomationRunWriter } from './automation-run-writer' export const NO_DISPATCH_HOST = 'No Orca window was available to launch the automation.' +/** A record the tick could not evaluate at all — its schedule no longer resolves (#16303). */ +export const UNEVALUABLE_SCHEDULE = + 'Orca could not evaluate this automation and skipped the occurrence.' + /** A record the authority refuses to execute at all, with no target diagnosis of its own. */ export const NO_RUNNABLE_HOST = 'This automation has no host to run on.' @@ -51,3 +60,62 @@ export function recordRefusedAutomationRun(input: { error: target.ok ? NO_RUNNABLE_HOST : target.error }) } + +/** + * Marks the poison record the scheduler tick just stepped over, so the user sees why it + * stalled. Folds on the fixed sentence and the unchanged nextRunAt, so a record that stays + * broken writes one row rather than one per tick, and never throws back into the tick. + */ +export function recordUnevaluableAutomation(input: { + runs: AutomationRunWriter + automation: Automation + error: unknown +}): void { + const { automation } = input + try { + // nextRunAt deliberately stays put: the record is retried so a repaired schedule resumes + // on its own. The fold is what keeps that from writing a row — and logging — every tick. + if (input.runs.repeatSkip(automation.id, UNEVALUABLE_SCHEDULE, automation.nextRunAt)) { + return + } + console.error('[automations] failed to evaluate automation:', automation.id, input.error) + const run = input.runs.createRun(automation, automation.nextRunAt) + input.runs.updateRun({ + runId: run.id, + status: 'skipped_unavailable', + workspaceId: automation.workspaceId, + error: UNEVALUABLE_SCHEDULE + }) + } catch (writeError) { + // The original failure has not been reported yet on this path, so carry it too. + console.error( + '[automations] failed to record unevaluable automation:', + automation.id, + input.error, + writeError + ) + } +} + +/** + * Sends the dispatch request through the renderer channel, closing the run out as + * `dispatch_failed` when the send throws — a failed send is not an unreadable schedule. + */ +export function sendRendererDispatch( + channel: Pick | null, + payload: AutomationDispatchRequest, + runs: AutomationRunWriter, + run: AutomationRun +): AutomationRun { + try { + channel?.send('automations:dispatchRequested', payload) + return run + } catch (error) { + return runs.updateRun({ + runId: run.id, + status: 'dispatch_failed', + workspaceId: run.workspaceId, + error: error instanceof Error ? error.message : String(error) + }) + } +} diff --git a/src/main/automations/run-usage-collection.ts b/src/main/automations/run-usage-collection.ts index ab1233e5765..2e6d5075a22 100644 --- a/src/main/automations/run-usage-collection.ts +++ b/src/main/automations/run-usage-collection.ts @@ -1,4 +1,6 @@ import type { Automation, AutomationRun, AutomationRunUsage } from '../../shared/automations-types' +import type { Store } from '../persistence' +import type { AutomationRunWriter } from './automation-run-writer' import type { ClaudeUsageStore } from '../claude-usage/store' import type { CodexUsageStore } from '../codex-usage/store' @@ -97,3 +99,33 @@ export async function collectAutomationRunUsage({ } return unavailable(null, 'provider_unsupported', 'This agent does not report usage to Orca yet.') } + +/** Collects and writes the usage a just-finalized run earned, returning the row to answer with. */ +export async function writeAutomationRunUsage(input: { + store: Store + runs: AutomationRunWriter + run: AutomationRun + claudeUsage: ClaudeUsageStore | null + codexUsage: CodexUsageStore | null +}): Promise { + const { store, run } = input + const usage = await collectAutomationRunUsage({ + automation: store.listAutomations().find((entry) => entry.id === run.automationId), + run, + claudeUsage: input.claudeUsage, + codexUsage: input.codexUsage + }) + // Why: the run is final during the await above, so a concurrent create-time + // retention prune may have evicted it — the usage write must not throw then. + if (!store.listAutomationRuns(run.automationId).some((entry) => entry.id === run.id)) { + return run + } + return input.runs.updateRun({ + runId: run.id, + status: run.status, + workspaceId: run.workspaceId, + terminalSessionId: run.terminalSessionId, + usage, + error: run.error + }) +} diff --git a/src/main/automations/service.test.ts b/src/main/automations/service.test.ts index 773a3f3d9ba..7077a70b6d1 100644 --- a/src/main/automations/service.test.ts +++ b/src/main/automations/service.test.ts @@ -647,4 +647,158 @@ describe('AutomationService', () => { expect(updated.usage?.status).toBe('unavailable') expect(updated.usage?.unavailableReason).toBe('provider_unsupported') }) + + // #16303: listAutomations sorts by name, so 'A ...' is evaluated before 'B ...'. + it('keeps evaluating later due automations after an unreadable schedule throws', async () => { + vi.setSystemTime(new Date('2026-05-13T08:59:00')) + const store = await createStore() + store.addRepo(makeRepo()) + const poison = store.createAutomation({ + name: 'A poison schedule', + prompt: 'Check the repo', + agentId: 'claude', + projectId: 'r1', + workspaceMode: 'existing', + workspaceId: 'wt1', + timezone: 'UTC', + rrule: 'FREQ=DAILY;BYHOUR=9;BYMINUTE=0', + dtstart: new Date('2026-05-12T00:00:00').getTime() + }) + const healthy = store.createAutomation({ + name: 'B healthy schedule', + prompt: 'Check the repo', + agentId: 'claude', + projectId: 'r1', + workspaceMode: 'existing', + workspaceId: 'wt1', + timezone: 'UTC', + rrule: 'FREQ=DAILY;BYHOUR=9;BYMINUTE=0', + dtstart: new Date('2026-05-12T00:00:00').getTime() + }) + // Persisted by an older build, or hand-edited: WEEKLY with no BYDAY cannot resolve a day. + mutateDataFile((state) => { + const entry = state.automations.find((automation) => automation.id === poison.id)! + entry.rrule = 'FREQ=WEEKLY;BYHOUR=9;BYMINUTE=0' + }) + const reloaded = await createStore() + const logged = vi.spyOn(console, 'error').mockImplementation(() => {}) + + vi.setSystemTime(new Date('2026-05-13T09:01:00')) + const send = vi.fn() + const service = new AutomationService(reloaded, { tickMs: 60_000 }) + service.setWebContents({ isDestroyed: () => false, send }) + + service.start() + service.setRendererReady() + await vi.waitFor(() => + expect(send).toHaveBeenCalledWith('automations:dispatchRequested', expect.any(Object)) + ) + service.stop() + + const [, payload] = send.mock.calls[0] + expect(payload.automation.id).toBe(healthy.id) + expect(reloaded.listAutomationRuns(healthy.id)[0]?.status).toBe('dispatching') + const poisonRun = reloaded.listAutomationRuns(poison.id)[0] + expect(poisonRun?.status).toBe('skipped_unavailable') + expect(poisonRun?.error).toBe( + 'Orca could not evaluate this automation and skipped the occurrence.' + ) + expect(logged).toHaveBeenCalled() + }) + + // Same isolation, reached through the cron parser rather than the RRULE one, because that + // is the path all four cron repairs run on. + it('keeps evaluating later due automations after an unreadable cron schedule throws', async () => { + vi.setSystemTime(new Date('2026-05-13T08:59:00')) + const store = await createStore() + store.addRepo(makeRepo()) + const poison = store.createAutomation({ + name: 'A poison cron', + prompt: 'Check the repo', + agentId: 'claude', + projectId: 'r1', + workspaceMode: 'existing', + workspaceId: 'wt1', + timezone: 'UTC', + rrule: '0 9 * * *', + dtstart: new Date('2026-05-12T00:00:00').getTime() + }) + const healthy = store.createAutomation({ + name: 'B healthy cron', + prompt: 'Check the repo', + agentId: 'claude', + projectId: 'r1', + workspaceMode: 'existing', + workspaceId: 'wt1', + timezone: 'UTC', + rrule: '0 9 * * *', + dtstart: new Date('2026-05-12T00:00:00').getTime() + }) + // Day of month 32 never validates at input; only a hand-edited or older-build row has it. + mutateDataFile((state) => { + const entry = state.automations.find((automation) => automation.id === poison.id)! + entry.rrule = '0 9 32 * *' + }) + const reloaded = await createStore() + const logged = vi.spyOn(console, 'error').mockImplementation(() => {}) + + vi.setSystemTime(new Date('2026-05-13T09:01:00')) + const send = vi.fn() + const service = new AutomationService(reloaded, { tickMs: 60_000 }) + service.setWebContents({ isDestroyed: () => false, send }) + + service.start() + service.setRendererReady() + await vi.waitFor(() => + expect(send).toHaveBeenCalledWith('automations:dispatchRequested', expect.any(Object)) + ) + service.stop() + + const [, payload] = send.mock.calls[0] + expect(payload.automation.id).toBe(healthy.id) + expect(reloaded.listAutomationRuns(healthy.id)[0]?.status).toBe('dispatching') + expect(reloaded.listAutomationRuns(poison.id)[0]?.error).toBe( + 'Orca could not evaluate this automation and skipped the occurrence.' + ) + expect(logged).toHaveBeenCalled() + }) + + // A send that throws is a dispatch failure, not an unreadable schedule: the run must land on + // dispatch_failed rather than being left 'dispatching' beside a bogus skipped_unavailable row. + it('marks the run dispatch_failed when the renderer send throws', async () => { + vi.setSystemTime(new Date('2026-05-13T08:59:00')) + const store = await createStore() + store.addRepo(makeRepo()) + const automation = store.createAutomation({ + name: 'Renderer gone', + prompt: 'Check the repo', + agentId: 'claude', + projectId: 'r1', + workspaceMode: 'existing', + workspaceId: 'wt1', + timezone: 'UTC', + rrule: '0 9 * * *', + dtstart: new Date('2026-05-12T00:00:00').getTime() + }) + const logged = vi.spyOn(console, 'error').mockImplementation(() => {}) + logged.mockClear() + + vi.setSystemTime(new Date('2026-05-13T09:01:00')) + const send = vi.fn(() => { + throw new Error('renderer is gone') + }) + const service = new AutomationService(store, { tickMs: 60_000 }) + service.setWebContents({ isDestroyed: () => false, send }) + + service.start() + service.setRendererReady() + await vi.waitFor(() => expect(send).toHaveBeenCalled()) + service.stop() + + const runs = store.listAutomationRuns(automation.id) + expect(runs).toHaveLength(1) + expect(runs[0]?.status).toBe('dispatch_failed') + expect(runs[0]?.error).toBe('renderer is gone') + expect(logged).not.toHaveBeenCalled() + }) }) diff --git a/src/main/automations/service.ts b/src/main/automations/service.ts index 9ed9564e5e5..b683fb6c5a2 100644 --- a/src/main/automations/service.ts +++ b/src/main/automations/service.ts @@ -1,4 +1,8 @@ import type { WebContents } from 'electron' + +/** All the service asks of the renderer: is it still there, and take this message. Narrower + * than WebContents so a test can supply the real shape instead of casting one. */ +export type AutomationRendererChannel = Pick import type { Store } from '../persistence' import { isFinalAutomationRunStatus, @@ -12,7 +16,7 @@ import type { ClaudeUsageStore } from '../claude-usage/store' import type { CodexUsageStore } from '../codex-usage/store' import { runAutomationPrecheck } from './precheck-runner' import { resolveAutomationRunTarget, type AutomationRunTargetResult } from './run-target-resolution' -import { collectAutomationRunUsage } from './run-usage-collection' +import { writeAutomationRunUsage } from './run-usage-collection' import type { HeadlessAutomationDispatcher } from './headless-dispatch' import { clearAutomationDispatchTokens, createAutomationDispatchToken } from './dispatch-tokens' import { runHeadlessAutomationDispatch } from './headless-dispatch-runner' @@ -24,6 +28,8 @@ import { createAutomationRunWriter, type AutomationRunWriter } from './automatio import { describeScheduledRefusal, recordRefusedAutomationRun, + recordUnevaluableAutomation, + sendRendererDispatch, NO_DISPATCH_HOST } from './dispatch-refusal' import type { @@ -37,7 +43,7 @@ export class AutomationService { private readonly store: Store private readonly tickMs: number private timer: ReturnType | null = null - private webContents: WebContents | null = null + private webContents: AutomationRendererChannel | null = null private rendererReady = false private evaluating = false private readonly claudeUsage: ClaudeUsageStore | null @@ -88,7 +94,7 @@ export class AutomationService { this.publish?.(payload) } - setWebContents(webContents: WebContents | null): void { + setWebContents(webContents: AutomationRendererChannel | null): void { this.webContents = webContents this.rendererReady = false } @@ -204,25 +210,13 @@ export class AutomationService { if (run.usage) { return run } - const usage = await collectAutomationRunUsage({ - automation: this.store.listAutomations().find((entry) => entry.id === run.automationId), + return await writeAutomationRunUsage({ + store: this.store, + runs: this.runs, run, claudeUsage: this.claudeUsage, codexUsage: this.codexUsage }) - // Why: the run is final during the await above, so a concurrent create-time - // retention prune may have evicted it — the usage write must not throw then. - if (!this.store.listAutomationRuns(run.automationId).some((entry) => entry.id === run.id)) { - return run - } - return this.runs.updateRun({ - runId: run.id, - status: run.status, - workspaceId: run.workspaceId, - terminalSessionId: run.terminalSessionId, - usage, - error: run.error - }) } private async evaluateDueRuns(): Promise { @@ -236,7 +230,13 @@ export class AutomationService { if (!automation.enabled || automation.nextRunAt > now) { continue } - await this.evaluateAutomation(automation, now) + // Isolated per record (#16303): an unreadable schedule throws out of the + // occurrence math, and an uncaught throw here skipped every later due row. + try { + await this.evaluateAutomation(automation, now) + } catch (error) { + recordUnevaluableAutomation({ runs: this.runs, automation, error }) + } } } finally { this.evaluating = false @@ -336,7 +336,6 @@ export class AutomationService { run: updated, dispatchToken: createAutomationDispatchToken(automation.id, updated.id) } - this.webContents?.send('automations:dispatchRequested', payload) - return updated + return sendRendererDispatch(this.webContents, payload, this.runs, updated) } } diff --git a/src/renderer/src/components/automations/AutomationEditorDialog.tsx b/src/renderer/src/components/automations/AutomationEditorDialog.tsx index b90a41c2f3d..d6ca70851ff 100644 --- a/src/renderer/src/components/automations/AutomationEditorDialog.tsx +++ b/src/renderer/src/components/automations/AutomationEditorDialog.tsx @@ -50,6 +50,9 @@ export type AutomationDraft = { time: string dayOfWeek: string customSchedule: string + // The cadence this record was opened with, or null for a new one. The strict schedule gate + // judges new input; a saved cadence that still runs is not re-judged against it. + savedSchedule: string | null missedRunGraceMinutes: string scheduleWarning: string | null } diff --git a/src/renderer/src/components/automations/AutomationSchedulePicker.render.test.tsx b/src/renderer/src/components/automations/AutomationSchedulePicker.render.test.tsx index f4ba0b46e5b..50d1c927f3e 100644 --- a/src/renderer/src/components/automations/AutomationSchedulePicker.render.test.tsx +++ b/src/renderer/src/components/automations/AutomationSchedulePicker.render.test.tsx @@ -49,6 +49,7 @@ const BASE_DRAFT: AutomationDraft = { dayOfWeek: '5', customSchedule: '', missedRunGraceMinutes: '720', + savedSchedule: null, scheduleWarning: null } diff --git a/src/renderer/src/components/automations/AutomationSchedulePicker.test.ts b/src/renderer/src/components/automations/AutomationSchedulePicker.test.ts index 9202dd58774..3e5f2af7ae7 100644 --- a/src/renderer/src/components/automations/AutomationSchedulePicker.test.ts +++ b/src/renderer/src/components/automations/AutomationSchedulePicker.test.ts @@ -34,6 +34,7 @@ const BASE_DRAFT: AutomationDraft = { dayOfWeek: '1', customSchedule: '', missedRunGraceMinutes: '720', + savedSchedule: null, scheduleWarning: null } diff --git a/src/renderer/src/components/automations/AutomationSchedulePicker.tsx b/src/renderer/src/components/automations/AutomationSchedulePicker.tsx index 3dfe438cee6..9cfbf50772d 100644 --- a/src/renderer/src/components/automations/AutomationSchedulePicker.tsx +++ b/src/renderer/src/components/automations/AutomationSchedulePicker.tsx @@ -11,6 +11,7 @@ import { buildAutomationCronSchedule } from '../../../../shared/automation-sched import { isValidAutomationSchedule } from '../../../../shared/automation-schedule-parsing' import type { AutomationDraft } from './AutomationEditorDialog' import { AutomationCustomCronPanel } from './AutomationCustomCronPanel' +import { acceptsAutomationDraftSchedule } from './automation-schedule-input-gate' import { AutomationTimeField, parseAutomationTime } from './AutomationTimeField' import { Field } from './automation-page-parts' import { translate } from '@/i18n/i18n' @@ -74,10 +75,16 @@ export function AutomationSchedulePicker({ }): React.JSX.Element { const customSchedule = draft.customSchedule.trim() const weekdayNames = getUiWeekdayNames() + // Same gate the save path uses, so an untouched legacy cadence is not flagged red for a + // rule it only has to satisfy as new input. + const acceptsSchedule = (schedule: string): boolean => + acceptsAutomationDraftSchedule({ + customSchedule: schedule, + savedRrule: draft.savedSchedule, + validate: validateAdvancedSchedule + }) const customScheduleInvalid = - draft.preset === 'custom' && - customSchedule.length > 0 && - !validateAdvancedSchedule(customSchedule) + draft.preset === 'custom' && customSchedule.length > 0 && !acceptsSchedule(customSchedule) const setTime = (time: string): void => { onDraftChange((current) => ({ @@ -119,7 +126,7 @@ export function AutomationSchedulePicker({ ) : ( diff --git a/src/renderer/src/components/automations/HermesCronOutputView.tsx b/src/renderer/src/components/automations/HermesCronOutputView.tsx index 6a6fc6e350e..83acd213ddc 100644 --- a/src/renderer/src/components/automations/HermesCronOutputView.tsx +++ b/src/renderer/src/components/automations/HermesCronOutputView.tsx @@ -14,7 +14,7 @@ import { import type { LucideIcon } from 'lucide-react' import CommentMarkdown from '@/components/sidebar/CommentMarkdown' import { cn } from '@/lib/utils' -import { isValidAutomationSchedule } from '../../../../shared/automation-schedule-parsing' +import { isRunnableAutomationSchedule } from '../../../../shared/automation-schedule-parsing' import { formatUiAutomationSchedule } from './automation-schedule-label' import { translate } from '@/i18n/i18n' import { parseHermesOutput, type ParsedHermesSection } from './hermes-cron-output-parse' @@ -33,7 +33,7 @@ function isErrorSection(section: ParsedHermesSection): boolean { function getScheduleDisplay(value: string): string | null { const trimmed = value.trim() - if (!isValidAutomationSchedule(trimmed)) { + if (!isRunnableAutomationSchedule(trimmed)) { return null } return formatUiAutomationSchedule(trimmed) diff --git a/src/renderer/src/components/automations/automation-edit-draft.ts b/src/renderer/src/components/automations/automation-edit-draft.ts index 2e9ad08cc26..cacf4fb0280 100644 --- a/src/renderer/src/components/automations/automation-edit-draft.ts +++ b/src/renderer/src/components/automations/automation-edit-draft.ts @@ -9,8 +9,8 @@ import type { Automation, ExternalAutomationJob } from '../../../../shared/automations-types' import { getAutomationRunRepoId } from '../../../../shared/automation-run-identity' import { - isValidAutomationCronSchedule, - isValidAutomationSchedule, + isRunnableAutomationCronSchedule, + isRunnableAutomationSchedule, tryParseAutomationRrule } from '../../../../shared/automation-schedule-parsing' import type { AutomationDraft } from './AutomationEditorDialog' @@ -19,7 +19,7 @@ import { getAutomationSetupDecisionDraftValue } from './automation-setup-decisio export function buildAutomationEditDraft(automation: Automation): AutomationDraft { const schedule = tryParseAutomationRrule(automation.rrule) - const hasCustomSchedule = !schedule && isValidAutomationSchedule(automation.rrule) + const hasCustomSchedule = !schedule && isRunnableAutomationSchedule(automation.rrule) return { name: automation.name, prompt: automation.prompt, @@ -39,6 +39,7 @@ export function buildAutomationEditDraft(automation: Automation): AutomationDraf time: schedule ? formatTimeInput(schedule.hour, schedule.minute) : AUTOMATION_DEFAULT_TIME, dayOfWeek: String(schedule?.dayOfWeek ?? 1), customSchedule: hasCustomSchedule ? automation.rrule : '', + savedSchedule: automation.rrule, missedRunGraceMinutes: String(automation.missedRunGraceMinutes), scheduleWarning: schedule || hasCustomSchedule @@ -52,7 +53,7 @@ export function buildExternalAutomationEditDraft( placement: { projectId: string; workspaceId: string } ): AutomationDraft { const rawSchedule = job.rawSchedule?.trim() ?? '' - const hasCustomSchedule = isValidAutomationCronSchedule(rawSchedule) + const hasCustomSchedule = isRunnableAutomationCronSchedule(rawSchedule) return { name: job.name, prompt: job.prompt ?? job.promptPreview, @@ -69,6 +70,7 @@ export function buildExternalAutomationEditDraft( time: AUTOMATION_DEFAULT_TIME, dayOfWeek: '1', customSchedule: hasCustomSchedule ? rawSchedule : '', + savedSchedule: rawSchedule || null, missedRunGraceMinutes: '720', scheduleWarning: hasCustomSchedule ? null diff --git a/src/renderer/src/components/automations/automation-legacy-schedule-editing.test.ts b/src/renderer/src/components/automations/automation-legacy-schedule-editing.test.ts new file mode 100644 index 00000000000..39f8327d216 --- /dev/null +++ b/src/renderer/src/components/automations/automation-legacy-schedule-editing.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, it } from 'vitest' +import type { Automation } from '../../../../shared/automations-types' +import { + isRunnableAutomationSchedule, + isValidAutomationSchedule +} from '../../../../shared/automation-schedule-parsing' +import { buildAutomationEditDraft } from './automation-edit-draft' +import { getCronScheduleStatusLabel } from './AutomationCustomCronPanel' +import { acceptsAutomationDraftSchedule } from './automation-schedule-input-gate' + +// `*/90` on minutes was accepted before the oversized-step refusal (#15895) and still runs, +// firing at :00. Only a row persisted by an older build can hold it. +const LEGACY_OVERSIZED_STEP = '*/90 * * * *' + +const makeAutomation = (overrides: Partial = {}): Automation => ({ + id: 'a1', + name: 'Nightly sweep', + prompt: 'Check the repo', + precheck: null, + agentId: 'claude', + projectId: 'r1', + executionTargetType: 'local', + executionTargetId: 'local', + schedulerOwner: 'local_host_service', + workspaceMode: 'existing', + workspaceId: 'wt1', + baseBranch: null, + reuseSession: false, + timezone: 'UTC', + rrule: LEGACY_OVERSIZED_STEP, + dtstart: 0, + enabled: true, + nextRunAt: 0, + missedRunPolicy: 'run_once_within_grace', + missedRunGraceMinutes: 720, + createdAt: 0, + updatedAt: 0, + ...overrides +}) + +describe('editing an automation whose saved schedule predates the input gate', () => { + it('still refuses the same expression as new input', () => { + expect(isValidAutomationSchedule(LEGACY_OVERSIZED_STEP)).toBe(false) + expect(isRunnableAutomationSchedule(LEGACY_OVERSIZED_STEP)).toBe(true) + }) + + it('opens the editor with the saved schedule intact and no warning', () => { + const draft = buildAutomationEditDraft(makeAutomation()) + expect(draft.preset).toBe('custom') + expect(draft.customSchedule).toBe(LEGACY_OVERSIZED_STEP) + expect(draft.scheduleWarning).toBeNull() + }) + + // The regression this guards: a rename was blocked behind re-authoring a schedule the + // user never touched and that is still firing. + it('lets a rename through without re-authoring the schedule', () => { + const automation = makeAutomation() + const renamed = { ...buildAutomationEditDraft(automation), name: 'Renamed sweep' } + expect(renamed.scheduleWarning).toBeNull() + expect( + acceptsAutomationDraftSchedule({ + customSchedule: renamed.customSchedule, + savedRrule: automation.rrule, + validate: isValidAutomationSchedule + }) + ).toBe(true) + }) + + it('still refuses a schedule the user actually changes', () => { + const automation = makeAutomation() + for (const edited of ['*/91 * * * *', '0 */25 * * *', 'nonsense']) { + expect( + acceptsAutomationDraftSchedule({ + customSchedule: edited, + savedRrule: automation.rrule, + validate: isValidAutomationSchedule + }) + ).toBe(false) + } + }) + + it('refuses an oversized step on a new automation, which has nothing saved', () => { + expect( + acceptsAutomationDraftSchedule({ + customSchedule: LEGACY_OVERSIZED_STEP, + savedRrule: null, + validate: isValidAutomationSchedule + }) + ).toBe(false) + }) + + // The editor's live cron status runs the same gate, so an untouched legacy cadence is not + // painted red with "fix this before saving" for a rule it only owes as new input. + it('reports the saved schedule as valid in the editor cron status', () => { + const draft = buildAutomationEditDraft(makeAutomation()) + const accepts = (schedule: string): boolean => + acceptsAutomationDraftSchedule({ + customSchedule: schedule, + savedRrule: draft.savedSchedule, + validate: isValidAutomationSchedule + }) + expect(getCronScheduleStatusLabel(draft.customSchedule, accepts).kind).toBe('valid') + // A different oversized step is new input, so it is still called out. + expect(getCronScheduleStatusLabel('*/91 * * * *', accepts).kind).toBe('invalid') + }) + + it('carries the saved schedule on the draft so the gate can see it', () => { + expect(buildAutomationEditDraft(makeAutomation()).savedSchedule).toBe(LEGACY_OVERSIZED_STEP) + }) + + // Leniency is scoped to the oversized-step gate; a schedule that cannot parse at all is + // still unrepresentable and must keep warning rather than silently round-trip. + it('keeps warning about a saved schedule that cannot be parsed', () => { + const draft = buildAutomationEditDraft(makeAutomation({ rrule: '0 9 32 * *' })) + expect(draft.customSchedule).toBe('') + expect(draft.scheduleWarning).toBeTruthy() + }) +}) diff --git a/src/renderer/src/components/automations/automation-save-action.ts b/src/renderer/src/components/automations/automation-save-action.ts index 102e4f4bdb6..e586dbc61be 100644 --- a/src/renderer/src/components/automations/automation-save-action.ts +++ b/src/renderer/src/components/automations/automation-save-action.ts @@ -5,6 +5,7 @@ import { isValidAutomationSchedule } from '../../../../shared/automation-schedule-parsing' import { translate } from '@/i18n/i18n' +import { acceptsAutomationDraftSchedule } from './automation-schedule-input-gate' import { parseDraftTime } from './automation-draft-model' import { saveHermesAutomation } from './automation-hermes-save' import { saveOrcaAutomation } from './automation-orca-save' @@ -51,7 +52,14 @@ export function createAutomationSaveAction(context: AutomationSaveContext) { const validateAdvancedSchedule = isHermesSave ? isValidAutomationCronSchedule : isValidAutomationSchedule - if (draft.preset === 'custom' && !validateAdvancedSchedule(draft.customSchedule)) { + if ( + draft.preset === 'custom' && + !acceptsAutomationDraftSchedule({ + customSchedule: draft.customSchedule, + savedRrule: draft.savedSchedule, + validate: validateAdvancedSchedule + }) + ) { toast.error( translate( 'auto.components.automations.AutomationsPage.6e91dab317', diff --git a/src/renderer/src/components/automations/automation-schedule-input-gate.ts b/src/renderer/src/components/automations/automation-schedule-input-gate.ts new file mode 100644 index 00000000000..8fa762c55a2 --- /dev/null +++ b/src/renderer/src/components/automations/automation-schedule-input-gate.ts @@ -0,0 +1,19 @@ +/** + * Decides when the editor's strict schedule gate applies. + * + * The gate judges a schedule the user is introducing or changing. A cadence already saved and + * still running is left alone: rows written before the oversized-step refusal (#15895) stay + * valid to run but not to re-enter, and re-judging one would block edits that never touched + * the schedule — a rename, a prompt change — behind re-authoring it. + */ +export function acceptsAutomationDraftSchedule(input: { + customSchedule: string + savedRrule: string | null + validate: (schedule: string) => boolean +}): boolean { + const schedule = input.customSchedule.trim() + if (input.savedRrule !== null && input.savedRrule.trim() === schedule) { + return true + } + return input.validate(schedule) +} diff --git a/src/renderer/src/components/automations/external-automation-schedule-display.ts b/src/renderer/src/components/automations/external-automation-schedule-display.ts index 1b21f1d475a..15359176acc 100644 --- a/src/renderer/src/components/automations/external-automation-schedule-display.ts +++ b/src/renderer/src/components/automations/external-automation-schedule-display.ts @@ -2,7 +2,7 @@ import type { ExternalAutomationJob, ExternalAutomationManager } from '../../../../shared/automations-types' -import { isValidAutomationCronSchedule } from '../../../../shared/automation-schedule-parsing' +import { isRunnableAutomationCronSchedule } from '../../../../shared/automation-schedule-parsing' import { formatUiAutomationSchedule } from './automation-schedule-label' import { translate } from '@/i18n/i18n' @@ -27,7 +27,7 @@ export function getExternalAutomationScheduleDisplay( ] for (const candidate of candidateSchedules) { - if (candidate && isValidAutomationCronSchedule(candidate)) { + if (candidate && isRunnableAutomationCronSchedule(candidate)) { return { label: formatUiAutomationSchedule(candidate) } } } diff --git a/src/renderer/src/components/automations/use-automation-editor-actions.ts b/src/renderer/src/components/automations/use-automation-editor-actions.ts index bbe5104b5a4..0d7fed7bb15 100644 --- a/src/renderer/src/components/automations/use-automation-editor-actions.ts +++ b/src/renderer/src/components/automations/use-automation-editor-actions.ts @@ -76,6 +76,7 @@ export function useAutomationEditorActions({ dayOfWeek: '1', customSchedule: '', missedRunGraceMinutes: '720', + savedSchedule: null, scheduleWarning: null } const nextDraft = template diff --git a/src/renderer/src/components/automations/use-automations-page-local-state.ts b/src/renderer/src/components/automations/use-automations-page-local-state.ts index 7a097b144c3..51d1361aa5f 100644 --- a/src/renderer/src/components/automations/use-automations-page-local-state.ts +++ b/src/renderer/src/components/automations/use-automations-page-local-state.ts @@ -147,6 +147,7 @@ export function useAutomationsPageLocalState(store: AutomationsPageStoreState) { dayOfWeek: '1', customSchedule: '', missedRunGraceMinutes: '720', + savedSchedule: null, scheduleWarning: null }) const draftRef = useRef(draft) diff --git a/src/shared/automation-cron-field-parsing.ts b/src/shared/automation-cron-field-parsing.ts new file mode 100644 index 00000000000..b43e4d4fe5a --- /dev/null +++ b/src/shared/automation-cron-field-parsing.ts @@ -0,0 +1,133 @@ +// Cron field parsing for Orca's automation schedules. +// A field step is bounded by the count of distinct values the field holds: a step of 90 on +// minutes is one value at :00, never "every 90 minutes", so it is refused as input (#15895). +export type CronParseOptions = { + /** Input-time gate: reject a step wider than the field's domain instead of silently + * degenerating to a single value. Off for persisted rows, which must keep running the + * cadence they were saved with rather than start throwing mid-tick. */ + rejectOversizedStep?: boolean +} + +export const MONTH_NAMES: Record = { + JAN: 1, + FEB: 2, + MAR: 3, + APR: 4, + MAY: 5, + JUN: 6, + JUL: 7, + AUG: 8, + SEP: 9, + OCT: 10, + NOV: 11, + DEC: 12 +} + +export const DAY_NAMES: Record = { + SU: 0, + MO: 1, + TU: 2, + WE: 3, + TH: 4, + FR: 5, + SA: 6, + SUN: 0, + MON: 1, + TUE: 2, + WED: 3, + THU: 4, + FRI: 5, + SAT: 6 +} + +function parseCronNumber( + value: string, + names: Record | null, + field: string +): number { + const normalized = value.toUpperCase() + const named = names?.[normalized] + const parsed = named ?? Number(normalized) + if (!Number.isInteger(parsed)) { + throw new Error(`Invalid cron ${field}.`) + } + return parsed +} + +export function parseCronField(args: { + value: string + min: number + max: number + field: string + names?: Record + normalize?: (value: number) => number + // Distinct values the field holds, when `normalize` aliases some away — day of week + // spans 0-7 but holds seven days, so `*/8` is oversized even though 8 <= 7-0+1. + distinctValueCount?: number + rejectOversizedStep?: boolean +}): Set { + const result = new Set() + for (const rawPart of args.value.split(',')) { + const part = rawPart.trim() + if (!part) { + throw new Error(`Invalid cron ${args.field}.`) + } + const stepParts = part.split('/') + if (stepParts.length > 2) { + throw new Error(`Invalid cron ${args.field}.`) + } + const [rangePart, stepPart] = stepParts + if (!rangePart) { + throw new Error(`Invalid cron ${args.field}.`) + } + const step = stepPart === undefined ? 1 : Number(stepPart) + if (!Number.isInteger(step) || step < 1) { + throw new Error(`Invalid cron ${args.field}.`) + } + const domainSize = args.distinctValueCount ?? args.max - args.min + 1 + if (args.rejectOversizedStep && step > domainSize) { + throw new Error(`Cron ${args.field} step must be between 1 and ${domainSize}.`) + } + + let start: number + let end: number + if (rangePart === '*') { + start = args.min + end = args.max + } else if (rangePart.includes('-')) { + const rangeParts = rangePart.split('-') + if (rangeParts.length !== 2 || !rangeParts[0] || !rangeParts[1]) { + throw new Error(`Invalid cron ${args.field}.`) + } + const [startPart, endPart] = rangeParts + start = parseCronNumber(startPart, args.names ?? null, args.field) + end = parseCronNumber(endPart, args.names ?? null, args.field) + } else { + start = parseCronNumber(rangePart, args.names ?? null, args.field) + end = start + } + + const normalizedStart = args.normalize?.(start) ?? start + const normalizedEnd = args.normalize?.(end) ?? end + if ( + start < args.min || + start > args.max || + end < args.min || + end > args.max || + normalizedStart < args.min || + normalizedStart > args.max || + normalizedEnd < args.min || + normalizedEnd > args.max || + start > end + ) { + throw new Error(`Invalid cron ${args.field}.`) + } + for (let value = start; value <= end; value += step) { + result.add(args.normalize?.(value) ?? value) + } + } + if (result.size === 0) { + throw new Error(`Invalid cron ${args.field}.`) + } + return result +} diff --git a/src/shared/automation-cron-input-validation.test.ts b/src/shared/automation-cron-input-validation.test.ts new file mode 100644 index 00000000000..0c50dd0445b --- /dev/null +++ b/src/shared/automation-cron-input-validation.test.ts @@ -0,0 +1,101 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { nextAutomationOccurrenceAfter } from './automation-schedule-occurrences' +import { + isValidAutomationCronSchedule, + isValidAutomationSchedule, + isRunnableAutomationSchedule, + parseCronExpression +} from './automation-schedule-parsing' + +const ascending = (values: Set): number[] => [...values].sort((left, right) => left - right) + +describe('cron oversized steps (#15895)', () => { + it('refuses a step wider than the field domain at input time', () => { + expect(isValidAutomationSchedule('*/90 * * * *')).toBe(false) + expect(isValidAutomationCronSchedule('*/90 * * * *')).toBe(false) + expect(isValidAutomationSchedule('0 */25 * * *')).toBe(false) + expect(isValidAutomationSchedule('0 9 */32 * *')).toBe(false) + expect(isValidAutomationSchedule('0 9 * */13 *')).toBe(false) + // Day of week spans 0-7 but holds seven days, so 8 is oversized even though 8 <= 7-0+1. + expect(isValidAutomationSchedule('0 9 * * */8')).toBe(false) + expect(() => parseCronExpression('*/90 * * * *', { rejectOversizedStep: true })).toThrow( + 'Cron minute step must be between 1 and 60.' + ) + }) + + it('keeps every step that fits its field domain', () => { + expect(isValidAutomationSchedule('*/15 * * * *')).toBe(true) + expect(isValidAutomationSchedule('*/60 * * * *')).toBe(true) + expect(isValidAutomationSchedule('0 */24 * * *')).toBe(true) + expect(isValidAutomationSchedule('0 9 */31 * *')).toBe(true) + expect(isValidAutomationSchedule('0 9 * */12 *')).toBe(true) + expect(isValidAutomationSchedule('0 9 * * */7')).toBe(true) + }) + + // The gate is input-only. A row persisted before it keeps running the cadence it was saved + // with rather than throwing mid-tick, which is what keeps it editable (see the editor tests). + it('still runs a persisted oversized step, degenerating it to its single value', () => { + expect(isRunnableAutomationSchedule('*/90 * * * *')).toBe(true) + expect(ascending(parseCronExpression('*/90 * * * *').minutes)).toEqual([0]) + expect( + nextAutomationOccurrenceAfter( + '*/90 * * * *', + new Date(2026, 4, 1, 0, 0).getTime(), + new Date(2026, 4, 15, 9, 5).getTime() + ) + ).toBe(new Date(2026, 4, 15, 10, 0).getTime()) + }) +}) + +// Node reads the OS timezone on Windows and ignores a runtime process.env.TZ change, so the +// stub — and the precondition asserting it took — cannot work there. +describe.skipIf(process.platform === 'win32')('cron occurrence local-time controls', () => { + afterEach(() => { + vi.unstubAllEnvs() + }) + + it('skips the wall-clock hour that local spring-forward removes', () => { + vi.stubEnv('TZ', 'America/New_York') + // Precondition: the stub took and 2026-03-08 really does lose an hour here. + expect(new Date(2026, 2, 8, 12).getTimezoneOffset()).toBe( + new Date(2026, 2, 8, 0).getTimezoneOffset() - 60 + ) + + expect( + nextAutomationOccurrenceAfter( + '30 2 * * *', + new Date(2026, 0, 1).getTime(), + new Date(2026, 2, 8, 0, 0).getTime() + ) + ).toBe(new Date(2026, 2, 9, 2, 30).getTime()) + }) + + it('fires both repeats of the wall-clock hour local fall-back replays', () => { + vi.stubEnv('TZ', 'America/New_York') + expect(new Date(2026, 10, 1, 12).getTimezoneOffset()).toBe( + new Date(2026, 10, 1, 0).getTimezoneOffset() + 60 + ) + + const first = nextAutomationOccurrenceAfter( + '30 1 * * *', + new Date(2026, 0, 1).getTime(), + new Date(2026, 10, 1, 0, 0).getTime() + ) + const second = nextAutomationOccurrenceAfter( + '30 1 * * *', + new Date(2026, 0, 1).getTime(), + first + ) + + expect(new Date(first).getHours()).toBe(1) + expect(new Date(second).getHours()).toBe(1) + expect(second - first).toBe(60 * 60 * 1000) + + // Pre-existing limit this change does not touch or fix (#20154): local 01:30 is ambiguous, + // so flooring it rebuilds the earlier EDT instant and the scan lands back on the EST repeat + // instead of tomorrow. Pinned so a later DST fix has to update it deliberately. + expect( + nextAutomationOccurrenceAfter('30 1 * * *', new Date(2026, 0, 1).getTime(), second) + ).toBe(second) + }) +}) diff --git a/src/shared/automation-schedule-parsing.ts b/src/shared/automation-schedule-parsing.ts index 746bb434de2..af3ce324f1a 100644 --- a/src/shared/automation-schedule-parsing.ts +++ b/src/shared/automation-schedule-parsing.ts @@ -2,6 +2,12 @@ import type { AutomationSchedulePreset } from './automations-types' import { cronHasPossibleOccurrence } from './automation-cron-occurrence' import { isClipboardTextByteLengthOverLimit } from './clipboard-text' +import { + DAY_NAMES, + MONTH_NAMES, + parseCronField, + type CronParseOptions +} from './automation-cron-field-parsing' export const AUTOMATION_CRON_EXPRESSION_MAX_BYTES = 2 * 1024 export type ParsedRrule = { @@ -24,38 +30,10 @@ export type ParsedCron = { } export type ParsedSchedule = ParsedRrule | ParsedCron +export type { CronParseOptions } + const DAY_CODES = ['SU', 'MO', 'TU', 'WE', 'TH', 'FR', 'SA'] as const const WEEKDAY_CODES = ['MO', 'TU', 'WE', 'TH', 'FR'] as const -const MONTH_NAMES: Record = { - JAN: 1, - FEB: 2, - MAR: 3, - APR: 4, - MAY: 5, - JUN: 6, - JUL: 7, - AUG: 8, - SEP: 9, - OCT: 10, - NOV: 11, - DEC: 12 -} -const DAY_NAMES: Record = { - SU: 0, - MO: 1, - TU: 2, - WE: 3, - TH: 4, - FR: 5, - SA: 6, - SUN: 0, - MON: 1, - TUE: 2, - WED: 3, - THU: 4, - FRI: 5, - SAT: 6 -} function parseRrule(rrule: string): ParsedRrule { const entries = new Map() @@ -88,101 +66,22 @@ function parseRrule(rrule: string): ParsedRrule { return { kind: 'rrule', freq, byDay, byHour, byMinute } } -function parseCronNumber( - value: string, - names: Record | null, - field: string -): number { - const normalized = value.toUpperCase() - const named = names?.[normalized] - const parsed = named ?? Number(normalized) - if (!Number.isInteger(parsed)) { - throw new Error(`Invalid cron ${field}.`) - } - return parsed -} - -function parseCronField(args: { - value: string - min: number - max: number - field: string - names?: Record - normalize?: (value: number) => number -}): Set { - const result = new Set() - for (const rawPart of args.value.split(',')) { - const part = rawPart.trim() - if (!part) { - throw new Error(`Invalid cron ${args.field}.`) - } - const stepParts = part.split('/') - if (stepParts.length > 2) { - throw new Error(`Invalid cron ${args.field}.`) - } - const [rangePart, stepPart] = stepParts - if (!rangePart) { - throw new Error(`Invalid cron ${args.field}.`) - } - const step = stepPart === undefined ? 1 : Number(stepPart) - if (!Number.isInteger(step) || step < 1) { - throw new Error(`Invalid cron ${args.field}.`) - } - - let start: number - let end: number - if (rangePart === '*') { - start = args.min - end = args.max - } else if (rangePart.includes('-')) { - const rangeParts = rangePart.split('-') - if (rangeParts.length !== 2 || !rangeParts[0] || !rangeParts[1]) { - throw new Error(`Invalid cron ${args.field}.`) - } - const [startPart, endPart] = rangeParts - start = parseCronNumber(startPart, args.names ?? null, args.field) - end = parseCronNumber(endPart, args.names ?? null, args.field) - } else { - start = parseCronNumber(rangePart, args.names ?? null, args.field) - end = start - } - - const normalizedStart = args.normalize?.(start) ?? start - const normalizedEnd = args.normalize?.(end) ?? end - if ( - start < args.min || - start > args.max || - end < args.min || - end > args.max || - normalizedStart < args.min || - normalizedStart > args.max || - normalizedEnd < args.min || - normalizedEnd > args.max || - start > end - ) { - throw new Error(`Invalid cron ${args.field}.`) - } - for (let value = start; value <= end; value += step) { - result.add(args.normalize?.(value) ?? value) - } - } - if (result.size === 0) { - throw new Error(`Invalid cron ${args.field}.`) - } - return result -} - -export function parseCronExpression(expression: string): ParsedCron { +export function parseCronExpression( + expression: string, + options: CronParseOptions = {} +): ParsedCron { const parts = getAutomationCronExpressionFields(expression, 6) if (parts.length !== 5) { throw new Error('Cron schedule must have five fields.') } const [minute, hour, dayOfMonth, month, dayOfWeek] = parts + const rejectOversizedStep = options.rejectOversizedStep ?? false const daysOfMonth = parseCronField({ value: dayOfMonth, min: 1, max: 31, - field: 'day of month' + field: 'day of month', + rejectOversizedStep }) const daysOfWeek = parseCronField({ value: dayOfWeek, @@ -190,14 +89,29 @@ export function parseCronExpression(expression: string): ParsedCron { max: 7, field: 'day of week', names: DAY_NAMES, - normalize: (value) => (value === 7 ? 0 : value) + normalize: (value) => (value === 7 ? 0 : value), + distinctValueCount: 7, + rejectOversizedStep }) return { kind: 'cron', - minutes: parseCronField({ value: minute, min: 0, max: 59, field: 'minute' }), - hours: parseCronField({ value: hour, min: 0, max: 23, field: 'hour' }), + minutes: parseCronField({ + value: minute, + min: 0, + max: 59, + field: 'minute', + rejectOversizedStep + }), + hours: parseCronField({ value: hour, min: 0, max: 23, field: 'hour', rejectOversizedStep }), daysOfMonth, - months: parseCronField({ value: month, min: 1, max: 12, field: 'month', names: MONTH_NAMES }), + months: parseCronField({ + value: month, + min: 1, + max: 12, + field: 'month', + names: MONTH_NAMES, + rejectOversizedStep + }), daysOfWeek, dayOfMonthRestricted: daysOfMonth.size !== 31, dayOfWeekRestricted: daysOfWeek.size !== 7 @@ -245,33 +159,50 @@ function isAutomationCronFieldWhitespace(code: number): boolean { ) } -export function parseSchedule(schedule: string): ParsedSchedule { +export function parseSchedule(schedule: string, options: CronParseOptions = {}): ParsedSchedule { const trimmed = schedule.trim() if (trimmed.includes('=')) { return parseRrule(trimmed) } - return parseCronExpression(trimmed) + return parseCronExpression(trimmed, options) } -export function isValidAutomationSchedule(schedule: string): boolean { +function scheduleRuns(schedule: string, options: CronParseOptions): boolean { try { - const parsed = parseSchedule(schedule) - if (parsed.kind === 'cron' && !cronHasPossibleOccurrence(parsed, Date.now())) { - throw new Error('Cron schedule has no possible run.') - } - return true + const parsed = parseSchedule(schedule, options) + return parsed.kind !== 'cron' || cronHasPossibleOccurrence(parsed, Date.now()) } catch { return false } } +function cronScheduleRuns(schedule: string, options: CronParseOptions): boolean { + try { + return cronHasPossibleOccurrence(parseCronExpression(schedule.trim(), options), Date.now()) + } catch { + return false + } +} + +/** Accepts a schedule as new input, oversized-step refusal included (#15895). */ +export function isValidAutomationSchedule(schedule: string): boolean { + return scheduleRuns(schedule, { rejectOversizedStep: true }) +} + export function isValidAutomationCronSchedule(schedule: string): boolean { - try { - const parsed = parseCronExpression(schedule.trim()) - return cronHasPossibleOccurrence(parsed, Date.now()) - } catch { - return false - } + return cronScheduleRuns(schedule, { rejectOversizedStep: true }) +} + +// Whether Orca can still run a schedule it did not just receive. A row saved before the +// oversized-step gate, or one a provider owns, keeps running the cadence it has, so reading +// it back must not re-judge it as input — otherwise renaming an automation would demand +// re-authoring a schedule the user never touched. +export function isRunnableAutomationSchedule(schedule: string): boolean { + return scheduleRuns(schedule, {}) +} + +export function isRunnableAutomationCronSchedule(schedule: string): boolean { + return cronScheduleRuns(schedule, {}) } export function parseAutomationRrule(rrule: string): {