mirror of
https://github.com/getpaseo/paseo.git
synced 2026-11-03 07:03:18 +00:00
Reject cron steps that would silently run hourly (#8607)
* fix(schedules): reject ignored numeric cron steps * fix(schedules): preserve saved numeric steps during recovery Reject unsupported numeric steps at create/update while keeping saved schedules executable with their previous singleton cadence. Co-authored-by: 见安 <73627805+Mar-garet@users.noreply.github.com> --------- Co-authored-by: 见安 <73627805+Mar-garet@users.noreply.github.com>
This commit is contained in:
@@ -13,6 +13,37 @@ describe("cron expression parser", () => {
|
||||
expect(cron.dayOfWeek.matches(0)).toBe(false);
|
||||
});
|
||||
|
||||
it("rejects a step after a standalone number", () => {
|
||||
expect(validateCronExpression("0/15 * * * *")).toBe(
|
||||
"Invalid cron minute step: use * or a range before /",
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["5/1 * * * *", "minute"],
|
||||
["0,5/15 * * * *", "minute"],
|
||||
["* 1/2 * * *", "hour"],
|
||||
["* * 1/2 * *", "day-of-month"],
|
||||
["* * * 1/2 *", "month"],
|
||||
["* * * * 1/2", "day-of-week"],
|
||||
])("rejects ignored numeric steps in %s", (expression, field) => {
|
||||
expect(validateCronExpression(expression)).toBe(
|
||||
`Invalid cron ${field} step: use * or a range before /`,
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ expression: "*/15 * * * *", expected: [0, 15, 30, 45] },
|
||||
{ expression: "0-59/15 * * * *", expected: [0, 15, 30, 45] },
|
||||
{ expression: "5-59/15 * * * *", expected: [5, 20, 35, 50] },
|
||||
])("preserves supported steps in $expression", ({ expression, expected }) => {
|
||||
const cron = parseCronExpression(expression);
|
||||
const minutes = Array.from({ length: 60 }, (_, minute) => minute);
|
||||
const matches = minutes.filter((minute) => cron.minute.matches(minute));
|
||||
|
||||
expect(matches).toEqual(expected);
|
||||
});
|
||||
|
||||
it("reports invalid expressions with server-facing copy", () => {
|
||||
expect(validateCronExpression("* * *")).toBe("Cron expressions must have 5 fields");
|
||||
expect(validateCronExpression("*/5/2 * * * *")).toBe("Invalid cron minute step");
|
||||
|
||||
@@ -90,6 +90,9 @@ function parseField(source: string, bounds: CronFieldBounds): CronFieldMatcher {
|
||||
if (!/^\d+$/.test(base)) {
|
||||
throw new Error(`Invalid cron ${bounds.name} value`);
|
||||
}
|
||||
// COMPAT(cronNumericSteps): added in v0.12.0, remove after 2027-04-17 once
|
||||
// persisted numeric steps are migrated. Validation rejects new numeric steps;
|
||||
// existing schedules retain their historical singleton match.
|
||||
const value = Number.parseInt(base, 10);
|
||||
if (!Number.isInteger(value) || value < bounds.min || value > bounds.max) {
|
||||
throw new Error(`Invalid cron ${bounds.name} value`);
|
||||
@@ -122,6 +125,12 @@ export function parseCronExpression(expression: string): ParsedCronExpression {
|
||||
export function validateCronExpression(expression: string): string | null {
|
||||
try {
|
||||
parseCronExpression(expression);
|
||||
const fields = expression.trim().split(/\s+/);
|
||||
for (const [index, field] of fields.entries()) {
|
||||
if (field.split(",").some((part) => /^\d+\//.test(part))) {
|
||||
return `Invalid cron ${CRON_FIELD_BOUNDS[index].name} step: use * or a range before /`;
|
||||
}
|
||||
}
|
||||
return null;
|
||||
} catch (error) {
|
||||
return error instanceof Error ? error.message : "Invalid cron expression";
|
||||
|
||||
@@ -1,4 +1,7 @@
|
||||
import { parseCronExpression } from "@getpaseo/protocol/schedule/cron-expression";
|
||||
import {
|
||||
parseCronExpression,
|
||||
validateCronExpression,
|
||||
} from "@getpaseo/protocol/schedule/cron-expression";
|
||||
import type { ScheduleCadence } from "@getpaseo/protocol/schedule/types";
|
||||
|
||||
interface CronDateParts {
|
||||
@@ -78,7 +81,10 @@ function createCronDatePartsReader(timeZone: string | undefined): (date: Date) =
|
||||
|
||||
export function validateScheduleCadence(cadence: ScheduleCadence): void {
|
||||
if (cadence.type === "cron") {
|
||||
parseCronExpression(cadence.expression);
|
||||
const error = validateCronExpression(cadence.expression);
|
||||
if (error) {
|
||||
throw new Error(error);
|
||||
}
|
||||
if (cadence.timezone !== undefined) {
|
||||
assertValidTimeZone(cadence.timezone);
|
||||
}
|
||||
|
||||
@@ -2114,6 +2114,81 @@ describe("ScheduleService", () => {
|
||||
});
|
||||
});
|
||||
|
||||
test("rejects numeric cron steps when creating or updating a schedule", async () => {
|
||||
const service = createScheduleService({
|
||||
paseoHome: tempDir,
|
||||
logger: createTestLogger(),
|
||||
agentManager: new AgentManager({ logger: createTestLogger() }),
|
||||
agentStorage,
|
||||
providerSnapshotManager: NO_UNATTENDED_SCHEDULE_POLICY,
|
||||
now: () => now,
|
||||
});
|
||||
const input = {
|
||||
prompt: "Periodic check",
|
||||
cadence: { type: "cron" as const, expression: "*/15 * * * *" },
|
||||
target: {
|
||||
type: "new-agent" as const,
|
||||
config: { provider: "claude" as const, cwd: tempDir },
|
||||
},
|
||||
runOnCreate: false,
|
||||
};
|
||||
|
||||
await expect(
|
||||
service.create({ ...input, cadence: { type: "cron", expression: "0/15 * * * *" } }),
|
||||
).rejects.toThrow("Invalid cron minute step: use * or a range before /");
|
||||
expect(await service.list()).toEqual([]);
|
||||
|
||||
const created = await service.create(input);
|
||||
await expect(
|
||||
service.update({ id: created.id, cadence: { type: "cron", expression: "0/15 * * * *" } }),
|
||||
).rejects.toThrow("Invalid cron minute step: use * or a range before /");
|
||||
expect((await service.inspect(created.id)).cadence).toEqual(input.cadence);
|
||||
});
|
||||
|
||||
test("recovers and runs numeric cron steps persisted by an older daemon", async () => {
|
||||
const service = createScheduleService({
|
||||
paseoHome: tempDir,
|
||||
logger: createTestLogger(),
|
||||
agentManager: new AgentManager({ logger: createTestLogger() }),
|
||||
agentStorage,
|
||||
providerSnapshotManager: NO_UNATTENDED_SCHEDULE_POLICY,
|
||||
now: () => now,
|
||||
runner: async () => ({ agentId: null, output: "ok" }),
|
||||
});
|
||||
const created = await service.create({
|
||||
prompt: "Periodic check",
|
||||
cadence: { type: "cron", expression: "0 * * * *" },
|
||||
target: {
|
||||
type: "new-agent",
|
||||
config: { provider: "claude", cwd: tempDir },
|
||||
},
|
||||
runOnCreate: false,
|
||||
});
|
||||
const store = new ScheduleStore(join(tempDir, "schedules"), createTestLogger());
|
||||
await store.update(created.id, (schedule) => ({
|
||||
...schedule,
|
||||
cadence: { type: "cron", expression: "0/15 * * * *" },
|
||||
}));
|
||||
|
||||
now = new Date("2026-01-01T03:10:00.000Z");
|
||||
try {
|
||||
await service.start();
|
||||
await service.stop();
|
||||
expect((await service.inspect(created.id)).nextRunAt).toBe("2026-01-01T04:00:00.000Z");
|
||||
|
||||
now = new Date("2026-01-01T04:00:00.000Z");
|
||||
await service.tick();
|
||||
const inspected = await service.inspect(created.id);
|
||||
expect(inspected.cadence).toEqual({ type: "cron", expression: "0/15 * * * *" });
|
||||
expect(inspected.status).toBe("active");
|
||||
expect(inspected.nextRunAt).toBe("2026-01-01T05:00:00.000Z");
|
||||
expect(inspected.runs).toHaveLength(1);
|
||||
expect(inspected.runs[0]).toMatchObject({ status: "succeeded", output: "ok" });
|
||||
} finally {
|
||||
await service.stop();
|
||||
}
|
||||
});
|
||||
|
||||
test("advances stale nextRunAt on daemon restart", async () => {
|
||||
const service1 = createScheduleService({
|
||||
paseoHome: tempDir,
|
||||
|
||||
Reference in New Issue
Block a user