mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-08 03:30:43 +00:00
* fix: sendPolicy deny suppresses delivery, not inbound processing (#53328) Previously, sendPolicy "deny" returned early before the agent dispatch, preventing the agent from ever seeing the message. This broke the use case of an agent listening on WhatsApp groups with sendPolicy: deny to read messages without replying — the agent couldn't read them at all. Move the deny gate from before the agent dispatch to after it. The agent now processes inbound messages normally (context, memory, tool calls), but all outbound delivery paths are suppressed: final replies, tool results, block replies, working status, plan updates, typing indicators, and TTS payloads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: propagate sendPolicy to ACP tail dispatch instead of hardcoded allow The ACP tail dispatch path (ctx.AcpDispatchTailAfterReset) was passing sendPolicy: "allow" unconditionally, which would bypass delivery suppression in a /reset <tail> turn when the session has sendPolicy deny. Pass through the resolved sendPolicy so the tail dispatch respects it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: guard before_dispatch hook and ACP tail dispatch under sendPolicy deny before_dispatch handled replies were leaking through sendFinalPayload before the suppressDelivery guard was checked. ACP tail dispatch (from /new <tail>) was being rejected by acp-runtime.ts deny checks instead of proceeding with delivery suppression handled downstream. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * auto-reply: propagate deny suppression to reply_dispatch * fix(acp): suppress onReplyStart when user delivery is denied When sendPolicy resolves to "deny", ACP tail dispatch still invoked onReplyStart via startReplyLifecycle before the suppressUserDelivery check. Channels wire onReplyStart to typing indicators, so deny-scoped sessions could still emit outbound typing events on /reset <tail> flows and command bypass paths. Gate startReplyLifecycleOnce on suppressUserDelivery so the lifecycle is marked started but the callback is skipped. Payload delivery was already suppressed; this closes the typing-indicator leak flagged by Codex review (PR #65461 P1/P2). * fix(acp): route non-tail deny turns through ACP when suppression is wired tryDispatchAcpReplyHook was returning early for non-tail, non-command ACP turns under sendPolicy: "deny", causing ACP-bound sessions to fall back to the embedded reply path instead of flowing through acpManager.runTurn. That diverged ACP session state, tool calls, and memory whenever delivery suppression was active. Now the early-return only fires when sendPolicy is "deny" AND the event lacks suppressUserDelivery — i.e., when downstream delivery suppression is not wired up. When suppressUserDelivery is set, dispatch-acp-delivery already drops outbound sends (see onReplyStart / deliver guards), so ACP can safely run the turn with state consistency preserved. Existing behavior preserved: - Command bypass still overrides deny - Tail dispatch still overrides deny - Plain-text deny turns without suppression still short-circuit Addresses Codex bot P1 feedback on #65461. * fix: gate empty-body typing indicator behind suppressTyping (#53328) * fix: guard plugin-binding + fast-abort outbound paths under sendPolicy deny The original PR computed suppressDelivery inside the try block, which was after two outbound paths: 1. The plugin-owned binding block (sendBindingNotice calls for unavailable/declined/error outcomes, plus the plugin's own "handled" outcome) ran before the suppressDelivery flag existed, so plugin notices still leaked under deny. 2. The fast-abort path dispatched "Agent was aborted." via routeReplyToOriginating / sendFinalReply before the flag existed. Move resolveSendPolicy() above the plugin-binding block so suppressDelivery covers every outbound path downstream, matching the PR description's claim that "all outbound paths are guarded by the flag." Plugin-bound inbound handling under deny: plugin handlers can emit outbound replies we cannot rewind, so skip the claim hook entirely under deny and fall through to normal (suppressed) agent processing. touchConversationBindingRecord still runs so binding activity stays tracked. Fast-abort under deny: still run the abort and record the completed state, just don't emit the abort reply. Tests: - suppresses the fast-abort reply under sendPolicy deny - delivers the fast-abort reply normally when sendPolicy is allow (regression guard) - skips plugin-bound claim hook under deny and falls through to suppressed agent dispatch Addresses Codex review findings on PR #65461. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Lobster <lobster@shahine.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
213 lines
5.9 KiB
TypeScript
213 lines
5.9 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from "vitest";
|
|
import { buildTestCtx } from "../auto-reply/reply/test-ctx.js";
|
|
|
|
const { bypassMock, dispatchMock } = vi.hoisted(() => ({
|
|
bypassMock: vi.fn(),
|
|
dispatchMock: vi.fn(),
|
|
}));
|
|
|
|
vi.mock("../auto-reply/reply/dispatch-acp.runtime.js", () => ({
|
|
shouldBypassAcpDispatchForCommand: bypassMock,
|
|
tryDispatchAcpReply: dispatchMock,
|
|
}));
|
|
|
|
import { tryDispatchAcpReplyHook } from "./acp-runtime.js";
|
|
|
|
const event = {
|
|
ctx: buildTestCtx({
|
|
SessionKey: "agent:test:session",
|
|
CommandBody: "/acp cancel",
|
|
BodyForCommands: "/acp cancel",
|
|
BodyForAgent: "/acp cancel",
|
|
}),
|
|
runId: "run-1",
|
|
sessionKey: "agent:test:session",
|
|
inboundAudio: false,
|
|
sessionTtsAuto: "off" as const,
|
|
ttsChannel: undefined,
|
|
suppressUserDelivery: false,
|
|
shouldRouteToOriginating: false,
|
|
originatingChannel: undefined,
|
|
originatingTo: undefined,
|
|
shouldSendToolSummaries: true,
|
|
sendPolicy: "allow" as const,
|
|
};
|
|
|
|
const ctx = {
|
|
cfg: {},
|
|
dispatcher: {
|
|
sendToolResult: () => false,
|
|
sendBlockReply: () => false,
|
|
sendFinalReply: () => false,
|
|
waitForIdle: async () => {},
|
|
getQueuedCounts: () => ({ tool: 0, block: 0, final: 0 }),
|
|
getFailedCounts: () => ({ tool: 0, block: 0, final: 0 }),
|
|
markComplete: () => {},
|
|
},
|
|
abortSignal: undefined,
|
|
onReplyStart: undefined,
|
|
recordProcessed: vi.fn(),
|
|
markIdle: vi.fn(),
|
|
};
|
|
|
|
describe("tryDispatchAcpReplyHook", () => {
|
|
beforeEach(() => {
|
|
vi.clearAllMocks();
|
|
});
|
|
|
|
it("skips ACP runtime lookup for plain-text deny turns", async () => {
|
|
const result = await tryDispatchAcpReplyHook(
|
|
{
|
|
...event,
|
|
sendPolicy: "deny",
|
|
ctx: buildTestCtx({
|
|
SessionKey: "agent:test:session",
|
|
BodyForCommands: "write a test",
|
|
BodyForAgent: "write a test",
|
|
}),
|
|
},
|
|
ctx,
|
|
);
|
|
|
|
expect(result).toBeUndefined();
|
|
expect(bypassMock).not.toHaveBeenCalled();
|
|
expect(dispatchMock).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("skips ACP dispatch when send policy denies delivery and no bypass applies", async () => {
|
|
bypassMock.mockResolvedValue(false);
|
|
|
|
const result = await tryDispatchAcpReplyHook({ ...event, sendPolicy: "deny" }, ctx);
|
|
|
|
expect(result).toBeUndefined();
|
|
expect(dispatchMock).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("dispatches through ACP when command bypass applies", async () => {
|
|
bypassMock.mockResolvedValue(true);
|
|
dispatchMock.mockResolvedValue({
|
|
queuedFinal: true,
|
|
counts: { tool: 1, block: 2, final: 3 },
|
|
});
|
|
|
|
const result = await tryDispatchAcpReplyHook({ ...event, sendPolicy: "deny" }, ctx);
|
|
|
|
expect(result).toEqual({
|
|
handled: true,
|
|
queuedFinal: true,
|
|
counts: { tool: 1, block: 2, final: 3 },
|
|
});
|
|
expect(dispatchMock).toHaveBeenCalledWith(
|
|
expect.objectContaining({
|
|
ctx: event.ctx,
|
|
cfg: ctx.cfg,
|
|
dispatcher: ctx.dispatcher,
|
|
bypassForCommand: true,
|
|
}),
|
|
);
|
|
});
|
|
|
|
it("returns unhandled when ACP dispatcher declines the turn", async () => {
|
|
bypassMock.mockResolvedValue(false);
|
|
dispatchMock.mockResolvedValue(undefined);
|
|
|
|
const result = await tryDispatchAcpReplyHook(event, ctx);
|
|
|
|
expect(result).toBeUndefined();
|
|
expect(dispatchMock).toHaveBeenCalledOnce();
|
|
});
|
|
|
|
it("dispatches non-tail ACP turn under deny when suppressUserDelivery is set", async () => {
|
|
bypassMock.mockResolvedValue(false);
|
|
dispatchMock.mockResolvedValue({
|
|
queuedFinal: false,
|
|
counts: { tool: 0, block: 0, final: 0 },
|
|
});
|
|
|
|
const result = await tryDispatchAcpReplyHook(
|
|
{
|
|
...event,
|
|
sendPolicy: "deny",
|
|
suppressUserDelivery: true,
|
|
ctx: buildTestCtx({
|
|
SessionKey: "agent:test:session",
|
|
BodyForCommands: "write a test",
|
|
BodyForAgent: "write a test",
|
|
}),
|
|
},
|
|
ctx,
|
|
);
|
|
|
|
// Non-tail, non-command ACP turns under deny must still flow through ACP
|
|
// runtime so session/tool state stays consistent — delivery suppression is
|
|
// handled inside the ACP delivery path via suppressUserDelivery.
|
|
expect(dispatchMock).toHaveBeenCalledOnce();
|
|
expect(dispatchMock).toHaveBeenCalledWith(
|
|
expect.objectContaining({
|
|
suppressUserDelivery: true,
|
|
bypassForCommand: false,
|
|
}),
|
|
);
|
|
expect(result).toEqual({
|
|
handled: true,
|
|
queuedFinal: false,
|
|
counts: { tool: 0, block: 0, final: 0 },
|
|
});
|
|
});
|
|
|
|
it("allows tail dispatch through when sendPolicy is deny", async () => {
|
|
bypassMock.mockResolvedValue(false);
|
|
dispatchMock.mockResolvedValue({
|
|
queuedFinal: false,
|
|
counts: { tool: 0, block: 0, final: 0 },
|
|
});
|
|
|
|
const result = await tryDispatchAcpReplyHook(
|
|
{
|
|
...event,
|
|
sendPolicy: "deny",
|
|
isTailDispatch: true,
|
|
ctx: buildTestCtx({
|
|
SessionKey: "agent:test:session",
|
|
BodyForCommands: "continue after reset",
|
|
BodyForAgent: "continue after reset",
|
|
}),
|
|
},
|
|
ctx,
|
|
);
|
|
|
|
// Tail dispatch should proceed despite deny — delivery suppression is handled downstream
|
|
expect(dispatchMock).toHaveBeenCalledOnce();
|
|
expect(result).toEqual({
|
|
handled: true,
|
|
queuedFinal: false,
|
|
counts: { tool: 0, block: 0, final: 0 },
|
|
});
|
|
});
|
|
|
|
it("does not let ACP claim reset commands before local command handling", async () => {
|
|
bypassMock.mockResolvedValue(true);
|
|
dispatchMock.mockResolvedValue(undefined);
|
|
|
|
const result = await tryDispatchAcpReplyHook(
|
|
{
|
|
...event,
|
|
ctx: buildTestCtx({
|
|
SessionKey: "agent:test:session",
|
|
CommandBody: "/new",
|
|
BodyForCommands: "/new",
|
|
BodyForAgent: "/new",
|
|
}),
|
|
},
|
|
ctx,
|
|
);
|
|
|
|
expect(result).toBeUndefined();
|
|
expect(dispatchMock).toHaveBeenCalledWith(
|
|
expect.objectContaining({
|
|
bypassForCommand: true,
|
|
}),
|
|
);
|
|
});
|
|
});
|