Skip to content

Commit 2365a13

Browse files
amknightvincentkoc
amknight
and
vincentkoc
authored
fix(mattermost): keep message tool replies in threads (#93424)
* fix(mattermost): keep message tool replies in threads * fix(outbound): preserve one-root reply threading * fix(outbound): preserve explicit reply target precedence * fix(mattermost): mirror inherited replies to root session * test(outbound): align reply transport contract * fix(mattermost): align mirrored thread root --------- Co-authored-by: Alex Knight <[email protected]> Co-authored-by: Vincent Koc <[email protected]>
1 parent dc09d14 commit 2365a13

6 files changed

Lines changed: 195 additions & 28 deletions

File tree

‎extensions/mattermost/src/channel.test.ts‎

Lines changed: 88 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,14 @@ function requireMattermostReplyToModeResolver() {
7474
return resolveReplyToMode;
7575
}
7676

77+
function requireMattermostThreadTargetMatcher() {
78+
const matchesToolContextTarget = mattermostPlugin.threading?.matchesToolContextTarget;
79+
if (!matchesToolContextTarget) {
80+
throw new Error("mattermost threading.matchesToolContextTarget missing");
81+
}
82+
return matchesToolContextTarget;
83+
}
84+
7785
function requireMattermostSendText() {
7886
const sendText = mattermostPlugin.outbound?.sendText;
7987
if (!sendText) {
@@ -236,6 +244,27 @@ describe("mattermostPlugin", () => {
236244
},
237245
);
238246

247+
it("matches bare Mattermost channel ids against the active channel target", () => {
248+
const matchesToolContextTarget = requireMattermostThreadTargetMatcher();
249+
250+
expect(
251+
matchesToolContextTarget({
252+
target: "tqfek9psh7fw8mpa5berwyytqw",
253+
toolContext: {
254+
currentChannelId: "channel:tqfek9psh7fw8mpa5berwyytqw",
255+
},
256+
}),
257+
).toBe(true);
258+
expect(
259+
matchesToolContextTarget({
260+
target: "tqfek9psh7fw8mpa5berwyytqw",
261+
toolContext: {
262+
currentChannelId: "channel:kqfek9psh7fw8mpa5berwyytqw",
263+
},
264+
}),
265+
).toBe(false);
266+
});
267+
239268
it("exposes the effective reply root as the transport thread", () => {
240269
const resolveReplyTransport = mattermostPlugin.threading?.resolveReplyTransport;
241270
if (!resolveReplyTransport) {
@@ -249,8 +278,30 @@ describe("mattermostPlugin", () => {
249278
threadId: "other-thread",
250279
}),
251280
).toEqual({
252-
replyToId: "post-parent",
253-
threadId: "post-parent",
281+
replyToId: "other-thread",
282+
threadId: "other-thread",
283+
});
284+
expect(
285+
resolveReplyTransport({
286+
cfg: {},
287+
replyToId: "child-post",
288+
replyToIsExplicit: true,
289+
threadId: "root-post",
290+
}),
291+
).toEqual({
292+
replyToId: "root-post",
293+
threadId: "root-post",
294+
});
295+
expect(
296+
resolveReplyTransport({
297+
cfg: {},
298+
replyToId: "child-post",
299+
replyToIsExplicit: false,
300+
threadId: "root-post",
301+
}),
302+
).toEqual({
303+
replyToId: "root-post",
304+
threadId: "root-post",
254305
});
255306
expect(
256307
resolveReplyTransport({
@@ -402,6 +453,17 @@ describe("mattermostPlugin", () => {
402453
},
403454
}),
404455
).toBeUndefined();
456+
expect(
457+
resolveAutoThreadId({
458+
cfg: {},
459+
to: "tqfek9psh7fw8mpa5berwyytqw",
460+
toolContext: {
461+
currentChannelId: "channel:tqfek9psh7fw8mpa5berwyytqw",
462+
currentThreadTs: "root-1",
463+
replyToMode: "all",
464+
},
465+
}),
466+
).toBe("root-1");
405467
expect(
406468
resolveAutoThreadId({
407469
cfg: {},
@@ -714,7 +776,28 @@ describe("mattermostPlugin", () => {
714776
expect(options.replyToId).toBe("post-root");
715777
});
716778

717-
it("keeps explicit reply precedence when threadId is also provided", async () => {
779+
it("uses threadId as the Mattermost root when generic replyTo names a child post", async () => {
780+
const cfg = createMattermostTestConfig();
781+
782+
await mattermostPlugin.actions?.handleAction?.(
783+
createMattermostActionContext({
784+
action: "send",
785+
params: {
786+
to: "channel:CHAN1",
787+
message: "hello",
788+
threadId: "post-root",
789+
replyTo: "child-post",
790+
},
791+
cfg,
792+
accountId: "default",
793+
}),
794+
);
795+
796+
const options = expectSingleMattermostSend("channel:CHAN1", "hello");
797+
expect(options.replyToId).toBe("post-root");
798+
});
799+
800+
it("keeps explicit replyToId precedence when threadId is also provided", async () => {
718801
const cfg = createMattermostTestConfig();
719802

720803
await mattermostPlugin.actions?.handleAction?.(
@@ -723,6 +806,7 @@ describe("mattermostPlugin", () => {
723806
params: {
724807
to: "channel:CHAN1",
725808
message: "hello",
809+
replyToId: "explicit-root",
726810
threadId: "post-root",
727811
replyTo: "child-post",
728812
},
@@ -732,7 +816,7 @@ describe("mattermostPlugin", () => {
732816
);
733817

734818
const options = expectSingleMattermostSend("channel:CHAN1", "hello");
735-
expect(options.replyToId).toBe("child-post");
819+
expect(options.replyToId).toBe("explicit-root");
736820
});
737821

738822
it("routes filePath send actions through Mattermost media upload options", async () => {

‎extensions/mattermost/src/channel.ts‎

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -258,10 +258,8 @@ function resolveMattermostAutoThreadId(params: {
258258
typeof context?.currentMessageId === "number"
259259
? String(context.currentMessageId)
260260
: normalizeOptionalString(context?.currentMessageId);
261-
const currentTarget = context?.currentChannelId
262-
? normalizeMattermostMessagingTarget(context.currentChannelId)
263-
: undefined;
264-
if (currentThreadId && currentTarget === normalizeMattermostMessagingTarget(params.to)) {
261+
const currentTarget = normalizeMattermostThreadTarget(context?.currentChannelId);
262+
if (currentThreadId && currentTarget === normalizeMattermostThreadTarget(params.to)) {
265263
if (replyToId === currentMessageId) {
266264
return currentThreadId;
267265
}
@@ -276,6 +274,28 @@ function resolveMattermostAutoThreadId(params: {
276274
return replyToId;
277275
}
278276

277+
function normalizeMattermostThreadTarget(raw: string | undefined): string | undefined {
278+
const normalized = raw ? normalizeMattermostMessagingTarget(raw) : undefined;
279+
if (normalized) {
280+
return normalized;
281+
}
282+
const trimmed = normalizeOptionalString(raw);
283+
return trimmed && /^[a-z0-9]{26}$/i.test(trimmed) ? `channel:${trimmed}` : undefined;
284+
}
285+
286+
function matchesMattermostToolContextTarget(params: {
287+
target: string;
288+
toolContext: ChannelThreadingToolContext;
289+
}): boolean {
290+
const target = normalizeMattermostThreadTarget(params.target);
291+
if (!target) {
292+
return false;
293+
}
294+
return [params.toolContext.currentChannelId, params.toolContext.currentMessagingTarget].some(
295+
(currentTarget) => normalizeMattermostThreadTarget(currentTarget) === target,
296+
);
297+
}
298+
279299
function normalizeMattermostThreadId(value: string | number | undefined): string | undefined {
280300
return typeof value === "number" ? String(value) : normalizeOptionalString(value);
281301
}
@@ -420,12 +440,13 @@ const mattermostMessageActions: ChannelMessageActionAdapter = {
420440
: typeof params.message === "string"
421441
? params.message
422442
: "";
423-
// Match the shared runner semantics: trim empty reply IDs away before
424-
// falling back from replyToId to replyTo on direct plugin calls.
443+
// Mattermost post root_id is the thread root. A generic replyTo can name
444+
// the current child post, so prefer threadId unless the caller supplied the
445+
// Mattermost-specific replyToId root directly.
425446
const replyToId =
426447
normalizeOptionalString(params.replyToId) ??
427-
normalizeOptionalString(params.replyTo) ??
428-
normalizeOptionalString(params.threadId);
448+
normalizeOptionalString(params.threadId) ??
449+
normalizeOptionalString(params.replyTo);
429450
const resolvedAccountId = accountId || undefined;
430451

431452
const attachmentMedia = collectMattermostAttachmentMedia(params);
@@ -896,16 +917,18 @@ export const mattermostPlugin: ChannelPlugin<ResolvedMattermostAccount> = create
896917
},
897918
resolveAutoThreadId: ({ to, replyToId, toolContext }) =>
898919
resolveMattermostAutoThreadId({ to, replyToId, toolContext }),
920+
matchesToolContextTarget: ({ target, toolContext }) =>
921+
matchesMattermostToolContextTarget({ target, toolContext }),
899922
resolveReplyTransport: ({ threadId, replyToId, replyToIsExplicit, replyDelivery }) => {
900923
const ambientThreadId = threadId != null ? String(threadId) : undefined;
901924
const resolvedThreadId =
902925
replyDelivery?.chatType === "direct"
903926
? undefined
904-
: replyToIsExplicit
905-
? (replyToId ?? ambientThreadId)
906-
: replyDelivery
907-
? (ambientThreadId ?? replyToId ?? undefined)
908-
: (replyToId ?? ambientThreadId);
927+
: replyDelivery
928+
? replyToIsExplicit
929+
? (replyToId ?? ambientThreadId)
930+
: (ambientThreadId ?? replyToId ?? undefined)
931+
: (ambientThreadId ?? replyToId);
909932
return {
910933
replyToId: replyDelivery?.chatType === "direct" ? null : resolvedThreadId,
911934
threadId: resolvedThreadId ?? null,

‎src/infra/outbound/message-action-runner.core-send.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ describe("runMessageAction core send routing", () => {
237237
expect(payload.dryRun).toBe(true);
238238
});
239239

240-
it("sends with the provider-canonical reply root", async () => {
240+
it("preserves an explicit provider reply target with its canonical thread root", async () => {
241241
const sendText = vi.fn().mockResolvedValue({
242242
channel: "testchat",
243243
messageId: "m1",
@@ -312,7 +312,7 @@ describe("runMessageAction core send routing", () => {
312312
});
313313

314314
expect(firstMockArg(sendText, "send text")).toMatchObject({
315-
replyToId: "root-1",
315+
replyToId: "child-1",
316316
threadId: "root-1",
317317
});
318318
});

‎src/infra/outbound/message-action-runner.threading.test.ts‎

Lines changed: 58 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,9 +118,9 @@ describe("message action threading helpers", () => {
118118
toolContext: defaultForumToolContext,
119119
agentId: "main",
120120
resolveAutoThreadId: () => "root-42",
121-
resolveReplyTransport: ({ replyToId }) => ({
122-
replyToId,
123-
threadId: replyToId,
121+
resolveReplyTransport: ({ threadId }) => ({
122+
replyToId: threadId == null ? threadId : String(threadId),
123+
threadId: threadId ?? null,
124124
}),
125125
resolveOutboundSessionRoute,
126126
ensureOutboundSessionEntry,
@@ -191,6 +191,58 @@ describe("message action threading helpers", () => {
191191
expect(resolveAutoThreadId).not.toHaveBeenCalled();
192192
});
193193

194+
it("preserves an explicit reply target through Slack-style reply transport", () => {
195+
const actionParams: Record<string, unknown> = {
196+
channel: "forum",
197+
target: "forum:123",
198+
message: "hi",
199+
threadId: "root-42",
200+
replyTo: "child-777",
201+
};
202+
203+
const resolveAutoThreadId = vi.fn(() => "unexpected");
204+
const resolved = resolveAndApplyOutboundThreadId(actionParams, {
205+
cfg: forumConfig,
206+
to: "forum:123",
207+
toolContext: defaultForumToolContext,
208+
resolveAutoThreadId,
209+
replyToIsExplicit: true,
210+
resolveReplyTransport: ({ replyToId }) => ({
211+
replyToId,
212+
threadId: null,
213+
}),
214+
});
215+
216+
expect(actionParams.threadId).toBe("root-42");
217+
expect(actionParams.replyTo).toBe("child-777");
218+
expect(resolved).toBe("root-42");
219+
expect(resolveAutoThreadId).not.toHaveBeenCalled();
220+
});
221+
222+
it("canonicalizes an inherited reply target through a one-root transport", () => {
223+
const actionParams: Record<string, unknown> = {
224+
channel: "forum",
225+
target: "forum:123",
226+
message: "hi",
227+
threadId: "root-42",
228+
replyTo: "child-777",
229+
};
230+
231+
resolveAndApplyOutboundThreadId(actionParams, {
232+
cfg: forumConfig,
233+
to: "forum:123",
234+
toolContext: defaultForumToolContext,
235+
replyToIsExplicit: false,
236+
resolveReplyTransport: ({ threadId, replyToId, replyToIsExplicit }) => ({
237+
replyToId: replyToIsExplicit || threadId == null ? replyToId : String(threadId),
238+
threadId: threadId ?? null,
239+
}),
240+
});
241+
242+
expect(actionParams.threadId).toBe("root-42");
243+
expect(actionParams.replyTo).toBe("root-42");
244+
});
245+
194246
it.each([
195247
{ name: "threadId null", params: { threadId: null } },
196248
{ name: "topLevel true", params: { topLevel: true } },
@@ -250,9 +302,9 @@ describe("message action threading helpers", () => {
250302
to: "forum:123",
251303
toolContext: defaultForumToolContext,
252304
resolveAutoThreadId: () => "root-42",
253-
resolveReplyTransport: ({ replyToId }) => ({
254-
replyToId,
255-
threadId: replyToId,
305+
resolveReplyTransport: ({ threadId }) => ({
306+
replyToId: threadId == null ? threadId : String(threadId),
307+
threadId: threadId ?? null,
256308
}),
257309
});
258310

‎src/infra/outbound/message-action-runner.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1049,6 +1049,7 @@ async function handleSendAction(ctx: ResolvedActionContext): Promise<MessageActi
10491049
agentId,
10501050
});
10511051

1052+
const replyToIsExplicit = Boolean(readStringParam(params, "replyTo"));
10521053
resolveAndApplyOutboundReplyToId(params, {
10531054
channel,
10541055
toolContext: input.toolContext,
@@ -1067,6 +1068,7 @@ async function handleSendAction(ctx: ResolvedActionContext): Promise<MessageActi
10671068
resolvedTarget,
10681069
resolveAutoThreadId: getChannelPlugin(channel)?.threading?.resolveAutoThreadId,
10691070
resolveReplyTransport: getChannelPlugin(channel)?.threading?.resolveReplyTransport,
1071+
replyToIsExplicit,
10701072
resolveOutboundSessionRoute,
10711073
ensureOutboundSessionEntry,
10721074
});

‎src/infra/outbound/message-action-threading.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ export function resolveAndApplyOutboundThreadId(
3131
toolContext?: ChannelThreadingToolContext;
3232
resolveAutoThreadId?: ResolveAutoThreadId;
3333
resolveReplyTransport?: ResolveReplyTransport;
34+
replyToIsExplicit?: boolean;
3435
},
3536
): string | undefined {
3637
const threadId = readStringParam(actionParams, "threadId");
@@ -51,15 +52,18 @@ export function resolveAndApplyOutboundThreadId(
5152
const resolvedThreadId = threadId ?? autoResolvedThreadId;
5253
if (autoResolvedThreadId && !actionParams.threadId) {
5354
actionParams.threadId = autoResolvedThreadId;
55+
}
56+
if (replyToId && resolvedThreadId) {
5457
const canonicalReplyToId = context.resolveReplyTransport?.({
5558
cfg: context.cfg,
5659
accountId: context.accountId,
57-
threadId: autoResolvedThreadId,
58-
replyToId: autoResolvedThreadId,
60+
threadId: resolvedThreadId,
61+
replyToId,
62+
replyToIsExplicit: context.replyToIsExplicit,
5963
})?.replyToId;
6064
// Providers that use one canonical root for reply and thread routing opt in
6165
// through resolveReplyTransport. Other transports keep message replies intact.
62-
if (replyToId && canonicalReplyToId && replyToId !== canonicalReplyToId) {
66+
if (canonicalReplyToId && replyToId !== canonicalReplyToId) {
6367
actionParams.replyTo = canonicalReplyToId;
6468
}
6569
}
@@ -172,6 +176,7 @@ export async function prepareOutboundMirrorRoute(params: {
172176
resolvedTarget?: ResolvedMessagingTarget;
173177
resolveAutoThreadId?: ResolveAutoThreadId;
174178
resolveReplyTransport?: ResolveReplyTransport;
179+
replyToIsExplicit?: boolean;
175180
resolveOutboundSessionRoute: (
176181
params: ResolveOutboundSessionRouteParams,
177182
) => Promise<OutboundSessionRoute | null>;
@@ -192,6 +197,7 @@ export async function prepareOutboundMirrorRoute(params: {
192197
toolContext: params.toolContext,
193198
resolveAutoThreadId: params.resolveAutoThreadId,
194199
resolveReplyTransport: params.resolveReplyTransport,
200+
replyToIsExplicit: params.replyToIsExplicit,
195201
});
196202
const replyToId = readStringParam(params.actionParams, "replyTo");
197203
const outboundRoute =

0 commit comments

Comments
 (0)