Skip to content

Commit f35805d

Browse files
sahilsatralkarsahilsatralkar
committed
fix(slack): require delivery receipts for replies
1 parent 9924fff commit f35805d

12 files changed

Lines changed: 497 additions & 43 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ Docs: https://docs.openclaw.ai
6767
### Fixes
6868

6969
- CLI: strip generic OSC terminal escape payloads from sanitized output fields, preventing clipboard/title escape bodies from leaking into commitment tables and other terminal-safe text. Thanks @shakkernerd.
70+
- Slack: require concrete `chat.postMessage` timestamps before marking replies delivered, emit `message_sent` correlation for normal inbound replies, and warn when message-tool-only final text is intentionally suppressed. Fixes #80715. Thanks @cblustein-cpu.
7071
- Build: replace selected build utility `tsx` preloads with Node native type stripping so Node 26 build paths no longer emit `DEP0205` module loader deprecation warnings. (#78584) Thanks @keshavbotagent.
7172
- Media generation: honor configured music and video generation timeouts when tool calls omit `timeoutMs`, matching image generation behavior. (#80687)
7273
- CLI/update/status: label beta-channel plugin fallback and model-pricing refresh failures as warnings, keeping mixed beta/latest plugin cohorts visible without making core update or Gateway reachability look failed. Fixes #80689. Thanks @BKF-Gitty.

‎extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts‎

Lines changed: 129 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,22 @@ const FINAL_REPLY_TEXT = "final answer";
44
const THREAD_TS = "thread-1";
55
const SAME_TEXT = "same reply";
66

7+
function slackDeliveryResult(overrides?: { messageId?: string; channelId?: string }) {
8+
return {
9+
target: "channel:C123",
10+
messageId: overrides?.messageId ?? "171234.999",
11+
channelId: overrides?.channelId ?? "C123",
12+
threadTs: THREAD_TS,
13+
};
14+
}
15+
716
const createSlackDraftStreamMock = vi.fn();
8-
const deliverRepliesMock = vi.fn(async () => {});
17+
const deliverRepliesMock = vi.fn(async () => [slackDeliveryResult()]);
918
const finalizeSlackPreviewEditMock = vi.fn(async () => {});
19+
const messageHookRunnerMock = {
20+
hasHooks: vi.fn<(name?: string) => boolean>(() => false),
21+
runMessageSent: vi.fn<(event: unknown, hookCtx: unknown) => Promise<void>>(async () => {}),
22+
};
1023
const postMessageMock = vi.fn(async () => ({ ok: true, ts: "171234.999" }));
1124
const updateLastRouteMock = vi.fn(async () => {});
1225
const appendSlackStreamMock = vi.fn(async () => {});
@@ -205,6 +218,7 @@ function createPreparedSlackMessage(params?: {
205218
threadTs?: string;
206219
status: string;
207220
}) => Promise<void>;
221+
runtime?: Record<string, unknown>;
208222
typingReaction?: string;
209223
ackReactionMessageTs?: string;
210224
ackReactionPromise?: Promise<boolean> | null;
@@ -217,7 +231,7 @@ function createPreparedSlackMessage(params?: {
217231
return {
218232
ctx: {
219233
cfg: params?.cfg ?? {},
220-
runtime: {},
234+
runtime: params?.runtime ?? {},
221235
botToken: "xoxb-test",
222236
app: { client: { chat: { postMessage: postMessageMock } } },
223237
teamId: "T1",
@@ -500,6 +514,10 @@ vi.mock("openclaw/plugin-sdk/outbound-runtime", () => ({
500514
resolveAgentOutboundIdentity: () => undefined,
501515
}));
502516

517+
vi.mock("openclaw/plugin-sdk/plugin-runtime", () => ({
518+
getGlobalHookRunner: () => messageHookRunnerMock,
519+
}));
520+
503521
vi.mock("openclaw/plugin-sdk/reply-history", () => ({
504522
clearHistoryEntriesIfEnabled: () => {},
505523
}));
@@ -712,7 +730,12 @@ describe("dispatchPreparedSlackMessage preview fallback", () => {
712730
beforeEach(() => {
713731
createSlackDraftStreamMock.mockReset();
714732
deliverRepliesMock.mockReset();
733+
deliverRepliesMock.mockResolvedValue([slackDeliveryResult()]);
715734
finalizeSlackPreviewEditMock.mockReset();
735+
messageHookRunnerMock.hasHooks.mockReset();
736+
messageHookRunnerMock.hasHooks.mockReturnValue(false);
737+
messageHookRunnerMock.runMessageSent.mockReset();
738+
messageHookRunnerMock.runMessageSent.mockResolvedValue(undefined);
716739
postMessageMock.mockClear();
717740
updateLastRouteMock.mockReset();
718741
appendSlackStreamMock.mockReset();
@@ -757,6 +780,110 @@ describe("dispatchPreparedSlackMessage preview fallback", () => {
757780
expectDeliverReplyCall(0, FINAL_REPLY_TEXT);
758781
});
759782

783+
it("emits message_sent with Slack delivery correlation after normal reply delivery", async () => {
784+
messageHookRunnerMock.hasHooks.mockImplementation((name) => name === "message_sent");
785+
deliverRepliesMock.mockResolvedValueOnce([
786+
slackDeliveryResult({ messageId: "171234.555", channelId: "C999" }),
787+
]);
788+
789+
await dispatchPreparedSlackMessage(
790+
createPreparedSlackMessage({
791+
route: { sessionKey: "agent:agent-1:slack:channel:c999:thread:171234.111" },
792+
}),
793+
);
794+
795+
expect(messageHookRunnerMock.runMessageSent).toHaveBeenCalledTimes(1);
796+
const [event, hookCtx] = messageHookRunnerMock.runMessageSent.mock.calls[0] as unknown as [
797+
Record<string, unknown>,
798+
Record<string, unknown>,
799+
];
800+
expect(event).toMatchObject({
801+
to: "channel:C123",
802+
content: FINAL_REPLY_TEXT,
803+
success: true,
804+
messageId: "171234.555",
805+
sessionKey: "agent:agent-1:slack:channel:c999:thread:171234.111",
806+
});
807+
expect(hookCtx).toMatchObject({
808+
channelId: "slack",
809+
accountId: "default",
810+
conversationId: "C999",
811+
messageId: "171234.555",
812+
sessionKey: "agent:agent-1:slack:channel:c999:thread:171234.111",
813+
});
814+
});
815+
816+
it("keeps rapid same-session final replies observable with distinct Slack message ids", async () => {
817+
messageHookRunnerMock.hasHooks.mockImplementation((name) => name === "message_sent");
818+
mockedDispatchSequence = [
819+
{ kind: "final", payload: { text: "first final" } },
820+
{ kind: "final", payload: { text: "second final" } },
821+
];
822+
deliverRepliesMock
823+
.mockResolvedValueOnce([slackDeliveryResult({ messageId: "171234.001", channelId: "C123" })])
824+
.mockResolvedValueOnce([slackDeliveryResult({ messageId: "171234.002", channelId: "C123" })]);
825+
826+
await dispatchPreparedSlackMessage(createPreparedSlackMessage());
827+
828+
expect(deliverRepliesMock).toHaveBeenCalledTimes(2);
829+
expect(messageHookRunnerMock.runMessageSent).toHaveBeenCalledTimes(2);
830+
const events = messageHookRunnerMock.runMessageSent.mock.calls.map(
831+
([event]) =>
832+
event as {
833+
content?: unknown;
834+
messageId?: unknown;
835+
success?: unknown;
836+
},
837+
);
838+
expect(events).toEqual([
839+
expect.objectContaining({
840+
content: "first final",
841+
messageId: "171234.001",
842+
success: true,
843+
}),
844+
expect.objectContaining({
845+
content: "second final",
846+
messageId: "171234.002",
847+
success: true,
848+
}),
849+
]);
850+
});
851+
852+
it("does not mark normal delivery complete when deliverReplies returns no Slack identity", async () => {
853+
const runtimeError = vi.fn();
854+
messageHookRunnerMock.hasHooks.mockImplementation((name) => name === "message_sent");
855+
deliverRepliesMock.mockResolvedValueOnce([]);
856+
857+
await dispatchPreparedSlackMessage(
858+
createPreparedSlackMessage({
859+
cfg: { messages: { statusReactions: { enabled: true } } },
860+
runtime: { error: runtimeError },
861+
ackReactionMessageTs: "171234.111",
862+
ackReactionPromise: Promise.resolve(true),
863+
}),
864+
);
865+
866+
expect(deliverRepliesMock).toHaveBeenCalledTimes(1);
867+
expect(runtimeError).toHaveBeenCalledWith(
868+
expect.stringContaining("delivery returned no Slack message identity"),
869+
);
870+
expect(messageHookRunnerMock.runMessageSent).toHaveBeenCalledWith(
871+
expect.objectContaining({
872+
to: "channel:C123",
873+
content: FINAL_REPLY_TEXT,
874+
success: false,
875+
error: "Slack delivery returned no message identity",
876+
}),
877+
expect.objectContaining({
878+
channelId: "slack",
879+
accountId: "default",
880+
conversationId: "C123",
881+
}),
882+
);
883+
expect(statusReactionControllerMock.setDone).not.toHaveBeenCalled();
884+
expect(statusReactionControllerMock.restoreInitial).toHaveBeenCalledTimes(1);
885+
});
886+
760887
it("updates non-main DM last-route metadata on the prepared thread session", async () => {
761888
await dispatchPreparedSlackMessage(
762889
createPreparedSlackMessage({

0 commit comments

Comments
 (0)