fix(cron): failure alerts reject valid routes and lose account ownership (#117765)

* fix(cron): unify failure alert routing and account ownership

* fix(cron): preserve alert ownership across provider aliases
This commit is contained in:
Peter Steinberger
2026-08-01 21:26:48 -07:00
committed by GitHub
parent e7ce514f4e
commit 0745a7e012
4 changed files with 288 additions and 70 deletions
+11 -32
View File
@@ -8,6 +8,7 @@ import {
import { normalizeAccountId } from "../routing/account-id.js";
import { resolveNormalizedAccountEntry } from "../routing/account-lookup.js";
import { isDeliverableMessageChannel, normalizeMessageChannel } from "../utils/message-channel.js";
import { resolveFailureAlert } from "./service/failure-alerts.js";
import type { CronDelivery, CronFailureAlert, CronJobCreate } from "./types.js";
function hasExplicitChannelConfigEntry(cfg: OpenClawConfig): boolean {
@@ -180,45 +181,23 @@ export async function assertValidCronFailureAlert(params: {
failureAlert?: CronFailureAlert | false;
delivery?: CronDelivery;
}) {
const failureAlert = params.failureAlert;
const globalFailureAlert = params.cfg.cron?.failureAlert;
// `false` disables alerts. An unset job alert still inherits an enabled global
// alert, so validate its effective route rather than allowing it to bypass the
// same channel checks as an explicit per-job alert.
if (failureAlert === false || (!failureAlert && globalFailureAlert?.enabled !== true)) {
// Validate the scheduler-owned route so prefix, global, and inheritance
// decisions cannot diverge between mutations and actual alert delivery.
const failureAlert = resolveFailureAlert(
{ deps: { cronConfig: params.cfg.cron } },
{ delivery: params.delivery, failureAlert: params.failureAlert },
);
if (!failureAlert || failureAlert.mode === "webhook") {
return;
}
// Only announce alerts route through a channel type; webhook alerts POST to
// `to`. Resolve the effective mode exactly as runtime does in
// resolveFailureAlert(): a job that omits `mode` inherits the global cron
// failure-alert mode, so validating with a hard "announce" default would
// wrongly reject a channel that a globally webhook-mode alert never uses.
const effectiveMode = failureAlert?.mode ?? globalFailureAlert?.mode;
if (effectiveMode === "webhook") {
return;
}
// Mirror resolveFailureAlert(): the alert inherits the job delivery channel and
// `to`, then the final send channel is resolved from that effective (channel,
// to) pair - a provider prefix in `to` only wins when the effective channel is
// unset/"last". Inheriting even when the alert names no route of its own means a
// routing-changing edit (e.g. flipping mode to announce) that activates a
// legacy-invalid inherited delivery channel is rejected up front rather than
// only when the alert fires.
const effectiveChannel = failureAlert?.channel ?? params.delivery?.channel;
const effectiveTo = failureAlert?.to ?? params.delivery?.to;
const resolvedChannel =
resolveAnnounceValidationChannel({
channel: effectiveChannel,
to: effectiveTo,
}) ?? "last";
assertCompatibleAnnounceTarget({
channel: effectiveChannel,
to: effectiveTo,
channel: failureAlert.channel,
to: failureAlert.to,
field: "failureAlert.channel",
});
await assertConfiguredAnnounceChannel({
cfg: params.cfg,
channel: resolvedChannel,
channel: failureAlert.channel,
field: "failureAlert.channel",
});
}
@@ -1,10 +1,45 @@
import { describe, expect, it, vi } from "vitest";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { resetPluginRuntimeStateForTest, setActivePluginRegistry } from "../../plugins/runtime.js";
import {
createChannelTestPluginBase,
createTestRegistry,
} from "../../test-utils/channel-plugins.js";
import type { CronJob } from "../types.js";
import { resolveFailureAlert } from "./failure-alerts.js";
import { createCronServiceState } from "./state.js";
import { applyJobResult } from "./timer.js";
describe("cron failure alert account routing", () => {
beforeEach(() => {
setActivePluginRegistry(
createTestRegistry(
[
{ id: "telegram", aliases: [], targetPrefixes: ["telegram", "tg"] },
{
id: "googlechat",
aliases: ["gchat", "google-chat"],
targetPrefixes: ["googlechat", "google-chat", "gchat"],
},
].map(({ id, aliases, targetPrefixes }) => {
const plugin = createChannelTestPluginBase({ id });
return {
pluginId: id,
plugin: {
...plugin,
meta: { ...plugin.meta, aliases },
messaging: { targetPrefixes },
},
source: `test:${id}`,
};
}),
),
);
});
afterEach(() => {
resetPluginRuntimeStateForTest();
});
it.each([
{
name: "inherits the primary account when an alert uses its delivery route",
@@ -17,6 +52,136 @@ describe("cron failure alert account routing", () => {
threadId: 42,
},
},
{
name: "inherits the primary account and topic when an alert repeats its recipient",
globalAlert: { enabled: true, after: 1 },
jobAlert: { to: "telegram:19098680" },
expected: {
channel: "telegram",
to: "telegram:19098680",
accountId: "telegram-bot",
threadId: 42,
},
},
{
name: "inherits the primary account and topic through a provider target alias",
globalAlert: { enabled: true, after: 1 },
jobAlert: { to: "tg:19098680" },
expected: {
channel: "telegram",
to: "tg:19098680",
accountId: "telegram-bot",
threadId: 42,
},
},
{
name: "inherits the primary account and topic when delivery uses the target alias",
globalAlert: { enabled: true, after: 1 },
deliveryTo: "tg:19098680",
jobAlert: { to: "telegram:19098680" },
expected: {
channel: "telegram",
to: "telegram:19098680",
accountId: "telegram-bot",
threadId: 42,
},
},
{
name: "normalizes provider alias case and surrounding recipient whitespace",
globalAlert: { enabled: true, after: 1 },
jobAlert: { to: " TG: 19098680 " },
expected: {
channel: "telegram",
to: "TG: 19098680",
accountId: "telegram-bot",
threadId: 42,
},
},
{
name: "inherits the primary account and topic through a selected channel alias",
globalAlert: { enabled: true, after: 1 },
deliveryChannel: "gchat",
deliveryTo: "gchat:RoomA",
jobAlert: { channel: "gchat", to: "googlechat:RoomA" },
expected: {
channel: "googlechat",
to: "googlechat:RoomA",
accountId: "telegram-bot",
threadId: 42,
},
},
{
name: "inherits the primary account and topic when the alert selects a channel alias",
globalAlert: { enabled: true, after: 1 },
deliveryChannel: "googlechat",
deliveryTo: "googlechat:RoomA",
jobAlert: { channel: "gchat", to: "googlechat:RoomA" },
expected: {
channel: "googlechat",
to: "googlechat:RoomA",
accountId: "telegram-bot",
threadId: 42,
},
},
{
name: "inherits the primary account and topic when delivery selects a channel alias",
globalAlert: { enabled: true, after: 1 },
deliveryChannel: "gchat",
deliveryTo: "gchat:RoomA",
jobAlert: { channel: "googlechat", to: "googlechat:RoomA" },
expected: {
channel: "googlechat",
to: "googlechat:RoomA",
accountId: "telegram-bot",
threadId: 42,
},
},
{
name: "does not equate case-sensitive recipient identities across provider aliases",
globalAlert: { enabled: true, after: 1 },
deliveryChannel: "googlechat",
deliveryTo: "googlechat:RoomA",
jobAlert: { to: "gchat:rooma" },
expected: {
channel: "googlechat",
to: "gchat:rooma",
accountId: undefined,
threadId: undefined,
},
},
{
name: "does not equate a provider alias targeting another topic",
globalAlert: { enabled: true, after: 1 },
jobAlert: { to: "tg:19098680:topic:99" },
expected: {
channel: "telegram",
to: "tg:19098680:topic:99",
accountId: undefined,
threadId: undefined,
},
},
{
name: "does not inherit the primary account or topic for another recipient",
globalAlert: { enabled: true, after: 1 },
jobAlert: { to: "telegram:19098681" },
expected: {
channel: "telegram",
to: "telegram:19098681",
accountId: undefined,
threadId: undefined,
},
},
{
name: "does not inherit the primary topic when an aliased recipient uses another account",
globalAlert: { enabled: true, after: 1 },
jobAlert: { to: "tg:19098680", accountId: "alert-bot" },
expected: {
channel: "telegram",
to: "tg:19098680",
accountId: "alert-bot",
threadId: undefined,
},
},
{
name: "prefers an explicit alert account over the primary account",
globalAlert: { enabled: true, after: 1 },
@@ -49,7 +214,8 @@ describe("cron failure alert account routing", () => {
accountId: undefined,
},
},
])("$name", ({ globalAlert, jobAlert, expected }) => {
])("$name", (testCase) => {
const { globalAlert, jobAlert, expected } = testCase;
const state = createCronServiceState({
storePath: "/tmp/openclaw-cron-failure-alert-account-routing.json",
cronEnabled: true,
@@ -72,8 +238,8 @@ describe("cron failure alert account routing", () => {
payload: { kind: "agentTurn", message: "report" },
delivery: {
mode: "announce",
channel: "telegram",
to: "telegram:19098680",
channel: "deliveryChannel" in testCase ? testCase.deliveryChannel : "telegram",
to: "deliveryTo" in testCase ? testCase.deliveryTo : "telegram:19098680",
accountId: "telegram-bot",
threadId: 42,
},
@@ -124,7 +290,19 @@ describe("cron failure alert account routing", () => {
expect(job.state.lastFailureAlertAtMs).toBe(endedAt);
});
it("keeps the primary topic on same-account failure alerts", () => {
it.each([
{ name: "inherited", failureAlert: undefined },
{ name: "explicitly repeated", failureAlert: { to: "telegram:19098680" } },
{ name: "provider-aliased", failureAlert: { to: "tg:19098680" } },
{
name: "mixed selected-channel aliases",
deliveryChannel: "gchat",
deliveryTo: "gchat:RoomA",
failureAlert: { channel: "googlechat", to: "googlechat:RoomA" },
expectedChannel: "googlechat",
},
])("keeps the primary account and topic on $name failure alerts", (testCase) => {
const { failureAlert } = testCase;
const sendCronFailureAlert = vi.fn(async () => undefined);
const state = createCronServiceState({
storePath: "/tmp/openclaw-cron-failure-alert-thread-routing.json",
@@ -148,11 +326,12 @@ describe("cron failure alert account routing", () => {
payload: { kind: "agentTurn", message: "report" },
delivery: {
mode: "announce",
channel: "telegram",
to: "telegram:19098680",
channel: "deliveryChannel" in testCase ? testCase.deliveryChannel : "telegram",
to: "deliveryTo" in testCase ? testCase.deliveryTo : "telegram:19098680",
accountId: "telegram-bot",
threadId: 42,
},
...(failureAlert ? { failureAlert } : {}),
state: {},
};
@@ -165,8 +344,8 @@ describe("cron failure alert account routing", () => {
expect(sendCronFailureAlert).toHaveBeenCalledWith(
expect.objectContaining({
channel: "telegram",
to: "telegram:19098680",
channel: "expectedChannel" in testCase ? testCase.expectedChannel : "telegram",
to: failureAlert?.to ?? "telegram:19098680",
accountId: "telegram-bot",
threadId: 42,
}),
+25 -11
View File
@@ -2,7 +2,11 @@
import { normalizeOptionalLowercaseString } from "@openclaw/normalization-core/string-coerce";
import { truncateUtf16Safe } from "@openclaw/normalization-core/utf16-slice";
import type { FailoverReason } from "../../agents/embedded-agent-helpers/types.js";
import { resolveTargetPrefixedChannel } from "../../infra/outbound/channel-target-prefix.js";
import { normalizeAnyChannelId } from "../../channels/registry-normalize.js";
import {
resolveTargetPrefixedChannel,
stripTargetProviderPrefix,
} from "../../infra/outbound/channel-target-prefix.js";
import type { CronFailureNotificationDelivery, CronJob, CronMessageChannel } from "../types.js";
import type { CronServiceState } from "./state.js";
@@ -43,11 +47,19 @@ function normalizeCronMessageChannel(input: unknown): CronMessageChannel | undef
function resolveFailureAlertChannel(channel: unknown, to?: string): CronMessageChannel | undefined {
const normalized = normalizeCronMessageChannel(channel);
if (normalized && normalized !== "last") {
return normalized;
return normalizeAnyChannelId(normalized) ?? normalized;
}
return normalizeCronMessageChannel(resolveTargetPrefixedChannel(to)) ?? normalized;
}
function normalizeFailureAlertRecipient(channel: CronMessageChannel, to: string): string {
if (resolveTargetPrefixedChannel(to) !== channel) {
return to;
}
// Canonicalize loaded-provider aliases only; recipient/topic ids can be case-sensitive.
return stripTargetProviderPrefix(to, to.slice(0, to.indexOf(":")));
}
function normalizeTo(input: unknown): string | undefined {
if (typeof input !== "string") {
return undefined;
@@ -74,8 +86,8 @@ function clampNonNegativeInt(value: unknown, fallback: number): number {
/** Resolves effective failure-alert policy from job config, delivery defaults, and global cron config. */
export function resolveFailureAlert(
state: CronServiceState,
job: CronJob,
state: { deps: Pick<CronServiceState["deps"], "cronConfig"> },
job: Pick<CronJob, "delivery" | "failureAlert">,
): ResolvedFailureAlert | null {
const globalConfig = state.deps.cronConfig?.failureAlert;
const jobConfig = job.failureAlert === false ? undefined : job.failureAlert;
@@ -108,10 +120,15 @@ export function resolveFailureAlert(
channel === deliveryChannel || (channel === "last" && !deliveryChannel);
const compatibleDeliveryTo = inheritsDeliveryChannel ? deliveryTo : undefined;
const explicitTo = jobTo ?? globalTo;
const inheritsDeliveryRoute =
inheritsDeliveryChannel &&
(explicitTo === undefined ||
explicitTo === deliveryTo ||
(deliveryTo !== undefined &&
normalizeFailureAlertRecipient(channel, explicitTo) ===
normalizeFailureAlertRecipient(channel, deliveryTo)));
const inheritedDeliveryAccountId =
mode !== "webhook" && !explicitTo && inheritsDeliveryChannel
? job.delivery?.accountId
: undefined;
mode !== "webhook" && inheritsDeliveryRoute ? job.delivery?.accountId : undefined;
const accountId =
jobConfig?.accountId ??
(inheritsGlobalRoute ? globalConfig?.accountId : undefined) ??
@@ -119,10 +136,7 @@ export function resolveFailureAlert(
// A topic belongs to its channel, peer, and account; never attach the
// primary topic to an independently routed failure destination.
const inheritsDeliveryThread =
mode !== "webhook" &&
inheritsDeliveryChannel &&
(explicitTo === undefined || explicitTo === deliveryTo) &&
accountId === job.delivery?.accountId;
mode !== "webhook" && inheritsDeliveryRoute && accountId === job.delivery?.accountId;
// Announce alerts inherit the job delivery target; webhook alerts require an
// explicit alert target so chat recipients are not reused as URLs.
@@ -2643,13 +2643,13 @@ describe("cron method validation", () => {
});
it("rejects a provider-prefixed failureAlert.to for an unconfigured channel", async () => {
// No explicit channel, but `slack:...` resolves to slack, which is not
// configured here, so it must be rejected up front rather than at delivery.
// The alert owns its prefixed channel even when primary delivery is valid;
// reject that independently selected channel when it is not configured.
setRuntimeConfig(telegramConfig());
const { context, respond } = await invokeCronUpdate(
{ id: "cron-1", patch: { failureAlert: { to: "slack:C123" } } },
createCronJob(),
createCronJob({ delivery: { mode: "announce", channel: "telegram", to: "telegram:1" } }),
);
expect(context.cron.update).not.toHaveBeenCalled();
@@ -2807,18 +2807,68 @@ describe("cron method validation", () => {
expectCronSuccess(respond);
});
it("rejects a failureAlert.to whose prefix conflicts with the inherited delivery channel", async () => {
// The alert omits its own channel, so runtime sends via the delivery channel
// (telegram); a `slack:`-prefixed target would route to the wrong place, so
// reject it up front instead of letting it fail at delivery.
it.each([
{
name: "provider-prefixed recipient",
delivery: { mode: "announce", channel: "telegram", to: "telegram:1" },
alertTo: "slack:C123",
},
{
name: "provider-alias recipient",
delivery: { mode: "announce", channel: "slack", to: "slack:C123" },
alertTo: "tg:123",
},
] as const)("lets a $name select its own alert channel", async ({ delivery, alertTo }) => {
setRuntimeConfig(telegramSlackConfig());
const { context, respond } = await invokeCronUpdate(
{ id: "cron-1", patch: { failureAlert: { to: "slack:C123" } } },
{ id: "cron-1", patch: { failureAlert: { to: alertTo } } },
createCronJob({ delivery }),
);
expect(context.cron.update).toHaveBeenCalled();
expectCronSuccess(respond);
});
it("accepts a provider-prefixed alert on another channel when creating a job", async () => {
setRuntimeConfig(telegramSlackConfig());
const { context, respond } = await invokeCronAdd(
agentTurnCronParams({
delivery: { mode: "announce", channel: "telegram", to: "telegram:1" },
failureAlert: { to: "slack:C123" },
}),
);
expect(context.cron.add).toHaveBeenCalled();
expectCronSuccess(respond);
});
it("does not inherit a primary recipient for another explicit failure-alert channel", async () => {
setRuntimeConfig(telegramSlackConfig());
const { context, respond } = await invokeCronUpdate(
{ id: "cron-1", patch: { failureAlert: { channel: "slack" } } },
createCronJob({ delivery: { mode: "announce", channel: "telegram", to: "telegram:1" } }),
);
expect(context.cron.update).not.toHaveBeenCalled();
expect(context.cron.update).toHaveBeenCalled();
expectCronSuccess(respond);
});
it("rejects an unconfigured inherited global failure-alert channel", async () => {
setRuntimeConfig({
...telegramConfig(),
cron: { failureAlert: { enabled: true, channel: "slack", to: "slack:C123" } },
});
const { context, respond } = await invokeCronAdd(
agentTurnCronParams({
delivery: { mode: "announce", channel: "telegram", to: "telegram:1" },
}),
);
expect(context.cron.add).not.toHaveBeenCalled();
expectResponseError(respond, {
code: "INVALID_REQUEST",
messageIncludes: "failureAlert.channel",
@@ -2893,10 +2943,9 @@ describe("cron method validation", () => {
expectCronSuccess(respond);
});
it("revalidates an inherited failureAlert when a delivery-only patch changes the channel", async () => {
// The alert has no own channel, so it inherits delivery. Switching delivery
// from slack to telegram makes its slack-prefixed target route wrong, so the
// delivery-only edit must re-check the alert even though the patch omits it.
it("keeps a provider-prefixed failure alert when primary delivery changes channels", async () => {
// A provider-prefixed alert owns its channel even without `channel`, so a
// primary-delivery change cannot invalidate that independent destination.
setRuntimeConfig(telegramSlackConfig());
const { context, respond } = await invokeCronUpdate(
@@ -2907,11 +2956,8 @@ describe("cron method validation", () => {
}),
);
expect(context.cron.update).not.toHaveBeenCalled();
expectResponseError(respond, {
code: "INVALID_REQUEST",
messageIncludes: "failureAlert.channel",
});
expect(context.cron.update).toHaveBeenCalled();
expectCronSuccess(respond);
});
it("does not block a non-routing delivery edit on a job with a stale explicit alert channel", async () => {