mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-28 05:16:23 -06:00
fix(cron): allow clearing failure alert routing fields (#108578)
* fix(cron): clear failure alert routing fields Send explicit nulls for cleared Control UI failure-alert overrides and accept those clears only in cron update patches. Co-Authored-By: OpenAI Codex <noreply@openai.com> * test(cron): satisfy serialized alert patch types Narrow the serialized failure-alert fixture before asserting its cleared fields. Co-Authored-By: OpenAI Codex <noreply@openai.com> * fix(cron): clear failure alert overrides Co-authored-by: 詹幸心0668001037 <zhan.xingxin@xydigit.com> --------- Co-authored-by: OpenAI Codex <noreply@openai.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
@@ -36,6 +36,23 @@ describe("cron protocol validators", () => {
|
||||
expect(validateCronAddParams(minimalAddParams)).toBe(true);
|
||||
});
|
||||
|
||||
it("accepts failure alert field clears only in update patches", () => {
|
||||
const failureAlert = {
|
||||
after: null,
|
||||
channel: null,
|
||||
to: null,
|
||||
cooldownMs: null,
|
||||
includeSkipped: null,
|
||||
mode: null,
|
||||
accountId: null,
|
||||
};
|
||||
|
||||
expect(validateCronUpdateParams({ id: "job-1", patch: { failureAlert } })).toBe(true);
|
||||
expect(validateCronAddParams({ ...minimalAddParams, failureAlert })).toBe(false);
|
||||
expect(validateCronUpdateParams({ id: "job-1", patch: { failureAlert: null } })).toBe(true);
|
||||
expect(validateCronAddParams({ ...minimalAddParams, failureAlert: null })).toBe(false);
|
||||
});
|
||||
|
||||
it("rejects schedule integers that SQLite cannot round-trip safely", () => {
|
||||
const unsafe = Number.MAX_SAFE_INTEGER + 1;
|
||||
expect(
|
||||
|
||||
@@ -277,6 +277,16 @@ export const CronFailureAlertSchema = closedObject({
|
||||
accountId: Type.Optional(NonEmptyString),
|
||||
});
|
||||
|
||||
const CronFailureAlertPatchSchema = closedObject({
|
||||
after: Type.Optional(Type.Union([Type.Integer({ minimum: 1 }), Type.Null()])),
|
||||
channel: Type.Optional(Type.Union([CronAnnounceChannelSchema, Type.Null()])),
|
||||
to: Type.Optional(Type.Union([NonBlankString, Type.Null()])),
|
||||
cooldownMs: Type.Optional(Type.Union([Type.Integer({ minimum: 0 }), Type.Null()])),
|
||||
includeSkipped: Type.Optional(Type.Union([Type.Boolean(), Type.Null()])),
|
||||
mode: Type.Optional(Type.Union([Type.Literal("announce"), Type.Literal("webhook"), Type.Null()])),
|
||||
accountId: Type.Optional(Type.Union([NonEmptyString, Type.Null()])),
|
||||
});
|
||||
|
||||
/** Delivery destination used when failure alerts need a separate target. */
|
||||
export const CronFailureDestinationSchema = closedObject({
|
||||
channel: Type.Optional(CronAnnounceChannelSchema),
|
||||
@@ -500,7 +510,9 @@ export const CronJobPatchSchema = closedObject({
|
||||
wakeMode: Type.Optional(CronWakeModeSchema),
|
||||
payload: Type.Optional(CronPayloadPatchSchema),
|
||||
delivery: Type.Optional(CronDeliveryPatchSchema),
|
||||
failureAlert: Type.Optional(Type.Union([Type.Literal(false), CronFailureAlertSchema])),
|
||||
failureAlert: Type.Optional(
|
||||
Type.Union([Type.Literal(false), CronFailureAlertPatchSchema, Type.Null()]),
|
||||
),
|
||||
state: Type.Optional(CronJobStatePatchSchema),
|
||||
});
|
||||
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
// Cron job patch tests cover applying partial updates to scheduled jobs.
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { resolveCronDeliveryPlan, resolveFailureDestination } from "../delivery-plan.js";
|
||||
import { projectCronJobThroughStorageCodec } from "../store/row-codec.js";
|
||||
import type { CronJob } from "../types.js";
|
||||
import { applyJobPatch } from "./jobs.js";
|
||||
|
||||
@@ -241,3 +242,60 @@ describe("applyJobPatch delivery merge", () => {
|
||||
).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("applyJobPatch failure alert merge", () => {
|
||||
it("clears explicit fields, preserves omitted fields, and persists the result", () => {
|
||||
const job = makeJob({
|
||||
failureAlert: {
|
||||
after: 2,
|
||||
channel: "telegram",
|
||||
to: "123456",
|
||||
cooldownMs: 60_000,
|
||||
includeSkipped: true,
|
||||
mode: "announce",
|
||||
accountId: "bot-a",
|
||||
},
|
||||
});
|
||||
|
||||
applyJobPatch(job, {
|
||||
failureAlert: {
|
||||
after: null,
|
||||
to: null,
|
||||
cooldownMs: null,
|
||||
accountId: null,
|
||||
},
|
||||
});
|
||||
|
||||
expect(job.failureAlert).toEqual({
|
||||
after: undefined,
|
||||
channel: "telegram",
|
||||
to: undefined,
|
||||
cooldownMs: undefined,
|
||||
includeSkipped: true,
|
||||
mode: "announce",
|
||||
accountId: undefined,
|
||||
});
|
||||
expect(projectCronJobThroughStorageCodec(job).failureAlert).toEqual({
|
||||
channel: "telegram",
|
||||
includeSkipped: true,
|
||||
mode: "announce",
|
||||
});
|
||||
|
||||
applyJobPatch(job, {
|
||||
failureAlert: { channel: null, includeSkipped: null, mode: null },
|
||||
});
|
||||
expect(projectCronJobThroughStorageCodec(job).failureAlert).toEqual({});
|
||||
});
|
||||
|
||||
it("clears the whole override only for explicit null", () => {
|
||||
const original = { after: 2, channel: "telegram" as const };
|
||||
const job = makeJob({ failureAlert: original });
|
||||
|
||||
applyJobPatch(job, {});
|
||||
expect(job.failureAlert).toEqual(original);
|
||||
|
||||
applyJobPatch(job, { failureAlert: null });
|
||||
expect(job.failureAlert).toBeUndefined();
|
||||
expect(projectCronJobThroughStorageCodec(job).failureAlert).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -25,6 +25,7 @@ import type {
|
||||
CronDelivery,
|
||||
CronDeliveryPatch,
|
||||
CronFailureAlert,
|
||||
CronFailureAlertPatch,
|
||||
CronJob,
|
||||
CronJobCreate,
|
||||
CronJobPatch,
|
||||
@@ -1252,11 +1253,14 @@ function mergeCronDelivery(
|
||||
|
||||
function mergeCronFailureAlert(
|
||||
existing: CronFailureAlert | false | undefined,
|
||||
patch: CronFailureAlert | false | undefined,
|
||||
patch: CronFailureAlertPatch | false | null | undefined,
|
||||
): CronFailureAlert | false | undefined {
|
||||
if (patch === false) {
|
||||
return false;
|
||||
}
|
||||
if (patch === null) {
|
||||
return undefined;
|
||||
}
|
||||
if (patch === undefined) {
|
||||
return existing;
|
||||
}
|
||||
|
||||
@@ -235,6 +235,11 @@ export type CronFailureAlert = {
|
||||
accountId?: string;
|
||||
};
|
||||
|
||||
/** Partial failure-alert update; null clears an inherited field override. */
|
||||
export type CronFailureAlertPatch = {
|
||||
[K in keyof CronFailureAlert]?: CronFailureAlert[K] | null;
|
||||
};
|
||||
|
||||
/** Payload variants cron can execute in main-session or detached modes. */
|
||||
export type CronPayload =
|
||||
| ({ kind: "systemEvent"; text: string } & CronPayloadToolAllow)
|
||||
@@ -418,6 +423,7 @@ export type CronJobPatch = Partial<
|
||||
| "state"
|
||||
| "payload"
|
||||
| "delivery"
|
||||
| "failureAlert"
|
||||
| "declarationKey"
|
||||
| "displayName"
|
||||
| "owner"
|
||||
@@ -427,5 +433,6 @@ export type CronJobPatch = Partial<
|
||||
trigger?: CronTrigger | null;
|
||||
payload?: CronPayloadPatch;
|
||||
delivery?: CronDeliveryPatch;
|
||||
failureAlert?: CronFailureAlertPatch | false | null;
|
||||
state?: Partial<CronJobState>;
|
||||
};
|
||||
|
||||
@@ -1389,6 +1389,32 @@ describe("cron method validation", () => {
|
||||
expectCronSuccess(respond);
|
||||
});
|
||||
|
||||
it("passes explicit failure alert clears through cron.update", async () => {
|
||||
const failureAlert = {
|
||||
after: null,
|
||||
to: null,
|
||||
cooldownMs: null,
|
||||
accountId: null,
|
||||
};
|
||||
const { context, respond } = await invokeCronUpdate(
|
||||
{ id: "cron-1", patch: { failureAlert } },
|
||||
createCronJob({ failureAlert: { after: 2, to: "123", cooldownMs: 60_000 } }),
|
||||
);
|
||||
|
||||
expect(context.cron.update).toHaveBeenCalledWith("cron-1", { failureAlert });
|
||||
expectCronSuccess(respond);
|
||||
});
|
||||
|
||||
it("passes a whole failure alert override clear through cron.update", async () => {
|
||||
const { context, respond } = await invokeCronUpdate(
|
||||
{ id: "cron-1", patch: { failureAlert: null } },
|
||||
createCronJob({ failureAlert: { after: 2 } }),
|
||||
);
|
||||
|
||||
expect(context.cron.update).toHaveBeenCalledWith("cron-1", { failureAlert: null });
|
||||
expectCronSuccess(respond);
|
||||
});
|
||||
|
||||
it("rejects a blank cron.update display name", async () => {
|
||||
const { context, respond } = await invokeCronUpdate(
|
||||
{ id: "cron-1", patch: { displayName: " " } },
|
||||
|
||||
@@ -1368,6 +1368,102 @@ describe("cron controller", () => {
|
||||
).not.toHaveProperty("cooldownMs");
|
||||
});
|
||||
|
||||
it("clears persisted failure alert routing fields when their edit inputs are blanked", async () => {
|
||||
const request = vi.fn(async (method: string, _payload?: unknown) => {
|
||||
if (method === "cron.update") {
|
||||
return { id: "job-clear-alert-fields" };
|
||||
}
|
||||
if (method === "cron.list") {
|
||||
return { jobs: [{ id: "job-clear-alert-fields" }] };
|
||||
}
|
||||
if (method === "cron.status") {
|
||||
return { enabled: true, jobs: 1, nextWakeAtMs: null };
|
||||
}
|
||||
return {};
|
||||
});
|
||||
const job = {
|
||||
id: "job-clear-alert-fields",
|
||||
name: "Clear failure alert fields",
|
||||
enabled: true,
|
||||
createdAtMs: 0,
|
||||
updatedAtMs: 0,
|
||||
schedule: { kind: "cron" as const, expr: "0 * * * *" },
|
||||
sessionTarget: "isolated" as const,
|
||||
wakeMode: "next-heartbeat" as const,
|
||||
payload: { kind: "agentTurn" as const, message: "run" },
|
||||
delivery: { mode: "announce" as const },
|
||||
failureAlert: {
|
||||
after: 2,
|
||||
channel: "telegram",
|
||||
to: "123456",
|
||||
cooldownMs: 60_000,
|
||||
accountId: "bot-a",
|
||||
},
|
||||
state: {},
|
||||
};
|
||||
const state = createState({
|
||||
client: { request } as unknown as CronState["client"],
|
||||
cronJobs: [job],
|
||||
});
|
||||
|
||||
startCronEdit(state, job);
|
||||
state.cronForm.failureAlertAfter = "";
|
||||
state.cronForm.failureAlertTo = "";
|
||||
state.cronForm.failureAlertCooldownSeconds = "";
|
||||
state.cronForm.failureAlertAccountId = "";
|
||||
await addCronJob(state);
|
||||
|
||||
const updateCall = findRequestCall(request.mock.calls, "cron.update");
|
||||
expectRecordFields(requireRecord(requestPatch(updateCall).failureAlert, "failureAlert"), {
|
||||
after: null,
|
||||
to: null,
|
||||
cooldownMs: null,
|
||||
accountId: null,
|
||||
});
|
||||
// oxlint-disable-next-line unicorn/prefer-structured-clone -- verify the websocket JSON wire shape
|
||||
const serializedPayload = JSON.parse(JSON.stringify(requestPayload(updateCall))) as unknown;
|
||||
expectRecordFields(
|
||||
requireRecord(
|
||||
requireRecord(requireRecord(serializedPayload, "payload").patch, "patch").failureAlert,
|
||||
"failureAlert",
|
||||
),
|
||||
{ after: null, to: null, cooldownMs: null, accountId: null },
|
||||
);
|
||||
});
|
||||
|
||||
it("clears a persisted failure alert override when switching back to inherit", async () => {
|
||||
const request = vi.fn(async (method: string, _payload?: unknown) => {
|
||||
if (method === "cron.update") {
|
||||
return { id: "job-inherit-alert" };
|
||||
}
|
||||
return {};
|
||||
});
|
||||
const job = {
|
||||
id: "job-inherit-alert",
|
||||
name: "Inherit failure alerts",
|
||||
enabled: true,
|
||||
createdAtMs: 0,
|
||||
updatedAtMs: 0,
|
||||
schedule: { kind: "cron" as const, expr: "0 * * * *" },
|
||||
sessionTarget: "isolated" as const,
|
||||
wakeMode: "next-heartbeat" as const,
|
||||
payload: { kind: "agentTurn" as const, message: "run" },
|
||||
failureAlert: { after: 2, channel: "telegram" },
|
||||
state: {},
|
||||
};
|
||||
const state = createState({
|
||||
client: { request } as unknown as CronState["client"],
|
||||
cronJobs: [job],
|
||||
});
|
||||
|
||||
startCronEdit(state, job);
|
||||
state.cronForm.failureAlertMode = "inherit";
|
||||
await addCronJob(state);
|
||||
|
||||
const updateCall = findRequestCall(request.mock.calls, "cron.update");
|
||||
expect(requestPatch(updateCall).failureAlert).toBeNull();
|
||||
});
|
||||
|
||||
it("includes failureAlert=false when disabled per job", async () => {
|
||||
const request = vi.fn(async (method: string, _payload?: unknown) => {
|
||||
if (method === "cron.update") {
|
||||
|
||||
+14
-13
@@ -913,13 +913,14 @@ function normalizePersistedDeliveryChannel(
|
||||
return channel;
|
||||
}
|
||||
|
||||
function buildFailureAlert(form: CronFormState, existingChannel?: string) {
|
||||
function buildFailureAlert(form: CronFormState, existing?: CronJob["failureAlert"]) {
|
||||
if (form.failureAlertMode === "disabled") {
|
||||
return false as const;
|
||||
}
|
||||
if (form.failureAlertMode !== "custom") {
|
||||
return undefined;
|
||||
return existing !== undefined ? null : undefined;
|
||||
}
|
||||
const existingConfig = existing && typeof existing === "object" ? existing : undefined;
|
||||
const after = toNumber(form.failureAlertAfter.trim(), 0);
|
||||
const cooldownRaw = form.failureAlertCooldownSeconds.trim();
|
||||
const cooldownSeconds = cooldownRaw.length > 0 ? toNumber(cooldownRaw, 0) : undefined;
|
||||
@@ -929,18 +930,23 @@ function buildFailureAlert(form: CronFormState, existingChannel?: string) {
|
||||
: undefined;
|
||||
const deliveryMode = form.failureAlertDeliveryMode;
|
||||
const accountId = form.failureAlertAccountId.trim();
|
||||
const to = form.failureAlertTo.trim();
|
||||
const patch: Record<string, unknown> = {
|
||||
after: after > 0 ? Math.floor(after) : undefined,
|
||||
after: after > 0 ? Math.floor(after) : existingConfig?.after !== undefined ? null : undefined,
|
||||
channel: normalizePersistedDeliveryChannel(form.failureAlertChannel, {
|
||||
preserveLastOnUpdate: Boolean(existingChannel),
|
||||
preserveLastOnUpdate: Boolean(existingConfig?.channel),
|
||||
}),
|
||||
to: form.failureAlertTo.trim() || undefined,
|
||||
...(cooldownMs !== undefined ? { cooldownMs } : {}),
|
||||
to: to || (existingConfig?.to ? null : undefined),
|
||||
...(cooldownMs !== undefined
|
||||
? { cooldownMs }
|
||||
: existingConfig?.cooldownMs !== undefined
|
||||
? { cooldownMs: null }
|
||||
: {}),
|
||||
};
|
||||
if (deliveryMode) {
|
||||
patch.mode = deliveryMode;
|
||||
}
|
||||
patch.accountId = accountId || undefined;
|
||||
patch.accountId = accountId || (existingConfig?.accountId ? null : undefined);
|
||||
return patch;
|
||||
}
|
||||
|
||||
@@ -1032,12 +1038,7 @@ export async function addCronJob(state: CronState): Promise<CronSaveResult> {
|
||||
: selectedDeliveryMode === "none"
|
||||
? ({ mode: "none" } as const)
|
||||
: undefined;
|
||||
const failureAlert = buildFailureAlert(
|
||||
form,
|
||||
editingJob?.failureAlert && typeof editingJob.failureAlert === "object"
|
||||
? editingJob.failureAlert.channel
|
||||
: undefined,
|
||||
);
|
||||
const failureAlert = buildFailureAlert(form, editingJob?.failureAlert);
|
||||
const agentId = form.clearAgent ? null : form.agentId.trim();
|
||||
const sessionKeyRaw = form.sessionKey.trim();
|
||||
const sessionKey = sessionKeyRaw || (editingJob?.sessionKey ? null : undefined);
|
||||
|
||||
Reference in New Issue
Block a user