Adversarial self-review

Reviewed by reading the full changed files in context and tracing both the runner send path (executeSendAction → tryHandleWithPluginAction → dispatchChannelMessageAction → handleAction) and the canonicalization in resolveAndApplyOutboundThreadId, plus the Mattermost + Slack threading adapters and the existing tests. Did not run the suite locally (my checkout is behind origin/main); relied on static analysis + the green Testbox run cited above.

Verified understanding

Three coupled changes:

1. extensions/mattermost/src/channel.ts:446-449 — replyToId ?? threadId ?? replyTo precedence flip. Reachable in production: MM supportsAction returns true for send and has no actions.prepareSendPayload, so sends route through handleAction.
2. extensions/mattermost/src/channel.ts:277-296 — normalizeMattermostThreadTarget / matchesMattermostToolContextTarget. Genuine root cause: normalizeMattermostMessagingTarget returns undefined for bare names, so the target comparison in resolveMattermostAutoThreadId silently failed and auto-threading was skipped.
3. src/infra/outbound/message-action-threading.ts:52-67 — canonicalization now runs whenever replyToId && resolvedThreadId, extending it to the explicit-threadId case (was auto-resolved-only).

Well-targeted fix, correct root cause, comments updated, backward-compat for non-opt-in transports still covered by the existing "uses explicit forum threadId without rewriting replyTo" test.

Findings

1. [Medium] The shared message-action-threading.ts change alters Slack, but only Mattermost was verified.
Slack is the other provider implementing resolveReplyTransport (extensions/slack/src/channel.ts:817). The new case-3 path is reachable for Slack (explicit threadId + auto-populated replyTo): it now canonicalizes replyTo to the explicit threadId instead of threading under the inbound message ts. I believe this is more correct (respects the agent's explicit threadId; Slack flattens threads), so no concrete regression — but it's a core/shared edit that changes Slack's outbound thread_ts while the PR is scoped/verified Mattermost-only, and no Slack test covers this path (action-threading.test.ts only covers resolveSlackAutoThreadId; the core canonicalization test only covers the unchanged auto-resolved case). Please add a Slack case-3 assertion or document the Slack reasoning + run Slack threading tests.

2. [Low-Medium] normalizeMattermostThreadTarget ignores 53-char DM channel IDs and is a third MM-id encoding.
channel.ts:283 matches only /^[a-z0-9]{26}$/i, but existing looksLikeMattermostTargetId recognizes both 26-char IDs and 53-char DM IDs (<26>__<26>). A bare 53-char DM channel id as to won't normalize, so auto-threading / same-target matching (and the replyToMode:"first" consumption riding on matchesToolContextTarget) misses for that shape. Consider reusing/extending looksLikeMattermostTargetId so the ID heuristic lives in one place, or comment why 26-char-only.

3. [Low] Canonicalization bypasses the MM DM (chatType:"direct") branch.
message-action-threading.ts:56-61 calls resolveReplyTransport without replyDelivery, so MM's chatType === "direct" branch (which returns replyToId: null, channel.ts:907-911) is skipped and a DM with a resolvable thread gets replyTo→root. Mirrors pre-existing auto-path behavior and MM DMs do support threads, so likely fine — worth confirming against the in-flight mattermost-dm-validation work.

4. [Nit] Redundant guard at message-action-threading.ts:64: replyToId && is already guaranteed truthy by the outer if (replyToId && resolvedThreadId) (line 55).

5. [Nit] /^[a-z0-9]{26}$/i could treat a 26-char all-alnum channel name as an ID; impact is negligible (only the threading comparison, not target resolution), but a one-line "deliberate heuristic" comment would help.

6. [Nit] PR body's git diff --check lists session-route.ts / session-route.test.ts, which aren't in this 4-file PR — tidy the body.

Bottom line

Correct root-cause fix with good Mattermost coverage. Main thing to address before merge is #1 — the core/shared edit changes Slack's threading; add a Slack assertion or document/verify that path. #2-#3 are edge-correctness follow-ups; #4-#6 are cleanup.

land-ready verification:

• canonicalized generic Mattermost message-action replies to the resolved root so delivery and outbound session mirroring stay on the same thread; direct provider actions still preserve explicit reply-target precedence
• dropped an accidental unrelated session-cache commit from the PR branch before this final head
• node scripts/run-vitest.mjs src/infra/outbound/message-action-runner.core-send.test.ts src/infra/outbound/message-action-runner.threading.test.ts extensions/mattermost/src/channel.test.ts extensions/slack/src/channel.test.ts — 123 passed
• node_modules/.bin/oxfmt --check ... and git diff --check — clean
• fresh autoreview — clean, no accepted/actionable findings
• all relevant CI checks are green; check-additional-runtime-topology-architecture is the shared current-main failure at src/agents/auth-profiles/sqlite.ts:103, outside this PR

This lands the scoped message-tool threading fix. #45082 remains open because its wider proactive-message behavior is only partially addressed here.