diff --git a/src/cron/delivery-channel-validation.ts b/src/cron/delivery-channel-validation.ts index d87ca2e31550..3328858d66cb 100644 --- a/src/cron/delivery-channel-validation.ts +++ b/src/cron/delivery-channel-validation.ts @@ -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", }); } diff --git a/src/cron/service/failure-alerts.account-routing.test.ts b/src/cron/service/failure-alerts.account-routing.test.ts index 6d4b51af849f..144a719b66eb 100644 --- a/src/cron/service/failure-alerts.account-routing.test.ts +++ b/src/cron/service/failure-alerts.account-routing.test.ts @@ -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, }), diff --git a/src/cron/service/failure-alerts.ts b/src/cron/service/failure-alerts.ts index 0ebe65849bf5..b809f3c46d54 100644 --- a/src/cron/service/failure-alerts.ts +++ b/src/cron/service/failure-alerts.ts @@ -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 }, + job: Pick, ): 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. diff --git a/src/gateway/server-methods/cron.validation.test.ts b/src/gateway/server-methods/cron.validation.test.ts index 06144eb6f7db..19947c95fab0 100644 --- a/src/gateway/server-methods/cron.validation.test.ts +++ b/src/gateway/server-methods/cron.validation.test.ts @@ -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 () => {