Skip to content

Commit 75e0053

Browse files
yaoyi1222yaoyi1222
authored
fix(auto-reply): warn on substantive private message-tool finals
Warn operators when message_tool_only produces unusually substantive private final text without a delivered source reply. Keeps short/NO_REPLY silence quiet, avoids logging response bodies, and distinguishes unrelated side effects from source-reply delivery.
1 parent 81b9da0 commit 75e0053

7 files changed

Lines changed: 427 additions & 8 deletions

File tree

‎docs/tools/slash-commands.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,11 +212,13 @@ plugins.
212212
- `/fast on|off` persists a session override; use the Sessions UI `inherit` option to clear it.
213213
- `/fast` is provider-specific: OpenAI/Codex map it to `service_tier=priority`; direct Anthropic requests map it to `service_tier=auto` or `standard_only`.
214214
- `/reasoning`, `/verbose`, and `/trace` are risky in group settings — they may reveal internal reasoning or plugin diagnostics. Keep them off in group chats.
215+
215216
</Accordion>
216217
<Accordion title="Model switching details">
217218
- `/model` persists the new model immediately to the session.
218219
- If the agent is idle, the next run uses it right away.
219220
- If a run is active, the switch is marked pending and applied at the next clean retry point.
221+
220222
</Accordion>
221223
</AccordionGroup>
222224

@@ -468,6 +470,7 @@ See [BTW side questions](/tools/btw) for the full behavior.
468470
- **Native Slack commands:** `agent:<agentId>:slack:slash:<userId>` (prefix configurable via `channels.slack.slashCommand.sessionPrefix`)
469471
- **Native Telegram commands:** `telegram:slash:<userId>` (targets the chat session via `CommandTargetSessionKey`)
470472
- **`/stop`** targets the active chat session to abort the current run.
473+
471474
</Accordion>
472475
<Accordion title="Slack specifics">
473476
`channels.slack.slashCommand` supports a single `/openclaw`-style command.
@@ -479,11 +482,13 @@ See [BTW side questions](/tools/btw) for the full behavior.
479482
- Command-only messages from allowlisted senders are handled immediately (bypass queue + model).
480483
- Inline shortcuts (`/help`, `/commands`, `/status`, `/whoami`) also work embedded in normal messages and are stripped before the model sees the remaining text.
481484
- Unauthorized command-only messages are silently ignored; inline `/...` tokens are treated as plain text.
485+
482486
</Accordion>
483487
<Accordion title="Argument notes">
484488
- Commands accept an optional `:` between the command and args (`/think: high`, `/send: on`).
485489
- `/new <model>` accepts a model alias, `provider/model`, or a provider name (fuzzy match); if no match, the text is treated as the message body.
486490
- `/allowlist add|remove` requires `commands.config: true` and honors channel `configWrites`.
491+
487492
</Accordion>
488493
</AccordionGroup>
489494

‎qa/scenarios/channels/message-tool-stranded-final-reply.md‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
# Message-tool-only private final reply warning
2+
3+
```yaml qa-scenario
4+
id: message-tool-stranded-final-reply
5+
title: Message-tool-only private final reply warning
6+
surface: channel
7+
coverage:
8+
primary:
9+
- channels.direct-visible-replies
10+
secondary:
11+
- channels.qa-channel
12+
- tools.message
13+
objective: Reproduce #85714 — under messages.visibleReplies=message_tool a long private final reply that never calls the message tool is kept private (no outbound), and the gateway emits the private-final WARN.
14+
gatewayConfigPatch:
15+
messages:
16+
visibleReplies: message_tool
17+
successCriteria:
18+
- The mock provider returns a long normal final answer and does not plan the message tool.
19+
- Under message_tool_only delivery the reply is kept private, so the direct conversation receives no outbound message.
20+
- The gateway logs the private-final WARN from source-reply/private-final.
21+
docsRefs:
22+
- docs/channels/qa-channel.md
23+
codeRefs:
24+
- src/auto-reply/reply/agent-runner.ts
25+
- src/auto-reply/reply/private-message-tool-final.ts
26+
- src/auto-reply/reply/dispatch-from-config.ts
27+
execution:
28+
kind: flow
29+
summary: Send a direct message_tool_only turn whose model reply omits the message tool, and verify a substantive private final warns without outbound delivery.
30+
config:
31+
conversationId: qa-stranded-dm
32+
promptSnippet: qa private final reply warning check
33+
prompt: "qa private final reply warning check. Reply to me directly in two complete sentences with `QA-STRANDED-85714` in the first sentence and a short explanation in the second sentence. Do NOT call any tool. Do NOT use the message tool."
34+
expectedMarker: QA-STRANDED-85714
35+
privateFinalLogNeedle: "source-reply/private-final"
36+
```
37+
38+
```yaml qa-flow
39+
steps:
40+
- name: warns for substantive private final text when the model omits the message tool
41+
actions:
42+
- call: waitForGatewayHealthy
43+
args:
44+
- ref: env
45+
- 60000
46+
- call: waitForQaChannelReady
47+
args:
48+
- ref: env
49+
- 60000
50+
- call: reset
51+
- set: logCursor
52+
value:
53+
expr: markGatewayLogCursor()
54+
- set: requestCountBefore
55+
value:
56+
expr: "env.mock ? (await fetchJson(`${env.mock.baseUrl}/debug/requests`)).length : 0"
57+
- call: state.addInboundMessage
58+
args:
59+
- conversation:
60+
id:
61+
expr: config.conversationId
62+
kind: direct
63+
senderId: alice
64+
senderName: Alice
65+
text:
66+
expr: config.prompt
67+
- call: waitForNoOutbound
68+
args:
69+
- ref: state
70+
- expr: liveTurnTimeoutMs(env, 30000)
71+
- set: scenarioRequests
72+
value:
73+
expr: "env.mock ? (await fetchJson(`${env.mock.baseUrl}/debug/requests`)).slice(requestCountBefore).filter((request) => String(request.allInputText ?? '').includes(config.promptSnippet)) : []"
74+
- assert:
75+
expr: "!env.mock || scenarioRequests.length > 0"
76+
message: expected mock request evidence that the turn actually ran
77+
- assert:
78+
expr: "!env.mock || scenarioRequests.every((request) => request.plannedToolName !== 'message')"
79+
message:
80+
expr: "`model should not have planned the message tool, saw ${JSON.stringify(scenarioRequests.map((request) => request.plannedToolName ?? null))}`"
81+
- set: privateFinalLog
82+
value:
83+
expr: "String(readGatewayLogs() ?? '').slice(logCursor)"
84+
- set: privateFinalLine
85+
value:
86+
expr: "(privateFinalLog.split('\\n').find((line) => line.includes(config.privateFinalLogNeedle)) ?? '').trim()"
87+
- assert:
88+
expr: "privateFinalLog.includes(config.privateFinalLogNeedle)"
89+
message:
90+
expr: "`expected the gateway to log ${config.privateFinalLogNeedle} after a substantive private message_tool_only reply, but it was absent`"
91+
detailsExpr: "`no-outbound private final; WARN logged=${privateFinalLog.includes(config.privateFinalLogNeedle)}; mock requests=${scenarioRequests.length}; gateway log: ${privateFinalLine}`"
92+
```

‎src/auto-reply/reply/agent-runner.misc.runreplyagent.test.ts‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,14 @@ vi.mock("../../agents/subagent-registry.js", () => ({
190190
markSubagentRunTerminated: () => 0,
191191
}));
192192

193+
// #85714: keep the real private-final decision but spy the WARN emitter so we
194+
// can assert it fires only through the substantive text suppression branch.
195+
const warnPrivateFinalSpy = vi.hoisted(() => vi.fn());
196+
vi.mock("./private-message-tool-final.js", async (importOriginal) => {
197+
const actual = await importOriginal<typeof import("./private-message-tool-final.js")>();
198+
return { ...actual, warnPrivateMessageToolFinal: warnPrivateFinalSpy };
199+
});
200+
193201
import { runReplyAgent } from "./agent-runner.js";
194202

195203
type RunWithModelFallbackParams = {
@@ -244,6 +252,7 @@ beforeEach(() => {
244252
embeddedRunTesting.resetActiveEmbeddedRuns();
245253
replyRunRegistryTesting.resetReplyRunRegistry();
246254
runEmbeddedAgentMock.mockClear();
255+
warnPrivateFinalSpy.mockClear();
247256
runCliAgentMock.mockClear();
248257
runWithModelFallbackMock.mockClear();
249258
runtimeErrorMock.mockClear();
@@ -2984,3 +2993,129 @@ describe("runReplyAgent mid-turn rate-limit fallback", () => {
29842993
expect(payload?.text).toBeUndefined();
29852994
});
29862995
});
2996+
2997+
describe("runReplyAgent private message_tool_only final warning (#85714)", () => {
2998+
async function runPrivateFinalCase(params: {
2999+
messagingToolSentTargets?: unknown[];
3000+
finalAssistantText?: string;
3001+
payloadText?: string;
3002+
successfulCronAdds?: number;
3003+
}) {
3004+
const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-stranded-"));
3005+
const storePath = path.join(tmp, "sessions.json");
3006+
const sessionKey = "stranded";
3007+
const sessionEntry = { sessionId: "session", updatedAt: Date.now(), totalTokens: 1_000 };
3008+
await fs.writeFile(storePath, JSON.stringify({ [sessionKey]: sessionEntry }, null, 2), "utf-8");
3009+
3010+
const finalAssistantText =
3011+
params.finalAssistantText ??
3012+
"Here is the answer the user asked for. It includes enough detail to read like a user-facing response rather than a short private note. This should have been sent with the message tool if the channel expected a visible reply.";
3013+
runEmbeddedAgentMock.mockResolvedValue({
3014+
// payloadText can differ from the assistant text to simulate metadata-only
3015+
// payloads (verbose notices, usage line) that must NOT trigger the warn —
3016+
// detection keys off the assistant final text, not the payload bundle.
3017+
payloads: [{ text: params.payloadText ?? finalAssistantText }],
3018+
meta: { agentMeta: {}, finalAssistantVisibleText: finalAssistantText },
3019+
...(params.messagingToolSentTargets
3020+
? { messagingToolSentTargets: params.messagingToolSentTargets }
3021+
: {}),
3022+
...(params.successfulCronAdds === undefined
3023+
? {}
3024+
: { successfulCronAdds: params.successfulCronAdds }),
3025+
});
3026+
3027+
const sessionCtx = {
3028+
Provider: "whatsapp",
3029+
OriginatingTo: "+15550001111",
3030+
AccountId: "primary",
3031+
MessageSid: "msg",
3032+
ChatType: "direct",
3033+
} as unknown as TemplateContext;
3034+
const followupRun = {
3035+
prompt: "hello",
3036+
summaryLine: "hello",
3037+
enqueuedAt: Date.now(),
3038+
run: {
3039+
agentId: "main",
3040+
agentDir: "/tmp/agent",
3041+
sessionId: "session",
3042+
sessionKey,
3043+
messageProvider: "whatsapp",
3044+
sessionFile: "/tmp/session.jsonl",
3045+
workspaceDir: tmp,
3046+
// Direct chat + visibleReplies=message_tool resolves to message_tool_only,
3047+
// so the final text is kept private (no automatic delivery).
3048+
config: { messages: { visibleReplies: "message_tool" } },
3049+
skillsSnapshot: {},
3050+
provider: "anthropic",
3051+
model: "claude",
3052+
thinkLevel: "low",
3053+
reasoningLevel: "on",
3054+
verboseLevel: "off",
3055+
elevatedLevel: "off",
3056+
bashElevated: { enabled: false, allowed: false, defaultLevel: "off" },
3057+
timeoutMs: 1_000,
3058+
blockReplyBreak: "message_end",
3059+
},
3060+
} as unknown as FollowupRun;
3061+
3062+
await runReplyAgent({
3063+
commandBody: "hello",
3064+
followupRun,
3065+
queueKey: sessionKey,
3066+
resolvedQueue: { mode: "interrupt" } as unknown as QueueSettings,
3067+
shouldSteer: false,
3068+
shouldFollowup: false,
3069+
isActive: false,
3070+
isStreaming: false,
3071+
typing: createMockTypingController(),
3072+
sessionCtx,
3073+
sessionEntry,
3074+
sessionStore: { [sessionKey]: sessionEntry },
3075+
sessionKey,
3076+
storePath,
3077+
defaultModel: "anthropic/claude-opus-4-6",
3078+
agentCfgContextTokens: 200_000,
3079+
resolvedVerboseLevel: "off",
3080+
isNewSession: false,
3081+
blockStreamingEnabled: false,
3082+
resolvedBlockStreamingBreak: "message_end",
3083+
shouldInjectGroupIntro: false,
3084+
typingMode: "instant",
3085+
});
3086+
}
3087+
3088+
it("warns when a substantive private final reply never used the message tool", async () => {
3089+
await runPrivateFinalCase({});
3090+
expect(warnPrivateFinalSpy).toHaveBeenCalledTimes(1);
3091+
expect(warnPrivateFinalSpy.mock.calls[0]?.[0]).toMatchObject({ sessionKey: "stranded" });
3092+
});
3093+
3094+
it("does not warn for a short private final reply", async () => {
3095+
await runPrivateFinalCase({ finalAssistantText: "Nothing to send here." });
3096+
expect(warnPrivateFinalSpy).not.toHaveBeenCalled();
3097+
});
3098+
3099+
it("does not warn when the message tool delivered this turn", async () => {
3100+
await runPrivateFinalCase({
3101+
messagingToolSentTargets: [{ tool: "message", provider: "whatsapp", to: "+15550001111" }],
3102+
});
3103+
expect(warnPrivateFinalSpy).not.toHaveBeenCalled();
3104+
});
3105+
3106+
it("still warns when only an unrelated cron side effect succeeded", async () => {
3107+
await runPrivateFinalCase({ successfulCronAdds: 1 });
3108+
expect(warnPrivateFinalSpy).toHaveBeenCalledTimes(1);
3109+
});
3110+
3111+
it("does not warn on an intentional NO_REPLY turn even when metadata payloads remain", async () => {
3112+
// Assistant went silent (NO_REPLY), but a verbose/usage metadata payload
3113+
// survives in finalPayloads. The warn must key off the assistant text, not
3114+
// the payload bundle, so no private-final warning should fire.
3115+
await runPrivateFinalCase({
3116+
finalAssistantText: "no_reply",
3117+
payloadText: "Auto-compaction complete (count 1).",
3118+
});
3119+
expect(warnPrivateFinalSpy).not.toHaveBeenCalled();
3120+
});
3121+
});

‎src/auto-reply/reply/agent-runner.ts‎

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,10 @@ import { resolveOriginMessageProvider, resolveOriginMessageTo } from "./origin-r
9494
import { sanitizePendingFinalDeliveryText } from "./pending-final-delivery.js";
9595
import { drainPendingToolTasks } from "./pending-tool-task-drain.js";
9696
import { readPostCompactionContext } from "./post-compaction-context.js";
97+
import {
98+
shouldWarnAboutPrivateMessageToolFinal,
99+
warnPrivateMessageToolFinal,
100+
} from "./private-message-tool-final.js";
97101
import { resolveActiveRunQueueAction } from "./queue-policy.js";
98102
import {
99103
enqueueFollowupRun,
@@ -230,15 +234,27 @@ function hasSuccessfulSideEffectDelivery(params: {
230234
messagingToolSentTargets?: unknown[];
231235
successfulCronAdds?: number;
232236
didSendDeterministicApprovalPrompt?: boolean;
237+
}): boolean {
238+
return (
239+
hasSuccessfulSourceReplyDelivery(params) ||
240+
(params.successfulCronAdds ?? 0) > 0 ||
241+
params.didSendDeterministicApprovalPrompt === true
242+
);
243+
}
244+
245+
function hasSuccessfulSourceReplyDelivery(params: {
246+
blockReplyPipeline: { didStream: () => boolean; isAborted: () => boolean } | null;
247+
directlySentBlockKeys?: Set<string>;
248+
messagingToolSentTexts?: string[];
249+
messagingToolSentMediaUrls?: string[];
250+
messagingToolSentTargets?: unknown[];
233251
}): boolean {
234252
return (
235253
(params.blockReplyPipeline?.didStream() && !params.blockReplyPipeline.isAborted()) ||
236254
(params.directlySentBlockKeys?.size ?? 0) > 0 ||
237255
hasNonEmptyStringArray(params.messagingToolSentTexts) ||
238256
hasNonEmptyStringArray(params.messagingToolSentMediaUrls) ||
239-
hasCommittedMessagingTargetDeliveryEvidence(params.messagingToolSentTargets) ||
240-
(params.successfulCronAdds ?? 0) > 0 ||
241-
params.didSendDeterministicApprovalPrompt === true
257+
hasCommittedMessagingTargetDeliveryEvidence(params.messagingToolSentTargets)
242258
);
243259
}
244260

@@ -1795,6 +1811,13 @@ export async function runReplyAgent(params: {
17951811
successfulCronAdds: runResult.successfulCronAdds,
17961812
didSendDeterministicApprovalPrompt: runResult.didSendDeterministicApprovalPrompt,
17971813
});
1814+
const successfulSourceReplyDelivery = hasSuccessfulSourceReplyDelivery({
1815+
blockReplyPipeline,
1816+
directlySentBlockKeys,
1817+
messagingToolSentTexts: runResult.messagingToolSentTexts,
1818+
messagingToolSentMediaUrls: runResult.messagingToolSentMediaUrls,
1819+
messagingToolSentTargets: runResult.messagingToolSentTargets,
1820+
});
17981821
const returnSilentFallbackFailureIfNeeded = async (): Promise<ReplyPayload | undefined> => {
17991822
const silentFallbackFailurePayload = buildSilentFallbackFailurePayload({
18001823
fallbackTransition,
@@ -2276,9 +2299,30 @@ export async function runReplyAgent(params: {
22762299
runtimePolicySessionKey,
22772300
opts,
22782301
});
2279-
const pendingText = sourceReplyPolicy.suppressDelivery
2280-
? ""
2281-
: buildPendingFinalDeliveryText(finalPayloads);
2302+
const finalDeliveryText = buildPendingFinalDeliveryText(finalPayloads);
2303+
// #85714: warn only for unusually substantive private final text. In
2304+
// message_tool_only, no tool call can be intentional silence, and
2305+
// finalDeliveryText also includes verbose/status/usage metadata.
2306+
const assistantFinalText = rawAssistantText ?? "";
2307+
if (
2308+
shouldWarnAboutPrivateMessageToolFinal({
2309+
sourceReplyDeliveryMode: sourceReplyPolicy.sourceReplyDeliveryMode,
2310+
sendPolicyDenied: sourceReplyPolicy.sendPolicyDenied,
2311+
successfulSourceReplyDelivery,
2312+
finalText: assistantFinalText,
2313+
})
2314+
) {
2315+
warnPrivateMessageToolFinal({
2316+
sessionKey,
2317+
channel:
2318+
sessionCtx.OriginatingChannel ??
2319+
sessionCtx.Surface ??
2320+
sessionCtx.Provider ??
2321+
activeSessionEntry?.channel,
2322+
finalTextLength: assistantFinalText.trim().length,
2323+
});
2324+
}
2325+
const pendingText = sourceReplyPolicy.suppressDelivery ? "" : finalDeliveryText;
22822326
const agentId = followupRun.run.agentId;
22832327
const heartbeatAgentCfg = agentId ? resolveAgentConfig(cfg, agentId)?.heartbeat : undefined;
22842328
const heartbeatAckMaxChars = Math.max(

0 commit comments

Comments
 (0)