From b66828f103bfd857412ead3cd52a57cba882907c Mon Sep 17 00:00:00 2001 From: Kai Liu Date: Mon, 1 Jun 2026 03:06:30 +0800 Subject: [PATCH 1/7] fix(cron): auto-disable jobs whose target channel is unreachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a cron job's send hits a permanent channel-access error (channel_not_found, not_in_channel, is_archived, token_revoked, etc.), the scheduler now disables the job and records the reason. Previously the same broken job would re-fire on every cron tick and capture an identical Sentry event for the lifetime of the daemon — see Sentry ODE-DEAMON-7 (5 events over ~5 weeks from one stuck job). - Add @/shared/delivery/permanent-error with cross-platform detection (Slack error strings, Discord numeric codes, Lark chat_not_found) and intentionally narrow classification: transient failures stay retryable. - Wire it into both the success-path send and the failure-notification send in runCronJob. The check on the notify path uses the *notify* error, not the original turn error, so a transient agent timeout doesn't disable a job when the channel is fine. - Add unit tests covering Slack/Discord/Lark error shapes and ensuring rate-limits / 5xx / ECONNRESET / message_not_found stay retryable. --- packages/core/cron/scheduler.ts | 98 ++++++++++++++++++- .../shared/delivery/permanent-error.test.ts | 67 +++++++++++++ packages/shared/delivery/permanent-error.ts | 94 ++++++++++++++++++ 3 files changed, 258 insertions(+), 1 deletion(-) create mode 100644 packages/shared/delivery/permanent-error.test.ts create mode 100644 packages/shared/delivery/permanent-error.ts diff --git a/packages/core/cron/scheduler.ts b/packages/core/cron/scheduler.ts index 3eb9efa3..74b2165a 100644 --- a/packages/core/cron/scheduler.ts +++ b/packages/core/cron/scheduler.ts @@ -15,6 +15,7 @@ import { markCronJobFailed, markCronJobRunning, markCronJobTriggered, + patchCronJob, reconcileInterruptedCronJobs, } from "@/config/local/cron-jobs"; import { @@ -37,6 +38,7 @@ import { sendSlackChannelMessage } from "@/core/runtime/slack-senders"; import { buildSessionEnvironment, prepareSessionWorkspace } from "@/core/session"; import { sendChannelMessage as sendDiscordChannelMessage } from "@/ims/discord/client"; import { sendChannelMessage as sendLarkChannelMessage } from "@/ims/lark/client"; +import { isPermanentChannelError } from "@/shared/delivery/permanent-error"; import { log } from "@/utils"; const CRON_POLL_INTERVAL_MS = 15_000; @@ -265,6 +267,57 @@ async function prepareCronSession(job: CronJobRecord, runId: string): Promise<{ return { session, sessionId, cwd, created }; } +/** + * Auto-disable a cron job whose destination channel is permanently + * unreachable (bot kicked, channel archived, token revoked, etc.). Without + * this, every cron tick would re-attempt the same send and capture an + * identical Sentry event for the lifetime of the daemon — see + * `isPermanentChannelError` for the precise classification. + * + * Best-effort: we never let bookkeeping failures here shadow the original + * delivery error. The function does NOT throw. + */ +async function disableCronJobForPermanentChannelError( + job: CronJobRecord, + error: unknown, + agentResultDetailId: string | null, +): Promise { + const reason = error instanceof Error ? error.message : String(error); + const summary = `auto-disabled: destination channel unreachable (${reason})`; + if (agentResultDetailId) { + try { + failAgentResult({ detailId: agentResultDetailId, errorText: summary }); + } catch (failError) { + log.warn("Failed to mark cron agent_result detail as failed during auto-disable", { + detailId: agentResultDetailId, + error: String(failError), + }); + } + } + try { + patchCronJob(job.id, { enabled: false }); + } catch (disableError) { + log.warn("Failed to disable cron job after permanent channel error", { + cronJobId: job.id, + error: String(disableError), + }); + } + try { + markCronJobFailed(job.id, summary); + } catch (markError) { + log.warn("Failed to mark auto-disabled cron job as failed", { + cronJobId: job.id, + error: String(markError), + }); + } + log.warn("Auto-disabled cron job: destination channel unreachable", { + cronJobId: job.id, + title: job.title, + channelId: job.channelId, + error: reason, + }); +} + async function runCronJob(job: CronJobRecord, minuteStartMs: number): Promise { const agent = createAgentAdapter(); const runId = getCronRunId(minuteStartMs); @@ -350,7 +403,22 @@ async function runCronJob(job: CronJobRecord, minuteStartMs: number): Promise { + test("matches Slack channel_not_found in message", () => { + expect( + isPermanentChannelError( + new Error("An API error occurred: channel_not_found"), + ), + ).toBe(true); + }); + + test("matches Slack SDK error shape with data.error", () => { + const err = Object.assign(new Error("slack_webapi_platform_error"), { + data: { error: "channel_not_found" }, + }); + expect(isPermanentChannelError(err)).toBe(true); + }); + + test("matches Slack not_in_channel / is_archived / account_inactive", () => { + expect(isPermanentChannelError(new Error("not_in_channel"))).toBe(true); + expect(isPermanentChannelError(new Error("is_archived"))).toBe(true); + expect(isPermanentChannelError(new Error("channel_is_archived"))).toBe(true); + expect(isPermanentChannelError(new Error("account_inactive"))).toBe(true); + }); + + test("matches Slack auth-revoked errors", () => { + expect(isPermanentChannelError(new Error("token_revoked"))).toBe(true); + expect(isPermanentChannelError(new Error("invalid_auth"))).toBe(true); + expect(isPermanentChannelError(new Error("missing_scope"))).toBe(true); + }); + + test("matches Discord-style numeric codes (Missing Access, Unknown Channel)", () => { + expect( + isPermanentChannelError(Object.assign(new Error("Missing Access"), { code: 50001 })), + ).toBe(true); + expect( + isPermanentChannelError(Object.assign(new Error("Unknown Channel"), { code: 10003 })), + ).toBe(true); + }); + + test("matches Lark chat_not_found", () => { + expect(isPermanentChannelError(new Error("chat_not_found: chat does not exist"))).toBe(true); + }); + + test("ignores transient / retryable failures", () => { + expect(isPermanentChannelError(new Error("status 429"))).toBe(false); + expect(isPermanentChannelError(new Error("ECONNRESET"))).toBe(false); + expect(isPermanentChannelError(new Error("status 500"))).toBe(false); + expect(isPermanentChannelError(new Error("socket hang up"))).toBe(false); + expect(isPermanentChannelError(new Error("rate_limited"))).toBe(false); + }); + + test("ignores nullish / empty inputs", () => { + expect(isPermanentChannelError(null)).toBe(false); + expect(isPermanentChannelError(undefined)).toBe(false); + expect(isPermanentChannelError("")).toBe(false); + expect(isPermanentChannelError({})).toBe(false); + }); + + test("ignores message_not_found (that's a delete/update race, not permanent)", () => { + // `message_not_found` is already handled as a benign update/delete race + // in @/core/observability/sentry. It is NOT a channel-access problem and + // must remain retryable from this helper's perspective. + expect(isPermanentChannelError(new Error("message_not_found"))).toBe(false); + }); +}); diff --git a/packages/shared/delivery/permanent-error.ts b/packages/shared/delivery/permanent-error.ts new file mode 100644 index 00000000..bce81fb6 --- /dev/null +++ b/packages/shared/delivery/permanent-error.ts @@ -0,0 +1,94 @@ +/** + * Detect "permanent" channel-access delivery failures — cases where retrying + * the same send is guaranteed to fail until a human intervenes (bot removed + * from the channel, channel archived/deleted, workspace token revoked, etc.). + * + * This is used by the cron scheduler to auto-disable a job whose target + * channel is unreachable. Without this guard the scheduler keeps firing the + * job every cron tick, each failure capturing a fresh Sentry event with the + * same `channel_not_found` fingerprint — turning one broken job into a + * permanent noise source (see Sentry ODE-DEAMON-7). + * + * The detection is intentionally narrow: we only return `true` for errors + * whose remediation is *outside the daemon's control*. Transient failures + * (network blips, 5xx, rate limits) must remain retryable and therefore + * MUST NOT be classified as permanent here. + */ + +// Tokens are matched case-insensitively against the stringified error. +// +// Slack: +// - channel_not_found: channel doesn't exist OR bot can't see it +// - not_in_channel: bot needs to be invited to post +// - is_archived / channel_is_archived: channel was archived +// - account_inactive: workspace user/account disabled +// - token_revoked / invalid_auth / not_authed: token no longer works +// - missing_scope: token lacks chat:write or similar — retrying won't help +// +// Discord (discord.js DiscordAPIError exposes `code`): +// - 10003 Unknown Channel +// - 50001 Missing Access +// - 50013 Missing Permissions +// - 50007 Cannot send messages to this user +// +// Lark (open-platform error codes vary; we match the common "permission +// denied" / "chat not exist" message text): +// - chat_not_found / chat does not exist +// - permission denied (generic; only matched when paired with chat context) +const PERMANENT_MESSAGE_TOKENS = [ + // Slack + "channel_not_found", + "not_in_channel", + "is_archived", + "channel_is_archived", + "account_inactive", + "token_revoked", + "invalid_auth", + "not_authed", + "missing_scope", + // Lark + "chat_not_found", + "chat not exist", +]; + +// Discord.js surfaces the raw API code on `err.code` as a number. +const PERMANENT_DISCORD_CODES = new Set([ + 10003, // Unknown Channel + 50001, // Missing Access + 50013, // Missing Permissions + 50007, // Cannot send messages to this user +]); + +function stringifyError(err: unknown): string { + if (err instanceof Error) return err.message; + if (typeof err === "string") return err; + try { + return JSON.stringify(err); + } catch { + return String(err); + } +} + +export function isPermanentChannelError(err: unknown): boolean { + if (!err) return false; + const message = stringifyError(err).toLowerCase(); + for (const token of PERMANENT_MESSAGE_TOKENS) { + if (message.includes(token)) return true; + } + if (typeof err === "object" && err !== null) { + const code = (err as { code?: unknown }).code; + if (typeof code === "number" && PERMANENT_DISCORD_CODES.has(code)) { + return true; + } + // Slack SDK exposes the API error string on `err.data.error`. + const data = (err as { data?: { error?: unknown } }).data; + const apiError = data?.error; + if (typeof apiError === "string") { + const normalized = apiError.toLowerCase(); + for (const token of PERMANENT_MESSAGE_TOKENS) { + if (normalized.includes(token)) return true; + } + } + } + return false; +} From 4decf95505d07a0aafcbe81f74116aa342e1238f Mon Sep 17 00:00:00 2001 From: Kai Liu Date: Tue, 2 Jun 2026 03:07:49 +0800 Subject: [PATCH 2/7] fix(cron): bypass channel revalidation when auto-disabling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Codex P2 review on PR #211. The previous patch routed auto-disable through patchCronJob -> updateCronJob -> getChannelSnapshot, which throws "Channel not found in configured workspaces" when the cron row's destination channel has since been removed from ode.json. In that exact case (stale local config, bot kicked, workspace re-onboarded) the auto-disable was silently swallowed and the row stayed enabled — the scheduler would keep firing and capturing the same channel_not_found Sentry event every tick, defeating the point of the PR. Add a direct-SQL disableCronJob(id) helper that flips enabled=0 without re-validating channel config, and use it from both call sites (success-path send and failure-notification send). Tests: new disableCronJob suite covers - happy path (enabled true -> false, returns true) - idempotent re-disable (returns false) - missing row (returns false) - stale-channel case where patchCronJob throws but disableCronJob still succeeds (the regression Codex flagged) bun test: 418 pass, 1 skip, 0 fail. --- packages/config/local/cron-jobs.test.ts | 76 +++++++++++++++++++++++++ packages/config/local/cron-jobs.ts | 25 ++++++++ packages/core/cron/scheduler.ts | 6 +- 3 files changed, 104 insertions(+), 3 deletions(-) diff --git a/packages/config/local/cron-jobs.test.ts b/packages/config/local/cron-jobs.test.ts index 2e47e081..b8a77dbd 100644 --- a/packages/config/local/cron-jobs.test.ts +++ b/packages/config/local/cron-jobs.test.ts @@ -9,10 +9,12 @@ import { clearCronJobsForTests, closeCronJobDatabaseForTests, createCronJob, + disableCronJob, getCronJobById, markCronJobCompleted, markCronJobFailed, markCronJobRunning, + patchCronJob, reconcileInterruptedCronJobs, } from "./cron-jobs"; @@ -202,3 +204,77 @@ describe("reconcileInterruptedCronJobs", () => { expect(getCronJobById(job.id)?.lastRunStatus).toBe("failed"); }); }); + +describe("disableCronJob", () => { + test("flips enabled to false and reports the transition", () => { + const job = createCronJob({ + title: "to-disable", + cronExpression: "*/5 * * * *", + channelId: "C_TEST", + messageText: "hi", + }); + expect(getCronJobById(job.id)?.enabled).toBe(true); + + const changed = disableCronJob(job.id); + expect(changed).toBe(true); + expect(getCronJobById(job.id)?.enabled).toBe(false); + }); + + test("returns false when the row is already disabled (idempotent)", () => { + const job = createCronJob({ + title: "already-off", + cronExpression: "*/5 * * * *", + channelId: "C_TEST", + messageText: "hi", + enabled: false, + }); + const changed = disableCronJob(job.id); + expect(changed).toBe(false); + expect(getCronJobById(job.id)?.enabled).toBe(false); + }); + + test("returns false when the row does not exist", () => { + expect(disableCronJob("does-not-exist")).toBe(false); + }); + + test("succeeds even when the row's channel was removed from local config", () => { + // This is the scenario `disableCronJob` exists to solve: + // a cron job points at a channel that has since been removed from + // ode.json. `patchCronJob` would throw via `getChannelSnapshot`, but + // the direct-SQL `disableCronJob` must still flip `enabled` to false + // so the scheduler stops firing the row. + const job = createCronJob({ + title: "stale-channel", + cronExpression: "*/5 * * * *", + channelId: "C_TEST", + messageText: "hi", + }); + + // Drop the channel from the on-disk config so getChannelSnapshot fails. + const emptyConfig = { + user: {}, + workspaces: [ + { + id: "ws-test", + name: "Test Workspace", + type: "slack", + channelDetails: [], // C_TEST is gone + }, + ], + }; + fs.writeFileSync(ODE_CONFIG_FILE, JSON.stringify(emptyConfig)); + invalidateOdeConfigCache(); + + // Sanity check: patchCronJob blows up because it re-validates the + // channel before writing. This is exactly the bug Codex flagged on + // PR #211 and is the reason we route around it. + expect(() => patchCronJob(job.id, { enabled: false })).toThrow( + /Channel not found/i, + ); + expect(getCronJobById(job.id)?.enabled).toBe(true); + + // disableCronJob bypasses channel resolution entirely. + expect(disableCronJob(job.id)).toBe(true); + expect(getCronJobById(job.id)?.enabled).toBe(false); + }); +}); diff --git a/packages/config/local/cron-jobs.ts b/packages/config/local/cron-jobs.ts index a07d7b4c..42621480 100644 --- a/packages/config/local/cron-jobs.ts +++ b/packages/config/local/cron-jobs.ts @@ -384,6 +384,31 @@ export function patchCronJob(id: string, params: PatchCronJobParams): CronJobRec return updateCronJob(id, merged); } +/** + * Directly flip `enabled` to 0 without re-validating the rest of the cron + * row. Unlike `patchCronJob`, this does NOT re-resolve the row's channel + * via `getChannelSnapshot`, so it is safe to call when the destination + * channel was removed from the local config — which is exactly the + * scenario that motivates auto-disable in the first place (bot kicked, + * workspace re-onboarded, channel id stale, etc.). + * + * Returns `true` if a row actually transitioned from enabled→disabled, so + * callers can avoid duplicate log/notification work if another writer beat + * them to it. A missing row returns `false` as well. + */ +export function disableCronJob(id: string): boolean { + const db = getDatabase(); + const result = db.query(` + UPDATE cron_jobs + SET + enabled = 0, + updated_at = ? + WHERE id = ? + AND enabled = 1 + `).run(Date.now(), id); + return result.changes > 0; +} + export function markCronJobTriggered(id: string, minuteStartMs: number): boolean { const db = getDatabase(); const result = db.query(` diff --git a/packages/core/cron/scheduler.ts b/packages/core/cron/scheduler.ts index 74b2165a..747ff9f5 100644 --- a/packages/core/cron/scheduler.ts +++ b/packages/core/cron/scheduler.ts @@ -9,13 +9,13 @@ import { } from "@/config"; import { type CronJobRecord, + disableCronJob, getCronJobById, listEnabledCronJobs, markCronJobCompleted, markCronJobFailed, markCronJobRunning, markCronJobTriggered, - patchCronJob, reconcileInterruptedCronJobs, } from "@/config/local/cron-jobs"; import { @@ -295,7 +295,7 @@ async function disableCronJobForPermanentChannelError( } } try { - patchCronJob(job.id, { enabled: false }); + disableCronJob(job.id); } catch (disableError) { log.warn("Failed to disable cron job after permanent channel error", { cronJobId: job.id, @@ -485,7 +485,7 @@ async function runCronJob(job: CronJobRecord, minuteStartMs: number): Promise Date: Wed, 3 Jun 2026 03:04:32 +0800 Subject: [PATCH 3/7] fix(discord): forward DiscordAPIError code through resolveTextChannel wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Codex P2 review on PR #211. resolveTextChannel() catches each client.channels.fetch() failure and then rethrows a generic Error("Discord channel ... is not text-based or inaccessible") with no `code` attached. That generic error reaches isPermanentChannelError(), which checks for numeric DiscordAPIErrorcode (10003 Unknown Channel / 50001 Missing Access / etc.) and falls through to false — so a Discord cron job whose bot has been removed from the target channel stays enabled and keeps firing every tick, exactly the noise pattern this PR exists to fix. Capture every Discord API code from each fetch attempt, pick a permanent one (10003 / 50001 / 50013 / 50007) when available, and attach it to the thrown wrapper as `code` (with the full list on `discordErrorCodes` for diagnostics). The wrapper's message is unchanged so callers that match on the string still work. Add a permanent-error.test.ts case asserting the wrapper Error with a forwarded code is classified as permanent. bun test: 419 pass, 1 skip, 0 fail. --- packages/ims/discord/client.ts | 31 ++++++++++++++++++- .../shared/delivery/permanent-error.test.ts | 12 +++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/packages/ims/discord/client.ts b/packages/ims/discord/client.ts index 62e97934..8c188f5e 100644 --- a/packages/ims/discord/client.ts +++ b/packages/ims/discord/client.ts @@ -117,6 +117,20 @@ async function maybeHandleLauncherCommand(params: { } async function resolveTextChannel(channelId: string, processorId?: string) { const attempts: string[] = []; + // Collect Discord API error codes across every fetch attempt so the + // caller can detect permanent-access failures (Unknown Channel / Missing + // Access / etc.) even though we re-throw a generic wrapper Error below. + // discord.js surfaces these on `err.code` as numbers; we forward them on + // the wrapper as `discordErrorCodes` and also expose the first as `code` + // so `isPermanentChannelError` can pick it up via the `code` shape. + const discordErrorCodes: number[] = []; + const captureCode = (error: unknown) => { + if (typeof error === "object" && error !== null) { + const code = (error as { code?: unknown }).code; + if (typeof code === "number") discordErrorCodes.push(code); + } + }; + if (processorId) { const pinnedClient = discordClientByProcessorId.get(processorId); if (pinnedClient) { @@ -127,6 +141,7 @@ async function resolveTextChannel(channelId: string, processorId?: string) { } attempts.push(`bot=${pinnedClient.user?.id || "unknown"}: channel_not_text_or_missing`); } catch (error) { + captureCode(error); const errorMessage = error instanceof Error ? error.message : String(error); attempts.push(`bot=${pinnedClient.user?.id || "unknown"}: ${errorMessage}`); } @@ -142,6 +157,7 @@ async function resolveTextChannel(channelId: string, processorId?: string) { } attempts.push(`bot=${client.user?.id || "unknown"}: channel_not_text_or_missing`); } catch (error) { + captureCode(error); const errorMessage = error instanceof Error ? error.message : String(error); attempts.push(`bot=${client.user?.id || "unknown"}: ${errorMessage}`); } @@ -155,7 +171,20 @@ async function resolveTextChannel(channelId: string, processorId?: string) { }); } - throw new Error(`Discord channel ${channelId} is not text-based or inaccessible`); + const wrapper = new Error(`Discord channel ${channelId} is not text-based or inaccessible`); + if (discordErrorCodes.length > 0) { + // Prefer the most informative code: prioritise "permanent" classes + // (10003 Unknown Channel, 50001 Missing Access, 50013 Missing Perms, + // 50007 Cannot DM) so isPermanentChannelError can disable the cron row + // even if some bots failed transiently. + const PERMANENT_PRIORITY = new Set([10003, 50001, 50013, 50007]); + const permanent = discordErrorCodes.find((c) => PERMANENT_PRIORITY.has(c)); + (wrapper as Error & { code?: number; discordErrorCodes?: number[] }).code = + permanent ?? discordErrorCodes[0]; + (wrapper as Error & { code?: number; discordErrorCodes?: number[] }).discordErrorCodes = + [...discordErrorCodes]; + } + throw wrapper; } async function buildDiscordContext( diff --git a/packages/shared/delivery/permanent-error.test.ts b/packages/shared/delivery/permanent-error.test.ts index ef37e15c..84a57e74 100644 --- a/packages/shared/delivery/permanent-error.test.ts +++ b/packages/shared/delivery/permanent-error.test.ts @@ -39,6 +39,18 @@ describe("isPermanentChannelError", () => { ).toBe(true); }); + test("matches the Discord resolveTextChannel wrapper when it carries a DiscordAPIError code", () => { + // resolveTextChannel() in packages/ims/discord/client.ts wraps the + // underlying DiscordAPIError in a generic Error("... is not text-based + // or inaccessible"). The wrapper forwards the original numeric `code` + // so this helper can still classify it as permanent. + const wrapper = Object.assign( + new Error("Discord channel 123 is not text-based or inaccessible"), + { code: 10003 }, + ); + expect(isPermanentChannelError(wrapper)).toBe(true); + }); + test("matches Lark chat_not_found", () => { expect(isPermanentChannelError(new Error("chat_not_found: chat does not exist"))).toBe(true); }); From e1a33ea782d76f7602a6e6df37b0b413bb119c6b Mon Sep 17 00:00:00 2001 From: Kai Liu Date: Tue, 16 Jun 2026 03:06:37 +0800 Subject: [PATCH 4/7] fix(delivery): handle Lark 'chat does not exist' and Discord mixed-bot failures Addresses two unaddressed Codex review comments on PR #211: 1. Lark deleted/stale chats: the Lark client re-throws the raw 'msg' field for some payloads, which surfaces as 'chat does not exist'. The shorter 'chat not exist' token does not match (the 'does' infix breaks the substring), so cron jobs targeting deleted Lark chats stayed enabled. Add 'chat does not exist' as an explicit token. 2. Discord multi-bot fetch races: when no processorId pins a bot, resolveTextChannel iterates every Discord client; mixing one bot's transient error with another (unrelated) bot's 'Missing Access' would forward the permanent code on the wrapper and auto-disable the cron job even though a retry could succeed. Track the pinned bot's code separately and only treat the multi-bot wrapper as permanent when either (a) the pinned bot returned a code, or (b) every captured code is permanent. Tests: - Added 'matches Lark deleted-chat message text alone' covering the bare 'chat does not exist' string. - Added 'ignores Discord resolveTextChannel wrapper without a forwarded code' covering the mixed-bot case (no code or transient code on the wrapper). - Full suite green: 435 pass, 1 skip, 0 fail. --- packages/ims/discord/client.ts | 53 +++++++++++++++---- .../shared/delivery/permanent-error.test.ts | 27 ++++++++++ packages/shared/delivery/permanent-error.ts | 10 +++- 3 files changed, 77 insertions(+), 13 deletions(-) diff --git a/packages/ims/discord/client.ts b/packages/ims/discord/client.ts index 8c188f5e..66b83621 100644 --- a/packages/ims/discord/client.ts +++ b/packages/ims/discord/client.ts @@ -123,17 +123,30 @@ async function resolveTextChannel(channelId: string, processorId?: string) { // discord.js surfaces these on `err.code` as numbers; we forward them on // the wrapper as `discordErrorCodes` and also expose the first as `code` // so `isPermanentChannelError` can pick it up via the `code` shape. + // + // The pinned bot's code is tracked separately: when `processorId` is + // provided we know which bot actually owns the channel, so its result + // is authoritative and unrelated bots' "permanent" errors (e.g. another + // workspace's bot legitimately returning `Missing Access` for a channel + // it cannot see) MUST NOT promote the wrapper to permanent. See PR #211 + // discussion: "Avoid disabling Discord jobs on mixed fetch failures". const discordErrorCodes: number[] = []; - const captureCode = (error: unknown) => { + let pinnedBotErrorCode: number | undefined; + let pinnedBotAttempted = false; + const captureCode = (error: unknown, isPinnedBot: boolean) => { if (typeof error === "object" && error !== null) { const code = (error as { code?: unknown }).code; - if (typeof code === "number") discordErrorCodes.push(code); + if (typeof code === "number") { + discordErrorCodes.push(code); + if (isPinnedBot) pinnedBotErrorCode = code; + } } }; if (processorId) { const pinnedClient = discordClientByProcessorId.get(processorId); if (pinnedClient) { + pinnedBotAttempted = true; try { const channel = await pinnedClient.channels.fetch(channelId); if (channel && channel.isTextBased()) { @@ -141,7 +154,7 @@ async function resolveTextChannel(channelId: string, processorId?: string) { } attempts.push(`bot=${pinnedClient.user?.id || "unknown"}: channel_not_text_or_missing`); } catch (error) { - captureCode(error); + captureCode(error, true); const errorMessage = error instanceof Error ? error.message : String(error); attempts.push(`bot=${pinnedClient.user?.id || "unknown"}: ${errorMessage}`); } @@ -157,7 +170,7 @@ async function resolveTextChannel(channelId: string, processorId?: string) { } attempts.push(`bot=${client.user?.id || "unknown"}: channel_not_text_or_missing`); } catch (error) { - captureCode(error); + captureCode(error, false); const errorMessage = error instanceof Error ? error.message : String(error); attempts.push(`bot=${client.user?.id || "unknown"}: ${errorMessage}`); } @@ -173,14 +186,32 @@ async function resolveTextChannel(channelId: string, processorId?: string) { const wrapper = new Error(`Discord channel ${channelId} is not text-based or inaccessible`); if (discordErrorCodes.length > 0) { - // Prefer the most informative code: prioritise "permanent" classes - // (10003 Unknown Channel, 50001 Missing Access, 50013 Missing Perms, - // 50007 Cannot DM) so isPermanentChannelError can disable the cron row - // even if some bots failed transiently. const PERMANENT_PRIORITY = new Set([10003, 50001, 50013, 50007]); - const permanent = discordErrorCodes.find((c) => PERMANENT_PRIORITY.has(c)); - (wrapper as Error & { code?: number; discordErrorCodes?: number[] }).code = - permanent ?? discordErrorCodes[0]; + + // Pick which code to forward as the `code` field consumed by + // `isPermanentChannelError`. Rules: + // 1. If a pinned bot was attempted, its outcome is authoritative — + // forward its code (if any). Other bots' permanent codes for the + // same channel are unrelated noise. + // 2. If no pinned bot was attempted (e.g. cron top-level send with + // no processorId), only treat the failure as permanent when every + // captured code is permanent. Mixed transient+permanent means at + // least one bot might succeed on retry, so do NOT promote. + let forwardedCode: number | undefined; + if (pinnedBotAttempted) { + forwardedCode = pinnedBotErrorCode; + } else { + const allPermanent = discordErrorCodes.every((c) => PERMANENT_PRIORITY.has(c)); + if (allPermanent) { + forwardedCode = discordErrorCodes.find((c) => PERMANENT_PRIORITY.has(c)); + } else { + forwardedCode = discordErrorCodes[0]; + } + } + + if (forwardedCode !== undefined) { + (wrapper as Error & { code?: number; discordErrorCodes?: number[] }).code = forwardedCode; + } (wrapper as Error & { code?: number; discordErrorCodes?: number[] }).discordErrorCodes = [...discordErrorCodes]; } diff --git a/packages/shared/delivery/permanent-error.test.ts b/packages/shared/delivery/permanent-error.test.ts index 84a57e74..117c4ec9 100644 --- a/packages/shared/delivery/permanent-error.test.ts +++ b/packages/shared/delivery/permanent-error.test.ts @@ -51,10 +51,37 @@ describe("isPermanentChannelError", () => { expect(isPermanentChannelError(wrapper)).toBe(true); }); + test("ignores Discord resolveTextChannel wrapper without a forwarded code", () => { + // When `resolveTextChannel` cannot confidently identify a permanent + // failure (e.g. mixed pinned-bot transient + unrelated-bot permanent), + // it forwards a non-permanent or no `code`. The classifier must not + // promote those to permanent on the wrapper's message alone. See PR + // #211 discussion: "Avoid disabling Discord jobs on mixed fetch + // failures". + const noCode = new Error("Discord channel 123 is not text-based or inaccessible"); + expect(isPermanentChannelError(noCode)).toBe(false); + + const transientCode = Object.assign( + new Error("Discord channel 123 is not text-based or inaccessible"), + { code: 500 }, + ); + expect(isPermanentChannelError(transientCode)).toBe(false); + }); + test("matches Lark chat_not_found", () => { expect(isPermanentChannelError(new Error("chat_not_found: chat does not exist"))).toBe(true); }); + test("matches Lark deleted-chat message text alone", () => { + // larkApi() re-throws the raw `msg` field from the Lark API for some + // payloads — for stale/deleted group chats this surfaces as just + // "chat does not exist", without the `chat_not_found` token. The + // shorter "chat not exist" token does NOT match this string because + // of the "does" infix, so we list the long form explicitly. See PR + // #211 discussion: "Match Lark's deleted-chat message". + expect(isPermanentChannelError(new Error("chat does not exist"))).toBe(true); + }); + test("ignores transient / retryable failures", () => { expect(isPermanentChannelError(new Error("status 429"))).toBe(false); expect(isPermanentChannelError(new Error("ECONNRESET"))).toBe(false); diff --git a/packages/shared/delivery/permanent-error.ts b/packages/shared/delivery/permanent-error.ts index bce81fb6..b024cc5c 100644 --- a/packages/shared/delivery/permanent-error.ts +++ b/packages/shared/delivery/permanent-error.ts @@ -32,8 +32,8 @@ // - 50007 Cannot send messages to this user // // Lark (open-platform error codes vary; we match the common "permission -// denied" / "chat not exist" message text): -// - chat_not_found / chat does not exist +// denied" / "chat not exist" / "chat does not exist" message text): +// - chat_not_found / chat not exist / chat does not exist // - permission denied (generic; only matched when paired with chat context) const PERMANENT_MESSAGE_TOKENS = [ // Slack @@ -49,6 +49,12 @@ const PERMANENT_MESSAGE_TOKENS = [ // Lark "chat_not_found", "chat not exist", + // `larkApi` re-throws the raw `msg` text from the Lark API for some + // payloads (notably stale/deleted group chats), which surfaces as the + // human-readable "chat does not exist". The shorter "chat not exist" + // token does not contain this substring (due to the "does" infix), so + // we list it explicitly. + "chat does not exist", ]; // Discord.js surfaces the raw API code on `err.code` as a number. From 18209406f1541ff9e7347ab04a8fe702123760cb Mon Sep 17 00:00:00 2001 From: Kai Liu Date: Tue, 30 Jun 2026 03:05:43 +0800 Subject: [PATCH 5/7] fix(discord): require every fetch attempt to be permanent Address Codex review: in resolveTextChannel's unpinned-bot path, the allPermanent check only inspected captured numeric discordErrorCodes. A transient failure without a numeric code (ECONNRESET, network error) would be absent from that list, so an unrelated bot's permanent code (Missing Access / Unknown Channel) could promote the wrapper to permanent and auto-disable a healthy job on a single transient blip. Track totalFailedAttempts alongside the captured codes; only promote to permanent when permanentCodes.length === totalFailedAttempts. --- packages/ims/discord/client.ts | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/ims/discord/client.ts b/packages/ims/discord/client.ts index 66b83621..b99dc40e 100644 --- a/packages/ims/discord/client.ts +++ b/packages/ims/discord/client.ts @@ -133,7 +133,15 @@ async function resolveTextChannel(channelId: string, processorId?: string) { const discordErrorCodes: number[] = []; let pinnedBotErrorCode: number | undefined; let pinnedBotAttempted = false; + // Count every fetch attempt that ended in a thrown error — not just the + // subset that carried a numeric `err.code`. The wrapper's + // `isPermanentChannelError` classification must NOT promote to permanent + // when any attempt failed transiently (e.g. ECONNRESET, generic network + // error), because that bot might succeed on retry. See PR #211 discussion: + // "Require every Discord fetch attempt to be permanent". + let totalFailedAttempts = 0; const captureCode = (error: unknown, isPinnedBot: boolean) => { + totalFailedAttempts += 1; if (typeof error === "object" && error !== null) { const code = (error as { code?: unknown }).code; if (typeof code === "number") { @@ -195,15 +203,21 @@ async function resolveTextChannel(channelId: string, processorId?: string) { // same channel are unrelated noise. // 2. If no pinned bot was attempted (e.g. cron top-level send with // no processorId), only treat the failure as permanent when every - // captured code is permanent. Mixed transient+permanent means at - // least one bot might succeed on retry, so do NOT promote. + // failed attempt carried a permanent code. A transient failure + // (ECONNRESET, network blip) without a numeric `code` would not + // appear in `discordErrorCodes`, so counting only those would let + // an unrelated bot's permanent code promote the wrapper even though + // a retry might succeed. We therefore also require that the number + // of captured permanent codes equals the number of failed attempts. let forwardedCode: number | undefined; if (pinnedBotAttempted) { forwardedCode = pinnedBotErrorCode; } else { - const allPermanent = discordErrorCodes.every((c) => PERMANENT_PRIORITY.has(c)); - if (allPermanent) { - forwardedCode = discordErrorCodes.find((c) => PERMANENT_PRIORITY.has(c)); + const permanentCodes = discordErrorCodes.filter((c) => PERMANENT_PRIORITY.has(c)); + const allAttemptsPermanent = + permanentCodes.length === totalFailedAttempts && totalFailedAttempts > 0; + if (allAttemptsPermanent) { + forwardedCode = permanentCodes[0]; } else { forwardedCode = discordErrorCodes[0]; } From 1fdf52b2fca18d39d49b7cb84410487a56db0fed Mon Sep 17 00:00:00 2001 From: Kai Liu Date: Tue, 7 Jul 2026 03:05:08 +0800 Subject: [PATCH 6/7] fix(cron): treat Lark's undefined-send as a permanent delivery failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a Lark cron job's channel/workspace credentials are removed from config, sendLarkChannelMessage returns undefined instead of throwing (packages/ims/lark/client.ts:383-384). Before this fix, runCronJob treated the missing message id as a successful delivery, called markCronJobCompleted, and the job stayed enabled and silently undelivered on every subsequent tick — the auto-disable helper introduced earlier in this PR was never reached. sendResultToChannel now throws an error whose message contains chat_not_found when the Lark sender returns undefined, so the existing isPermanentChannelError check in the runCronJob catch branch fires and disableCronJobForPermanentChannelError disables the job the same way it does for explicit chat_not_found responses. Added a unit test asserting the exact wrapper message classifies as permanent, so future edits to the wrapper text cannot silently regress the auto-disable path. Addresses PR #211 review comment 'Treat undefined sends as delivery failures'. --- packages/core/cron/scheduler.ts | 17 ++++++++++++++++- .../shared/delivery/permanent-error.test.ts | 17 +++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/core/cron/scheduler.ts b/packages/core/cron/scheduler.ts index 747ff9f5..e4a5729e 100644 --- a/packages/core/cron/scheduler.ts +++ b/packages/core/cron/scheduler.ts @@ -140,7 +140,22 @@ async function sendResultToChannel( if (job.platform === "discord") { return sendDiscordChannelMessage(job.channelId, text); } - return sendLarkChannelMessage(job.channelId, text); + const larkResult = await sendLarkChannelMessage(job.channelId, text); + if (larkResult === undefined) { + // `sendLarkChannelMessage` returns `undefined` (without throwing) only + // when `getLarkCredentialsForProcessor` cannot find credentials for the + // channel — i.e. the Lark workspace/channel config has been removed. + // Without this guard the cron scheduler treats the run as delivered, + // marks it completed, and the job stays enabled and silently + // undelivered forever (see PR #211 review: "Treat undefined sends as + // delivery failures"). Throwing a message whose text matches + // `isPermanentChannelError` lets the existing catch-branch auto- + // disable this job the same way an explicit `chat_not_found` would. + throw new Error( + `Lark send returned no message id for channel ${job.channelId} (chat_not_found or credentials missing)`, + ); + } + return larkResult; } /** diff --git a/packages/shared/delivery/permanent-error.test.ts b/packages/shared/delivery/permanent-error.test.ts index 117c4ec9..3b3a7459 100644 --- a/packages/shared/delivery/permanent-error.test.ts +++ b/packages/shared/delivery/permanent-error.test.ts @@ -103,4 +103,21 @@ describe("isPermanentChannelError", () => { // must remain retryable from this helper's perspective. expect(isPermanentChannelError(new Error("message_not_found"))).toBe(false); }); + + test("matches the synthetic 'Lark send returned no message id' wrapper", () => { + // `packages/core/cron/scheduler.ts::sendResultToChannel` synthesizes + // this error when `sendLarkChannelMessage` returns `undefined` + // (missing Lark credentials for the channel — a permanent config + // problem the daemon cannot self-heal). The wrapper text embeds + // `chat_not_found` so this classifier catches it via the shared + // token list. See PR #211 review: "Treat undefined sends as delivery + // failures". + expect( + isPermanentChannelError( + new Error( + "Lark send returned no message id for channel oc_xxxx (chat_not_found or credentials missing)", + ), + ), + ).toBe(true); + }); }); From a831e9d702495980609eedb137db2fbb7419b411 Mon Sep 17 00:00:00 2001 From: Kai Liu Date: Wed, 8 Jul 2026 03:04:50 +0800 Subject: [PATCH 7/7] fix(discord): suppress permanent code on wrapper for mixed Discord failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When resolveTextChannel encounters a mixed set of unpinned-bot failures (one permanent DiscordAPIError code, plus a non-permanent numeric code from an unrelated bot), the previous fallback still forwarded discordErrorCodes[0] as the wrapper's `code`. If the first captured code happened to be permanent (10003 / 50001 / 50013 / 50007), isPermanentChannelError promoted the wrapper to permanent and the cron job was auto-disabled — even though at least one retryable failure was in the mix and a retry against the other bot might have succeeded. The unpinned mixed branch now forwards only the first non-permanent numeric code (still a useful diagnostic signal on the wrapper) and leaves `code` undefined when every captured code is permanent but totalFailedAttempts exceeds permanentCodes.length (i.e. an untyped transient error is also in play). The full discordErrorCodes array is preserved on the wrapper for diagnostics. Adds a regression test in permanent-error.test.ts asserting that a wrapper carrying only `discordErrorCodes` (no top-level `code`) is not classified as permanent, even when the array contains permanent codes. Addresses PR #211 review 'Avoid forwarding a permanent code after a mixed Discord failure' (chatgpt-codex-connector, 2026-07-06). --- packages/ims/discord/client.ts | 20 ++++++++++++++++++- .../shared/delivery/permanent-error.test.ts | 16 +++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/ims/discord/client.ts b/packages/ims/discord/client.ts index b99dc40e..29e54e73 100644 --- a/packages/ims/discord/client.ts +++ b/packages/ims/discord/client.ts @@ -209,6 +209,19 @@ async function resolveTextChannel(channelId: string, processorId?: string) { // an unrelated bot's permanent code promote the wrapper even though // a retry might succeed. We therefore also require that the number // of captured permanent codes equals the number of failed attempts. + // + // When the attempts are mixed (some permanent, some not), we must + // NOT forward any permanent code as the wrapper's `code` field — + // otherwise `isPermanentChannelError` (which classifies purely on + // the numeric `code`) would auto-disable the cron even though at + // least one retryable failure was in the mix. We fall back to the + // first *non-permanent* code (still useful diagnostic signal on + // the wrapper without triggering permanent classification); if + // every captured code is permanent but `totalFailedAttempts` + // exceeds that count (i.e. an untyped transient error is also in + // play), we suppress the code entirely. See PR #211 discussion: + // "Avoid forwarding a permanent code after a mixed Discord + // failure". let forwardedCode: number | undefined; if (pinnedBotAttempted) { forwardedCode = pinnedBotErrorCode; @@ -219,7 +232,12 @@ async function resolveTextChannel(channelId: string, processorId?: string) { if (allAttemptsPermanent) { forwardedCode = permanentCodes[0]; } else { - forwardedCode = discordErrorCodes[0]; + // Mixed attempts: never expose a permanent code on the wrapper. + // Prefer the first non-permanent numeric code for diagnostics; if + // no non-permanent code was captured, leave `code` undefined so + // the failure stays classified as retryable. + const nonPermanentCode = discordErrorCodes.find((c) => !PERMANENT_PRIORITY.has(c)); + forwardedCode = nonPermanentCode; } } diff --git a/packages/shared/delivery/permanent-error.test.ts b/packages/shared/delivery/permanent-error.test.ts index 3b3a7459..622d37e2 100644 --- a/packages/shared/delivery/permanent-error.test.ts +++ b/packages/shared/delivery/permanent-error.test.ts @@ -68,6 +68,22 @@ describe("isPermanentChannelError", () => { expect(isPermanentChannelError(transientCode)).toBe(false); }); + test("does not classify wrapper as permanent when only discordErrorCodes carries permanent codes", () => { + // Regression guard for PR #211 review "Avoid forwarding a permanent + // code after a mixed Discord failure": when `resolveTextChannel` + // observes a mixed set of failures (e.g. bot A returned 10003 Unknown + // Channel, bot B returned 50035 Invalid Form Body), the wrapper still + // exposes every captured code on `discordErrorCodes` for diagnostics, + // but `code` must be undefined or non-permanent. `isPermanentChannelError` + // only inspects `code`, so a wrapper carrying `discordErrorCodes: + // [10003, 50035]` with no `code` must NOT be classified as permanent. + const wrapper = Object.assign( + new Error("Discord channel 123 is not text-based or inaccessible"), + { discordErrorCodes: [10003, 50035] }, + ); + expect(isPermanentChannelError(wrapper)).toBe(false); + }); + test("matches Lark chat_not_found", () => { expect(isPermanentChannelError(new Error("chat_not_found: chat does not exist"))).toBe(true); });