mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-28 05:46:17 +00:00
fix(cron): preview no-deliver message targets
Fix cron delivery previews for no-delivery jobs that still provide explicit message-tool targets. - Reuse one cron delivery-plan explicit-target predicate across preview and isolated-agent runtime paths. - Treat numeric threadId 0 as an explicit delivery target. - Avoid fail-closed wording for unresolved message-tool-only targets. Thanks @Alix-007 for the fix. Co-authored-by: Alix-007 <267018309+Alix-007@users.noreply.github.com>
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { resolveCronDeliveryPlan } from "./delivery-plan.js";
|
||||
import { hasExplicitCronDeliveryTarget, resolveCronDeliveryPlan } from "./delivery-plan.js";
|
||||
import { makeCronJob } from "./delivery.test-helpers.js";
|
||||
|
||||
describe("resolveCronDeliveryPlan", () => {
|
||||
@@ -28,4 +28,18 @@ describe("resolveCronDeliveryPlan", () => {
|
||||
requested: false,
|
||||
});
|
||||
});
|
||||
|
||||
it("treats numeric zero thread id as an explicit target", () => {
|
||||
const plan = resolveCronDeliveryPlan(
|
||||
makeCronJob({
|
||||
delivery: {
|
||||
mode: "none",
|
||||
threadId: 0,
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
expect(plan.threadId).toBe(0);
|
||||
expect(hasExplicitCronDeliveryTarget(plan)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -19,6 +19,12 @@ export type CronDeliveryPlan = {
|
||||
requested: boolean;
|
||||
};
|
||||
|
||||
export function hasExplicitCronDeliveryTarget(plan: CronDeliveryPlan): boolean {
|
||||
return Boolean(
|
||||
(plan.channel && plan.channel !== "last") || plan.to || plan.threadId != null || plan.accountId,
|
||||
);
|
||||
}
|
||||
|
||||
function normalizeChannel(value: unknown): CronMessageChannel | undefined {
|
||||
const trimmed = normalizeOptionalLowercaseString(value);
|
||||
if (!trimmed) {
|
||||
|
||||
@@ -66,4 +66,78 @@ describe("resolveCronDeliveryPreview", () => {
|
||||
expect(preview).toEqual({ label: "not requested", detail: "not requested" });
|
||||
expect(mocks.resolveDeliveryTarget).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("previews explicit message-tool targets on no-delivery jobs", async () => {
|
||||
const job = makeCronJob({
|
||||
agentId: "avery",
|
||||
delivery: {
|
||||
mode: "none",
|
||||
channel: "topicchat",
|
||||
to: "room#42",
|
||||
threadId: 42,
|
||||
accountId: "ops",
|
||||
},
|
||||
sessionTarget: "isolated",
|
||||
});
|
||||
|
||||
const preview = await resolveCronDeliveryPreview({
|
||||
cfg: {} as never,
|
||||
job,
|
||||
});
|
||||
|
||||
expect(mocks.resolveDeliveryTarget).toHaveBeenCalledWith(
|
||||
{},
|
||||
"avery",
|
||||
{
|
||||
channel: "topicchat",
|
||||
to: "room#42",
|
||||
threadId: 42,
|
||||
accountId: "ops",
|
||||
sessionKey: undefined,
|
||||
},
|
||||
{ dryRun: true },
|
||||
);
|
||||
expect(preview).toEqual({
|
||||
label: "none -> telegram:direct-123",
|
||||
detail: "explicit",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not describe unresolved no-delivery message-tool targets as fail-closed", async () => {
|
||||
mocks.resolveDeliveryTarget.mockResolvedValueOnce({
|
||||
ok: false,
|
||||
mode: "implicit",
|
||||
error: new Error("no route"),
|
||||
});
|
||||
const job = makeCronJob({
|
||||
agentId: "avery",
|
||||
delivery: {
|
||||
mode: "none",
|
||||
threadId: 0,
|
||||
},
|
||||
sessionTarget: "isolated",
|
||||
});
|
||||
|
||||
const preview = await resolveCronDeliveryPreview({
|
||||
cfg: {} as never,
|
||||
job,
|
||||
});
|
||||
|
||||
expect(mocks.resolveDeliveryTarget).toHaveBeenCalledWith(
|
||||
{},
|
||||
"avery",
|
||||
{
|
||||
channel: "last",
|
||||
to: undefined,
|
||||
threadId: 0,
|
||||
accountId: undefined,
|
||||
sessionKey: undefined,
|
||||
},
|
||||
{ dryRun: true },
|
||||
);
|
||||
expect(preview).toEqual({
|
||||
label: "none -> last",
|
||||
detail: "message tool target unresolved: no route",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { resolveDefaultAgentId } from "../agents/agent-scope-config.js";
|
||||
import type { OpenClawConfig } from "../config/types.openclaw.js";
|
||||
import { resolveCronDeliveryPlan } from "./delivery-plan.js";
|
||||
import { hasExplicitCronDeliveryTarget, resolveCronDeliveryPlan } from "./delivery-plan.js";
|
||||
import { resolveDeliveryTarget } from "./isolated-agent/delivery-target.js";
|
||||
import { resolveCronDeliverySessionKey } from "./session-target.js";
|
||||
import type { CronDeliveryPreview, CronJob } from "./types.js";
|
||||
@@ -40,7 +40,7 @@ export async function resolveCronDeliveryPreview(params: {
|
||||
job: CronJob;
|
||||
}): Promise<CronDeliveryPreview> {
|
||||
const plan = resolveCronDeliveryPlan(params.job);
|
||||
if (plan.mode === "none") {
|
||||
if (plan.mode === "none" && !hasExplicitCronDeliveryTarget(plan)) {
|
||||
return { label: "not requested", detail: "not requested" };
|
||||
}
|
||||
if (plan.mode === "webhook") {
|
||||
@@ -67,12 +67,15 @@ export async function resolveCronDeliveryPreview(params: {
|
||||
if (!resolved.ok) {
|
||||
return {
|
||||
label: `${plan.mode} -> ${formatTarget(requestedChannel, plan.to ?? null)}`,
|
||||
detail: formatDeliveryDetail({
|
||||
requestedChannel,
|
||||
resolved: false,
|
||||
sessionKey: deliverySessionKey,
|
||||
error: resolved.error.message,
|
||||
}),
|
||||
detail:
|
||||
plan.mode === "none"
|
||||
? `message tool target unresolved: ${resolved.error.message}`
|
||||
: formatDeliveryDetail({
|
||||
requestedChannel,
|
||||
resolved: false,
|
||||
sessionKey: deliverySessionKey,
|
||||
error: resolved.error.message,
|
||||
}),
|
||||
};
|
||||
}
|
||||
return {
|
||||
|
||||
@@ -232,7 +232,8 @@ vi.mock("../../config/sessions/store.runtime.js", () => ({
|
||||
updateSessionStore: updateSessionStoreMock,
|
||||
}));
|
||||
|
||||
vi.mock("../delivery-plan.js", () => ({
|
||||
vi.mock("../delivery-plan.js", async () => ({
|
||||
...(await vi.importActual<typeof import("../delivery-plan.js")>("../delivery-plan.js")),
|
||||
resolveCronDeliveryPlan: resolveCronDeliveryPlanMock,
|
||||
}));
|
||||
|
||||
|
||||
@@ -27,7 +27,11 @@ import { isCommandLaneTaskTimeoutError } from "../../process/command-queue.js";
|
||||
import { CommandLane } from "../../process/lanes.js";
|
||||
import { createLazyImportLoader } from "../../shared/lazy-promise.js";
|
||||
import { normalizeOptionalString } from "../../shared/string-coerce.js";
|
||||
import { resolveCronDeliveryPlan, type CronDeliveryPlan } from "../delivery-plan.js";
|
||||
import {
|
||||
hasExplicitCronDeliveryTarget,
|
||||
resolveCronDeliveryPlan,
|
||||
type CronDeliveryPlan,
|
||||
} from "../delivery-plan.js";
|
||||
import {
|
||||
createCronRunDiagnosticsFromAgentResult,
|
||||
createCronRunDiagnosticsFromError,
|
||||
@@ -344,12 +348,6 @@ function canPromptForMessageTool(params: {
|
||||
);
|
||||
}
|
||||
|
||||
function hasExplicitCronDeliveryTarget(plan: CronDeliveryPlan): boolean {
|
||||
return Boolean(
|
||||
(plan.channel && plan.channel !== "last") || plan.to || plan.threadId || plan.accountId,
|
||||
);
|
||||
}
|
||||
|
||||
async function resolveCronDeliveryContext(params: {
|
||||
cfg: OpenClawConfig;
|
||||
job: CronJob;
|
||||
|
||||
Reference in New Issue
Block a user