mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 16:01:01 +00:00
fix: harden cron announce NO_REPLY suppression (#65016) (thanks @BKF-Gitty)
This commit is contained in:
@@ -1,4 +1,10 @@
|
||||
import { isSilentReplyText, SILENT_REPLY_TOKEN } from "../auto-reply/tokens.js";
|
||||
import {
|
||||
isSilentReplyText,
|
||||
SILENT_REPLY_TOKEN,
|
||||
startsWithSilentToken,
|
||||
stripLeadingSilentToken,
|
||||
stripSilentToken,
|
||||
} from "../auto-reply/tokens.js";
|
||||
import { defaultRuntime } from "../runtime.js";
|
||||
import { isCronSessionKey } from "../sessions/session-key-utils.js";
|
||||
import { normalizeOptionalString } from "../shared/string-coerce.js";
|
||||
@@ -127,6 +133,27 @@ function isWakeContinuationRun(runId: string): boolean {
|
||||
return stripWakeRunSuffixes(trimmed) !== trimmed;
|
||||
}
|
||||
|
||||
function stripAndClassifyReply(text: string): string | null {
|
||||
let result = text;
|
||||
let didStrip = false;
|
||||
const hasLeadingSilentToken = startsWithSilentToken(result, SILENT_REPLY_TOKEN);
|
||||
if (hasLeadingSilentToken) {
|
||||
result = stripLeadingSilentToken(result, SILENT_REPLY_TOKEN);
|
||||
didStrip = true;
|
||||
}
|
||||
if (hasLeadingSilentToken || result.toLowerCase().includes(SILENT_REPLY_TOKEN.toLowerCase())) {
|
||||
result = stripSilentToken(result, SILENT_REPLY_TOKEN);
|
||||
didStrip = true;
|
||||
}
|
||||
if (
|
||||
didStrip &&
|
||||
(!result.trim() || isSilentReplyText(result, SILENT_REPLY_TOKEN) || isAnnounceSkip(result))
|
||||
) {
|
||||
return null;
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
async function wakeSubagentRunAfterDescendants(params: {
|
||||
runId: string;
|
||||
childSessionKey: string;
|
||||
@@ -385,10 +412,29 @@ export async function runSubagentAnnounceFlow(params: {
|
||||
|
||||
if (isAnnounceSkip(reply) || isSilentReplyText(reply, SILENT_REPLY_TOKEN)) {
|
||||
if (fallbackReply && !fallbackIsSilent) {
|
||||
reply = fallbackReply;
|
||||
const cleaned = stripAndClassifyReply(fallbackReply);
|
||||
if (cleaned === null) {
|
||||
return true;
|
||||
}
|
||||
reply = cleaned;
|
||||
} else {
|
||||
return true;
|
||||
}
|
||||
} else if (reply) {
|
||||
const cleaned = stripAndClassifyReply(reply);
|
||||
if (cleaned === null) {
|
||||
if (fallbackReply && !fallbackIsSilent) {
|
||||
const cleanedFallback = stripAndClassifyReply(fallbackReply);
|
||||
if (cleanedFallback === null) {
|
||||
return true;
|
||||
}
|
||||
reply = cleanedFallback;
|
||||
} else {
|
||||
return true;
|
||||
}
|
||||
} else {
|
||||
reply = cleaned;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -69,7 +69,7 @@ export function normalizeReplyPayload(
|
||||
if (hasLeadingSilentToken) {
|
||||
text = stripLeadingSilentToken(text, silentToken);
|
||||
}
|
||||
if (hasLeadingSilentToken || text.includes(silentToken)) {
|
||||
if (hasLeadingSilentToken || text.toLowerCase().includes(silentToken.toLowerCase())) {
|
||||
text = stripSilentToken(text, silentToken);
|
||||
if (!hasContent(text)) {
|
||||
opts.onSkip?.("silent");
|
||||
|
||||
@@ -24,7 +24,7 @@ function getSilentTrailingRegex(token: string): RegExp {
|
||||
return cached;
|
||||
}
|
||||
const escaped = escapeRegExp(token);
|
||||
const regex = new RegExp(`(?:^|\\s+|\\*+)${escaped}\\s*$`);
|
||||
const regex = new RegExp(`(?:^|\\s+|\\*+)${escaped}\\s*$`, "i");
|
||||
silentTrailingRegexByToken.set(token, regex);
|
||||
return regex;
|
||||
}
|
||||
|
||||
@@ -122,7 +122,7 @@ function makeBaseParams(overrides: {
|
||||
runSessionId?: string;
|
||||
sessionTarget?: string;
|
||||
deliveryBestEffort?: boolean;
|
||||
}) {
|
||||
}): Parameters<typeof dispatchCronDelivery>[0] {
|
||||
const resolvedDelivery = makeResolvedDelivery();
|
||||
return {
|
||||
cfg: {} as never,
|
||||
@@ -327,6 +327,32 @@ describe("dispatchCronDelivery — double-announce guard", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("skips awareness text when direct delivery strips a silent caption", async () => {
|
||||
vi.mocked(countActiveDescendantRuns).mockReturnValue(0);
|
||||
vi.mocked(isLikelyInterimCronMessage).mockReturnValue(false);
|
||||
|
||||
const params = makeBaseParams({ synthesizedText: undefined });
|
||||
params.deliveryPayloadHasStructuredContent = true;
|
||||
params.deliveryPayloads = [
|
||||
{ mediaUrl: "https://example.com/image.png", text: "All done\n\nNO_REPLY" },
|
||||
];
|
||||
params.outputText = "All done\n\nNO_REPLY";
|
||||
params.summary = "All done\n\nNO_REPLY";
|
||||
|
||||
const state = await dispatchCronDelivery(params);
|
||||
|
||||
expect(state.result).toBeUndefined();
|
||||
expect(state.delivered).toBe(true);
|
||||
expect(state.deliveryAttempted).toBe(true);
|
||||
expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1);
|
||||
expect(deliverOutboundPayloads).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
payloads: [{ mediaUrl: "https://example.com/image.png", text: undefined }],
|
||||
}),
|
||||
);
|
||||
expect(enqueueSystemEvent).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("keeps the cron run successful when awareness queueing throws after delivery", async () => {
|
||||
vi.mocked(countActiveDescendantRuns).mockReturnValue(0);
|
||||
vi.mocked(isLikelyInterimCronMessage).mockReturnValue(false);
|
||||
@@ -986,4 +1012,73 @@ describe("dispatchCronDelivery — double-announce guard", () => {
|
||||
expect.objectContaining({ status: "ok", delivered: false, deliveryAttempted: true }),
|
||||
);
|
||||
});
|
||||
|
||||
it("delivers substantive text that mentions NO_REPLY in non-trailing content (text delivery)", async () => {
|
||||
vi.mocked(countActiveDescendantRuns).mockReturnValue(0);
|
||||
vi.mocked(isLikelyInterimCronMessage).mockReturnValue(false);
|
||||
|
||||
const params = makeBaseParams({
|
||||
synthesizedText:
|
||||
"The NO_REPLY sentinel tells the agent to skip delivery when nothing changes.",
|
||||
});
|
||||
const state = await dispatchCronDelivery(params);
|
||||
|
||||
expect(state.deliveryAttempted).toBe(true);
|
||||
expect(state.delivered).toBe(true);
|
||||
expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("delivers substantive text that mentions NO_REPLY in non-trailing content (direct delivery)", async () => {
|
||||
vi.mocked(countActiveDescendantRuns).mockReturnValue(0);
|
||||
vi.mocked(isLikelyInterimCronMessage).mockReturnValue(false);
|
||||
|
||||
const params = makeBaseParams({
|
||||
synthesizedText:
|
||||
"Reminder: reply NO_REPLY when there is nothing to announce, otherwise send a summary.",
|
||||
});
|
||||
(params as Record<string, unknown>).deliveryPayloadHasStructuredContent = true;
|
||||
const state = await dispatchCronDelivery(params);
|
||||
|
||||
expect(state.deliveryAttempted).toBe(true);
|
||||
expect(state.delivered).toBe(true);
|
||||
expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("delivers non-trailing NO_REPLY mention with trailing whitespace", async () => {
|
||||
vi.mocked(countActiveDescendantRuns).mockReturnValue(0);
|
||||
vi.mocked(isLikelyInterimCronMessage).mockReturnValue(false);
|
||||
|
||||
const params = makeBaseParams({
|
||||
synthesizedText: "Use NO_REPLY when nothing actionable changed.\n",
|
||||
});
|
||||
const state = await dispatchCronDelivery(params);
|
||||
|
||||
expect(state.deliveryAttempted).toBe(true);
|
||||
expect(state.delivered).toBe(true);
|
||||
expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("drops only the payload with trailing NO_REPLY in a multi-payload direct delivery", async () => {
|
||||
vi.mocked(countActiveDescendantRuns).mockReturnValue(0);
|
||||
vi.mocked(isLikelyInterimCronMessage).mockReturnValue(false);
|
||||
|
||||
const params = makeBaseParams({ synthesizedText: undefined });
|
||||
params.deliveryPayloads = [
|
||||
{ text: "Working on it..." },
|
||||
{ text: "Final weather summary\n\nNO_REPLY" },
|
||||
];
|
||||
params.summary = "Working on it...";
|
||||
params.outputText = "Working on it...";
|
||||
|
||||
const state = await dispatchCronDelivery(params);
|
||||
|
||||
expect(state.deliveryAttempted).toBe(true);
|
||||
expect(state.delivered).toBe(true);
|
||||
expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1);
|
||||
expect(deliverOutboundPayloads).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
payloads: [{ text: "Working on it..." }],
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,11 @@
|
||||
import type { ReplyPayload } from "../../auto-reply/reply-payload.js";
|
||||
import { isSilentReplyText, stripSilentToken, SILENT_REPLY_TOKEN } from "../../auto-reply/tokens.js";
|
||||
import {
|
||||
isSilentReplyText,
|
||||
SILENT_REPLY_TOKEN,
|
||||
startsWithSilentToken,
|
||||
stripLeadingSilentToken,
|
||||
stripSilentToken,
|
||||
} from "../../auto-reply/tokens.js";
|
||||
import type { CliDeps } from "../../cli/outbound-send-deps.js";
|
||||
import {
|
||||
resolveAgentMainSessionKey,
|
||||
@@ -10,6 +16,7 @@ import { sleepWithAbort } from "../../infra/backoff.js";
|
||||
import { formatErrorMessage } from "../../infra/errors.js";
|
||||
import type { OutboundDeliveryResult } from "../../infra/outbound/deliver.js";
|
||||
import { normalizeTargetForProvider } from "../../infra/outbound/target-normalization.js";
|
||||
import { hasReplyPayloadContent } from "../../interactive/payload.js";
|
||||
import {
|
||||
normalizeLowercaseStringOrEmpty,
|
||||
normalizeOptionalLowercaseString,
|
||||
@@ -18,7 +25,7 @@ import {
|
||||
import { hasScheduledNextRunAtMs } from "../service/jobs.js";
|
||||
import type { CronJob, CronRunTelemetry } from "../types.js";
|
||||
import type { DeliveryTargetResolution } from "./delivery-target.js";
|
||||
import { pickSummaryFromOutput } from "./helpers.js";
|
||||
import { pickLastNonEmptyTextFromPayloads, pickSummaryFromOutput } from "./helpers.js";
|
||||
import type { RunCronAgentTurnResult } from "./run.types.js";
|
||||
import { expectsSubagentFollowup, isLikelyInterimCronMessage } from "./subagent-followup-hints.js";
|
||||
|
||||
@@ -27,6 +34,39 @@ function normalizeDeliveryTarget(channel: string, to: string): string {
|
||||
return normalizeTargetForProvider(channel, toTrimmed) ?? toTrimmed;
|
||||
}
|
||||
|
||||
type NormalizedSilentReplyText = {
|
||||
text: string | undefined;
|
||||
strippedTrailingSilentToken: boolean;
|
||||
};
|
||||
|
||||
function normalizeSilentReplyText(text: string | undefined): NormalizedSilentReplyText {
|
||||
if (!text) {
|
||||
return { text, strippedTrailingSilentToken: false };
|
||||
}
|
||||
if (isSilentReplyText(text, SILENT_REPLY_TOKEN)) {
|
||||
return { text: undefined, strippedTrailingSilentToken: false };
|
||||
}
|
||||
|
||||
let next = text;
|
||||
const hasLeadingSilentToken = startsWithSilentToken(next, SILENT_REPLY_TOKEN);
|
||||
if (hasLeadingSilentToken) {
|
||||
next = stripLeadingSilentToken(next, SILENT_REPLY_TOKEN);
|
||||
}
|
||||
|
||||
let strippedTrailingSilentToken = false;
|
||||
if (hasLeadingSilentToken || next.toLowerCase().includes(SILENT_REPLY_TOKEN.toLowerCase())) {
|
||||
const trimmedBefore = next.trim();
|
||||
const stripped = stripSilentToken(next, SILENT_REPLY_TOKEN);
|
||||
strippedTrailingSilentToken = stripped !== trimmedBefore;
|
||||
next = stripped;
|
||||
}
|
||||
|
||||
if (!next.trim() || isSilentReplyText(next, SILENT_REPLY_TOKEN)) {
|
||||
return { text: undefined, strippedTrailingSilentToken };
|
||||
}
|
||||
return { text: next, strippedTrailingSilentToken };
|
||||
}
|
||||
|
||||
export function matchesMessagingToolDeliveryTarget(
|
||||
target: { provider?: string; to?: string; accountId?: string },
|
||||
delivery: { channel?: string; to?: string; accountId?: string },
|
||||
@@ -293,9 +333,12 @@ async function queueCronAwarenessSystemEvent(params: {
|
||||
deliveryIdempotencyKey: string;
|
||||
outputText?: string;
|
||||
synthesizedText?: string;
|
||||
deliveryPayloads?: ReplyPayload[];
|
||||
}): Promise<void> {
|
||||
const text =
|
||||
normalizeOptionalString(params.outputText) ?? normalizeOptionalString(params.synthesizedText);
|
||||
const text = params.deliveryPayloads
|
||||
? pickLastNonEmptyTextFromPayloads(params.deliveryPayloads)
|
||||
: (normalizeOptionalString(params.outputText) ??
|
||||
normalizeOptionalString(params.synthesizedText));
|
||||
if (!text) {
|
||||
return;
|
||||
}
|
||||
@@ -462,21 +505,18 @@ export async function dispatchCronDelivery(
|
||||
: synthesizedText
|
||||
? [{ text: synthesizedText }]
|
||||
: [];
|
||||
// Suppress NO_REPLY sentinel so it never leaks to external channels.
|
||||
// Also suppress payloads where the agent appended a trailing NO_REPLY
|
||||
// after other text (e.g. "summary...\n\nNO_REPLY") — the token signals
|
||||
// "do not deliver" regardless of preceding content.
|
||||
const payloadsForDelivery = rawPayloads.filter((p) => {
|
||||
const text = p.text ?? "";
|
||||
if (isSilentReplyText(text, SILENT_REPLY_TOKEN)) {
|
||||
return false;
|
||||
}
|
||||
// Case-insensitive trailing check: uppercase before stripping since
|
||||
// stripSilentToken's regex is case-sensitive.
|
||||
const upper = text.toUpperCase();
|
||||
const stripped = stripSilentToken(upper, SILENT_REPLY_TOKEN);
|
||||
return stripped === upper.trim();
|
||||
});
|
||||
const payloadsForDelivery = rawPayloads
|
||||
.map((p) => {
|
||||
if (!p.text) {
|
||||
return p;
|
||||
}
|
||||
const normalized = normalizeSilentReplyText(p.text);
|
||||
return {
|
||||
...p,
|
||||
text: normalized.strippedTrailingSilentToken ? undefined : normalized.text,
|
||||
};
|
||||
})
|
||||
.filter((p) => hasReplyPayloadContent(p, { trimText: true }));
|
||||
if (payloadsForDelivery.length === 0) {
|
||||
return await finishSilentReplyDelivery();
|
||||
}
|
||||
@@ -583,6 +623,7 @@ export async function dispatchCronDelivery(
|
||||
deliveryIdempotencyKey,
|
||||
outputText,
|
||||
synthesizedText,
|
||||
deliveryPayloads: payloadsForDelivery,
|
||||
});
|
||||
}
|
||||
if (delivered) {
|
||||
@@ -700,17 +741,15 @@ export async function dispatchCronDelivery(
|
||||
...params.telemetry,
|
||||
});
|
||||
}
|
||||
// Suppress delivery when synthesizedText is (or ends with) NO_REPLY.
|
||||
// isSilentReplyText handles case-insensitive exact matches (e.g. "No_Reply");
|
||||
// stripSilentToken catches trailing tokens after other text.
|
||||
if (isSilentReplyText(synthesizedText, SILENT_REPLY_TOKEN)) {
|
||||
return await finishSilentReplyDelivery();
|
||||
}
|
||||
const upperSynthesized = synthesizedText.toUpperCase();
|
||||
const strippedSynthesized = stripSilentToken(upperSynthesized, SILENT_REPLY_TOKEN);
|
||||
if (strippedSynthesized !== upperSynthesized.trim()) {
|
||||
const normalizedSynthesizedText = normalizeSilentReplyText(synthesizedText);
|
||||
if (
|
||||
normalizedSynthesizedText.text === undefined ||
|
||||
normalizedSynthesizedText.strippedTrailingSilentToken
|
||||
) {
|
||||
return await finishSilentReplyDelivery();
|
||||
}
|
||||
synthesizedText = normalizedSynthesizedText.text;
|
||||
outputText = synthesizedText;
|
||||
if (params.isAborted()) {
|
||||
return params.withRunSession({
|
||||
status: "error",
|
||||
|
||||
Reference in New Issue
Block a user