fix(mattermost): prevent DM replies from creating threads (#72659)

* fix(mattermost): prevent DM replies from creating threads

* fix(mattermost): prevent DM replies from creating threads

* fix(mattermost): prevent DM replies from creating threads
This commit is contained in:
Vincent Koc
2026-04-27 02:37:47 -07:00
committed by GitHub
parent 72f7d7e4ea
commit 59fb5fd3a7
5 changed files with 113 additions and 6 deletions

View File

@@ -2,6 +2,7 @@ import { describe, expect, it, vi } from "vitest";
import {
evaluateMattermostMentionGate,
mapMattermostChannelTypeToChatType,
resolveMattermostTrustedChatKind,
} from "./monitor-gating.js";
describe("mattermost monitor gating", () => {
@@ -13,6 +14,23 @@ describe("mattermost monitor gating", () => {
expect(mapMattermostChannelTypeToChatType(undefined)).toBe("channel");
});
it("derives chat kind from trusted channel lookup before fallback state", () => {
expect(
resolveMattermostTrustedChatKind({
channelType: "O",
fallback: "direct",
}),
).toBe("channel");
expect(
resolveMattermostTrustedChatKind({
channelType: "D",
fallback: "channel",
}),
).toBe("direct");
expect(resolveMattermostTrustedChatKind({ fallback: "group" })).toBe("group");
expect(resolveMattermostTrustedChatKind({})).toBe("channel");
});
it("drops non-mentioned traffic when onchar is enabled but not triggered", () => {
const resolveRequireMention = vi.fn(() => true);

View File

@@ -14,6 +14,17 @@ export function mapMattermostChannelTypeToChatType(channelType?: string | null):
return "channel";
}
export function resolveMattermostTrustedChatKind(params: {
channelType?: string | null;
fallback?: ChatType;
}): ChatType {
const channelType = params.channelType?.trim();
if (channelType) {
return mapMattermostChannelTypeToChatType(channelType);
}
return params.fallback ?? "channel";
}
export type MattermostRequireMentionResolverInput = {
cfg: OpenClawConfig;
channel: "mattermost";

View File

@@ -162,6 +162,7 @@ describe("resolveMattermostReplyRootId with block streaming payloads", () => {
// mode, the deliver callback should still use the existing threadRootId.
expect(
resolveMattermostReplyRootId({
kind: "channel",
threadRootId: "thread-root-1",
replyToId: "streamed-reply-id",
}),
@@ -173,6 +174,7 @@ describe("resolveMattermostReplyRootId with block streaming payloads", () => {
// inbound post id as replyToId from the "all" threading mode.
expect(
resolveMattermostReplyRootId({
kind: "channel",
replyToId: "inbound-post-for-threading",
}),
).toBe("inbound-post-for-threading");
@@ -183,6 +185,7 @@ describe("resolveMattermostReplyRootId", () => {
it("uses replyToId for top-level replies", () => {
expect(
resolveMattermostReplyRootId({
kind: "channel",
replyToId: "inbound-post-123",
}),
).toBe("inbound-post-123");
@@ -191,6 +194,7 @@ describe("resolveMattermostReplyRootId", () => {
it("keeps the thread root when replying inside an existing thread", () => {
expect(
resolveMattermostReplyRootId({
kind: "channel",
threadRootId: "thread-root-456",
replyToId: "child-post-789",
}),
@@ -198,7 +202,36 @@ describe("resolveMattermostReplyRootId", () => {
});
it("falls back to undefined when neither reply target is available", () => {
expect(resolveMattermostReplyRootId({})).toBeUndefined();
expect(resolveMattermostReplyRootId({ kind: "channel" })).toBeUndefined();
});
it("keeps direct-message replies top-level even when a payload reply target exists", () => {
expect(
resolveMattermostReplyRootId({
kind: "direct",
threadRootId: "dm-root-456",
replyToId: "dm-post-123",
}),
).toBeUndefined();
});
it("keeps direct-message replies top-level when only the payload reply target exists", () => {
expect(
resolveMattermostReplyRootId({
kind: "direct",
replyToId: "dm-post-123",
}),
).toBeUndefined();
});
it("keeps group replies on the existing Mattermost thread root", () => {
expect(
resolveMattermostReplyRootId({
kind: "group",
threadRootId: "group-root-456",
replyToId: "group-child-789",
}),
).toBe("group-root-456");
});
});
@@ -206,6 +239,7 @@ describe("canFinalizeMattermostPreviewInPlace", () => {
it("allows in-place finalization when the final reply target matches the preview thread", () => {
expect(
canFinalizeMattermostPreviewInPlace({
kind: "channel",
previewRootId: "thread-root-456",
threadRootId: "thread-root-456",
replyToId: "child-post-789",
@@ -216,10 +250,20 @@ describe("canFinalizeMattermostPreviewInPlace", () => {
it("prevents in-place finalization when a top-level preview would become a threaded reply", () => {
expect(
canFinalizeMattermostPreviewInPlace({
kind: "channel",
replyToId: "child-post-789",
}),
).toBe(false);
});
it("uses direct-message root suppression when checking in-place finalization", () => {
expect(
canFinalizeMattermostPreviewInPlace({
kind: "direct",
replyToId: "dm-post-123",
}),
).toBe(true);
});
});
describe("shouldClearMattermostDraftPreview", () => {
@@ -259,6 +303,7 @@ describe("deliverMattermostReplyWithDraftPreview", () => {
await deliverMattermostReplyWithDraftPreview({
payload: { text: " \n > Reasoning:\n> _hidden_" } as never,
info: { kind: "final" },
kind: "channel",
client: createMattermostClientMock(),
draftStream,
effectiveReplyToId: "thread-root-1",
@@ -282,6 +327,7 @@ describe("deliverMattermostReplyWithDraftPreview", () => {
await deliverMattermostReplyWithDraftPreview({
payload: { text: "All good", replyToId: "reply-1" } as never,
info: { kind: "final" },
kind: "channel",
client: createMattermostClientMock(),
draftStream,
resolvePreviewFinalText: (text) => text?.trim(),
@@ -308,6 +354,7 @@ describe("deliverMattermostReplyWithDraftPreview", () => {
mediaUrl: "https://example.com/a.png",
} as never,
info: { kind: "final" },
kind: "channel",
client: createMattermostClientMock(),
draftStream,
effectiveReplyToId: "thread-root-1",
@@ -330,6 +377,7 @@ describe("deliverMattermostReplyWithDraftPreview", () => {
await deliverMattermostReplyWithDraftPreview({
payload: { text: "Error", isError: true } as never,
info: { kind: "final" },
kind: "channel",
client: createMattermostClientMock(),
draftStream,
effectiveReplyToId: "thread-root-1",
@@ -351,6 +399,7 @@ describe("deliverMattermostReplyWithDraftPreview", () => {
await deliverMattermostReplyWithDraftPreview({
payload: { text: "Final answer", replyToId: "child-post-789" } as never,
info: { kind: "final" },
kind: "channel",
client: createMattermostClientMock(),
draftStream,
effectiveReplyToId: "thread-root-456",
@@ -384,6 +433,7 @@ describe("deliverMattermostReplyWithDraftPreview", () => {
deliverMattermostReplyWithDraftPreview({
payload: { text: "Broken", replyToId: "reply-1" } as never,
info: { kind: "final" },
kind: "channel",
client: createMattermostClientMock(),
draftStream,
resolvePreviewFinalText: (text) => text?.trim(),
@@ -484,6 +534,17 @@ describe("resolveMattermostEffectiveReplyToId", () => {
}),
).toBeUndefined();
});
it("suppresses existing direct-message thread roots", () => {
expect(
resolveMattermostEffectiveReplyToId({
kind: "direct",
postId: "post-123",
replyToMode: "all",
threadRootId: "dm-root-456",
}),
).toBeUndefined();
});
});
describe("resolveMattermostThreadSessionContext", () => {
@@ -541,6 +602,7 @@ describe("resolveMattermostThreadSessionContext", () => {
kind: "direct",
postId: "post-123",
replyToMode: "all",
threadRootId: "dm-root-456",
}),
).toEqual({
effectiveReplyToId: undefined,

View File

@@ -41,6 +41,7 @@ import {
import {
evaluateMattermostMentionGate,
mapMattermostChannelTypeToChatType,
resolveMattermostTrustedChatKind,
} from "./monitor-gating.js";
import {
formatInboundFromLabel,
@@ -94,6 +95,7 @@ import { deactivateSlashCommands, getSlashCommandState } from "./slash-state.js"
export {
evaluateMattermostMentionGate,
mapMattermostChannelTypeToChatType,
resolveMattermostTrustedChatKind,
} from "./monitor-gating.js";
export type {
MattermostMentionGateInput,
@@ -231,9 +233,13 @@ function channelChatType(kind: ChatType): "direct" | "group" | "channel" {
}
export function resolveMattermostReplyRootId(params: {
kind: ChatType;
threadRootId?: string;
replyToId?: string;
}): string | undefined {
if (params.kind === "direct") {
return undefined;
}
const threadRootId = normalizeOptionalString(params.threadRootId);
if (threadRootId) {
return threadRootId;
@@ -242,12 +248,14 @@ export function resolveMattermostReplyRootId(params: {
}
export function canFinalizeMattermostPreviewInPlace(params: {
kind: ChatType;
previewRootId?: string;
threadRootId?: string;
replyToId?: string;
}): boolean {
return (
resolveMattermostReplyRootId({
kind: params.kind,
threadRootId: params.threadRootId,
replyToId: params.replyToId,
}) === params.previewRootId?.trim()
@@ -275,6 +283,7 @@ type MattermostDraftPreviewState = {
type MattermostDraftPreviewDeliverParams = {
payload: ReplyPayload;
info: { kind: "tool" | "block" | "final" };
kind: ChatType;
client: MattermostClient;
draftStream: Pick<
ReturnType<typeof createMattermostDraftStream>,
@@ -313,6 +322,7 @@ export async function deliverMattermostReplyWithDraftPreview(
typeof previewFinalText !== "string" ||
payload.isError ||
!canFinalizeMattermostPreviewInPlace({
kind: params.kind,
previewRootId: params.effectiveReplyToId,
threadRootId: params.effectiveReplyToId,
replyToId: payload.replyToId,
@@ -345,13 +355,13 @@ export function resolveMattermostEffectiveReplyToId(params: {
replyToMode: "off" | "first" | "all" | "batched";
threadRootId?: string | null;
}): string | undefined {
if (params.kind === "direct") {
return undefined;
}
const threadRootId = normalizeOptionalString(params.threadRootId);
if (threadRootId && params.replyToMode !== "off") {
return threadRootId;
}
if (params.kind === "direct") {
return undefined;
}
const postId = normalizeOptionalString(params.postId);
if (!postId) {
return undefined;
@@ -707,6 +717,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
accountId: account.accountId,
agentId: route.agentId,
replyToId: resolveMattermostReplyRootId({
kind,
threadRootId: threadContext.effectiveReplyToId,
replyToId: payload.replyToId,
}),
@@ -915,6 +926,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
accountId: account.accountId,
agentId: params.route.agentId,
replyToId: resolveMattermostReplyRootId({
kind: params.kind,
threadRootId: params.effectiveReplyToId,
replyToId: trimmedPayload.replyToId,
}),
@@ -1208,8 +1220,9 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
}
const channelInfo = await resolveChannelInfo(channelId);
const channelType = payload.data?.channel_type ?? channelInfo?.type ?? undefined;
const kind = mapMattermostChannelTypeToChatType(channelType);
const kind = resolveMattermostTrustedChatKind({
channelType: channelInfo?.type,
});
const chatType = channelChatType(kind);
const senderName =
@@ -1695,6 +1708,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
await deliverMattermostReplyWithDraftPreview({
payload,
info,
kind,
client,
draftStream,
effectiveReplyToId,
@@ -1710,6 +1724,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
accountId: account.accountId,
agentId: route.agentId,
replyToId: resolveMattermostReplyRootId({
kind,
threadRootId: effectiveReplyToId,
replyToId: payload.replyToId,
}),