From 09ca7d29fdb773e38e4a8b59325b98b4553b3e8d Mon Sep 17 00:00:00 2001 From: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Date: Fri, 22 May 2026 03:00:53 +0000 Subject: [PATCH] fix(clawsweeper): address review for automerge-openclaw-openclaw-83348 (1) --- .../monitor.inbound-system-event.test.ts | 127 ++++++++++++++++-- .../mattermost/src/mattermost/monitor.ts | 8 +- .../src/monitor-handler.test-helpers.ts | 31 +++-- .../message-handler.authz.test.ts | 52 +++++++ .../message-handler.test-support.ts | 10 ++ .../src/monitor-handler/message-handler.ts | 16 ++- 6 files changed, 216 insertions(+), 28 deletions(-) diff --git a/extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts b/extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts index 92d30dd730d..495db52d56e 100644 --- a/extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts +++ b/extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts @@ -1,3 +1,4 @@ +import { createInboundDebouncer } from "openclaw/plugin-sdk/channel-inbound-debounce"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { monitorMattermostProvider } from "./monitor.js"; import type { OpenClawConfig, RuntimeEnv } from "./runtime-api.js"; @@ -148,6 +149,14 @@ function createRuntimeCore( mainSessionKey?: string; sessionKey?: string; }, + overrides: { + inboundDebounceMs?: number; + isControlCommandMessage?: (text?: string) => boolean; + shouldComputeCommandAuthorized?: (text?: string) => boolean; + shouldHandleTextCommands?: () => boolean; + textHasControlCommand?: (text?: string) => boolean; + createInboundDebouncer?: typeof createInboundDebouncer; + } = {}, ) { const runPrepared = vi.fn( async (turn: { @@ -230,21 +239,23 @@ function createRuntimeCore( record: vi.fn(), }, commands: { - isControlCommandMessage: () => false, - shouldHandleTextCommands: () => false, + isControlCommandMessage: overrides.isControlCommandMessage ?? (() => false), + shouldComputeCommandAuthorized: overrides.shouldComputeCommandAuthorized ?? (() => false), + shouldHandleTextCommands: overrides.shouldHandleTextCommands ?? (() => false), }, debounce: { - resolveInboundDebounceMs: () => 0, - createInboundDebouncer: (params: { - onFlush: (entries: T[]) => Promise | void; - }) => ({ - enqueue: async (entry: T) => { - await params.onFlush([entry]); - }, - }), + resolveInboundDebounceMs: () => overrides.inboundDebounceMs ?? 0, + createInboundDebouncer: + overrides.createInboundDebouncer ?? + ((params: { onFlush: (entries: T[]) => Promise | void }) => ({ + enqueue: async (entry: T) => { + await params.onFlush([entry]); + }, + })), }, groups: { - resolveRequireMention: () => false, + resolveRequireMention: (params: { requireMentionOverride?: boolean }) => + params.requireMentionOverride ?? false, }, media: { readRemoteMediaBuffer: vi.fn(), @@ -317,7 +328,7 @@ function createRuntimeCore( text: { chunkMarkdownTextWithMode: (text: string) => [text], convertMarkdownTables: (text: string) => text, - hasControlCommand: () => false, + hasControlCommand: overrides.textHasControlCommand ?? (() => false), resolveChunkMode: () => "off", resolveMarkdownTableMode: () => "off", resolveTextChunkLimit: () => 4000, @@ -544,6 +555,98 @@ describe("mattermost inbound user posts", () => { expect(runtimeCore.channel.session.recordInboundSession).not.toHaveBeenCalled(); }); + it("flushes pending group text before authorizing a bare abort without a mention", async () => { + const socket = new FakeWebSocket(); + const abortController = new AbortController(); + mockState.abortController = abortController; + const mentionConfig: OpenClawConfig = { + commands: { useAccessGroups: false }, + messages: { inbound: { debounceMs: 60_000 } }, + channels: { + mattermost: { + enabled: true, + baseUrl: "https://mattermost.example.com", + botToken: "bot-token", + chatmode: "oncall", + dmPolicy: "open", + groupPolicy: "open", + }, + }, + }; + const isBareAbort = (text?: string) => ["abort", "stop"].includes(text?.trim() ?? ""); + const runtimeCore = createRuntimeCore(mentionConfig, undefined, { + inboundDebounceMs: 60_000, + createInboundDebouncer, + isControlCommandMessage: isBareAbort, + shouldComputeCommandAuthorized: isBareAbort, + shouldHandleTextCommands: () => true, + textHasControlCommand: () => false, + }); + mockState.runtimeCore = runtimeCore; + + const monitor = monitorMattermostProvider({ + config: mentionConfig, + runtime: testRuntime(), + abortSignal: abortController.signal, + webSocketFactory: () => socket, + }); + + await vi.waitFor(() => { + expect(socket.openListenerCount).toBeGreaterThan(0); + }); + socket.emitOpen(); + + await socket.emitMessage({ + event: "posted", + data: { + channel_id: "chan-1", + channel_name: "town-square", + channel_display_name: "Town Square", + sender_name: "alice", + post: JSON.stringify({ + id: "post-pending", + channel_id: "chan-1", + user_id: "user-1", + message: "pending text", + create_at: 1_714_000_000_000, + }), + }, + broadcast: { + channel_id: "chan-1", + user_id: "user-1", + }, + }); + expect(mockState.dispatchReplyFromConfig).not.toHaveBeenCalled(); + + await socket.emitMessage({ + event: "posted", + data: { + channel_id: "chan-1", + channel_name: "town-square", + channel_display_name: "Town Square", + sender_name: "alice", + post: JSON.stringify({ + id: "post-abort", + channel_id: "chan-1", + user_id: "user-1", + message: "abort", + create_at: 1_714_000_000_100, + }), + }, + broadcast: { + channel_id: "chan-1", + user_id: "user-1", + }, + }); + socket.emitClose(1000); + await monitor; + + expect(mockState.dispatchReplyFromConfig).toHaveBeenCalledTimes(1); + const ctx = mockState.dispatchReplyFromConfig.mock.calls.at(0)?.[0].ctx; + expect(ctx?.BodyForAgent).toBe("abort"); + expect(ctx?.CommandAuthorized).toBe(true); + }); + it("pins direct-message main route updates to the configured owner", async () => { const socket = new FakeWebSocket(); const abortController = new AbortController(); diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index a7d848489d1..2a8f5f25bf4 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -1318,8 +1318,10 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} cfg, surface: "mattermost", }); - const hasControlCommand = core.channel.text.hasControlCommand(rawText, cfg); - const isControlCommand = allowTextCommands && hasControlCommand; + const isControlCommand = + allowTextCommands && core.channel.commands.isControlCommandMessage(rawText, cfg); + const shouldComputeCommandAuthorized = + allowTextCommands && core.channel.commands.shouldComputeCommandAuthorized(rawText, cfg); const accessDecision = await resolveMattermostMonitorInboundAccess({ account, cfg, @@ -1330,7 +1332,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} groupPolicy, readStoreAllowFrom: pairing.readAllowFromStore, allowTextCommands, - hasControlCommand, + hasControlCommand: shouldComputeCommandAuthorized, eventKind: "message", mayPair: true, }); diff --git a/extensions/msteams/src/monitor-handler.test-helpers.ts b/extensions/msteams/src/monitor-handler.test-helpers.ts index 46cc36f4a73..633cca16a13 100644 --- a/extensions/msteams/src/monitor-handler.test-helpers.ts +++ b/extensions/msteams/src/monitor-handler.test-helpers.ts @@ -15,6 +15,11 @@ type MSTeamsTestRuntimeOptions = { recordInboundSession?: ReturnType; resolveAgentRoute?: (params: RuntimeRoutePeer) => unknown; hasControlCommand?: PluginRuntime["channel"]["text"]["hasControlCommand"]; + isControlCommandMessage?: PluginRuntime["channel"]["commands"]["isControlCommandMessage"]; + shouldComputeCommandAuthorized?: PluginRuntime["channel"]["commands"]["shouldComputeCommandAuthorized"]; + shouldHandleTextCommands?: PluginRuntime["channel"]["commands"]["shouldHandleTextCommands"]; + createInboundDebouncer?: PluginRuntime["channel"]["debounce"]["createInboundDebouncer"]; + resolveInboundDebounceMs?: PluginRuntime["channel"]["debounce"]["resolveInboundDebounceMs"]; resolveTextChunkLimit?: () => number; resolveStorePath?: () => string; }; @@ -66,21 +71,29 @@ export function installMSTeamsTestRuntime(options: MSTeamsTestRuntimeOptions = { system: { enqueueSystemEvent: options.enqueueSystemEvent ?? vi.fn() }, channel: { debounce: { - resolveInboundDebounceMs: () => 0, - createInboundDebouncer: (params: { - onFlush: (entries: T[]) => Promise; - }): { enqueue: (entry: T) => Promise } => ({ - enqueue: async (entry: T) => { - await params.onFlush([entry]); - }, - }), + resolveInboundDebounceMs: + options.resolveInboundDebounceMs ?? + ((() => 0) as PluginRuntime["channel"]["debounce"]["resolveInboundDebounceMs"]), + createInboundDebouncer: + options.createInboundDebouncer ?? + ((params: { + onFlush: (entries: T[]) => Promise; + }): { enqueue: (entry: T) => Promise } => ({ + enqueue: async (entry: T) => { + await params.onFlush([entry]); + }, + })), }, pairing: { readAllowFromStore: options.readAllowFromStore ?? vi.fn(async () => []), upsertPairingRequest: options.upsertPairingRequest ?? vi.fn(async () => null), }, commands: { - isControlCommandMessage: options.hasControlCommand ?? (() => false), + isControlCommandMessage: + options.isControlCommandMessage ?? options.hasControlCommand ?? (() => false), + shouldComputeCommandAuthorized: + options.shouldComputeCommandAuthorized ?? options.hasControlCommand ?? (() => false), + shouldHandleTextCommands: options.shouldHandleTextCommands ?? (() => true), }, text: { hasControlCommand: options.hasControlCommand ?? (() => false), diff --git a/extensions/msteams/src/monitor-handler/message-handler.authz.test.ts b/extensions/msteams/src/monitor-handler/message-handler.authz.test.ts index 763a92b8e7d..25afe57de58 100644 --- a/extensions/msteams/src/monitor-handler/message-handler.authz.test.ts +++ b/extensions/msteams/src/monitor-handler/message-handler.authz.test.ts @@ -1,3 +1,4 @@ +import { createInboundDebouncer } from "openclaw/plugin-sdk/channel-inbound-debounce"; import { describe, expect, it, vi } from "vitest"; import type { OpenClawConfig, PluginRuntime } from "../../runtime-api.js"; import type { GraphThreadMessage } from "../graph-thread.js"; @@ -84,6 +85,11 @@ describe("msteams monitor handler authz", () => { cfg: OpenClawConfig, options: { hasControlCommand?: PluginRuntime["channel"]["text"]["hasControlCommand"]; + isControlCommandMessage?: PluginRuntime["channel"]["commands"]["isControlCommandMessage"]; + shouldComputeCommandAuthorized?: PluginRuntime["channel"]["commands"]["shouldComputeCommandAuthorized"]; + shouldHandleTextCommands?: PluginRuntime["channel"]["commands"]["shouldHandleTextCommands"]; + createInboundDebouncer?: PluginRuntime["channel"]["debounce"]["createInboundDebouncer"]; + resolveInboundDebounceMs?: PluginRuntime["channel"]["debounce"]["resolveInboundDebounceMs"]; } = {}, ) { const readAllowFromStore = vi.fn(async () => ["attacker-aad"]); @@ -100,6 +106,11 @@ describe("msteams monitor handler authz", () => { accountId: "default", })), hasControlCommand: options.hasControlCommand, + isControlCommandMessage: options.isControlCommandMessage, + shouldComputeCommandAuthorized: options.shouldComputeCommandAuthorized, + shouldHandleTextCommands: options.shouldHandleTextCommands, + createInboundDebouncer: options.createInboundDebouncer, + resolveInboundDebounceMs: options.resolveInboundDebounceMs, }); } @@ -606,6 +617,47 @@ describe("msteams monitor handler authz", () => { expect(runtimeApiMockState.dispatchReplyFromConfigWithSettledDispatcher).not.toHaveBeenCalled(); }); + it("flushes pending group text before authorizing a bare abort without a mention", async () => { + resetThreadMocks(); + const isBareAbort = vi.fn((text?: string) => + ["abort", "stop"].includes(text?.trim().toLowerCase() ?? ""), + ); + const { deps } = createDeps( + { + commands: { useAccessGroups: false }, + messages: { inbound: { debounceMs: 60_000 } }, + channels: { + msteams: { + groupPolicy: "open", + requireMention: true, + }, + }, + } as OpenClawConfig, + { + hasControlCommand: vi.fn(() => false), + isControlCommandMessage: isBareAbort, + shouldComputeCommandAuthorized: isBareAbort, + shouldHandleTextCommands: vi.fn(() => true), + createInboundDebouncer, + resolveInboundDebounceMs: vi.fn(() => 60_000), + }, + ); + + const handler = createMSTeamsMessageHandler(deps); + await handler(createAttackerGroupActivity({ text: "pending text" })); + expect(runtimeApiMockState.dispatchReplyFromConfigWithSettledDispatcher).not.toHaveBeenCalled(); + + await handler(createAttackerGroupActivity({ text: "abort" })); + + expect(runtimeApiMockState.dispatchReplyFromConfigWithSettledDispatcher).toHaveBeenCalledTimes( + 1, + ); + const dispatched = firstSettledDispatch(); + const ctxPayload = recordFromMockCall(dispatched.ctxPayload); + expect(ctxPayload.BodyForAgent).toBe("abort"); + expect(ctxPayload.CommandAuthorized).toBe(true); + }); + it("marks skipped channel message system events as non-owner", async () => { resetThreadMocks(); const { deps, enqueueSystemEvent } = createDeps({ diff --git a/extensions/msteams/src/monitor-handler/message-handler.test-support.ts b/extensions/msteams/src/monitor-handler/message-handler.test-support.ts index fe13e3f6e22..d3f7679867f 100644 --- a/extensions/msteams/src/monitor-handler/message-handler.test-support.ts +++ b/extensions/msteams/src/monitor-handler/message-handler.test-support.ts @@ -12,6 +12,11 @@ type MessageHandlerDepsOptions = { recordInboundSession?: ReturnType; resolveAgentRoute?: (params: { peer: { kind: string; id: string } }) => unknown; hasControlCommand?: PluginRuntime["channel"]["text"]["hasControlCommand"]; + isControlCommandMessage?: PluginRuntime["channel"]["commands"]["isControlCommandMessage"]; + shouldComputeCommandAuthorized?: PluginRuntime["channel"]["commands"]["shouldComputeCommandAuthorized"]; + shouldHandleTextCommands?: PluginRuntime["channel"]["commands"]["shouldHandleTextCommands"]; + createInboundDebouncer?: PluginRuntime["channel"]["debounce"]["createInboundDebouncer"]; + resolveInboundDebounceMs?: PluginRuntime["channel"]["debounce"]["resolveInboundDebounceMs"]; }; export function createMessageHandlerDeps( @@ -41,6 +46,11 @@ export function createMessageHandlerDeps( recordInboundSession, resolveAgentRoute, hasControlCommand: options.hasControlCommand, + isControlCommandMessage: options.isControlCommandMessage, + shouldComputeCommandAuthorized: options.shouldComputeCommandAuthorized, + shouldHandleTextCommands: options.shouldHandleTextCommands, + createInboundDebouncer: options.createInboundDebouncer, + resolveInboundDebounceMs: options.resolveInboundDebounceMs, resolveTextChunkLimit: () => 4000, resolveStorePath: () => "/tmp/test-store", }); diff --git a/extensions/msteams/src/monitor-handler/message-handler.ts b/extensions/msteams/src/monitor-handler/message-handler.ts index 91807a1ec09..427f1778029 100644 --- a/extensions/msteams/src/monitor-handler/message-handler.ts +++ b/extensions/msteams/src/monitor-handler/message-handler.ts @@ -281,6 +281,14 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { threadId, }); + const allowTextCommands = core.channel.commands.shouldHandleTextCommands({ + cfg, + surface: "msteams", + }); + const isControlCommand = + allowTextCommands && core.channel.commands.isControlCommandMessage(text, cfg); + const shouldComputeCommandAuthorized = + allowTextCommands && core.channel.commands.shouldComputeCommandAuthorized(text, cfg); const { dmPolicy, senderId, @@ -295,7 +303,7 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { } = await resolveMSTeamsSenderAccess({ cfg, activity, - hasControlCommand: core.channel.text.hasControlCommand(text, cfg), + hasControlCommand: shouldComputeCommandAuthorized, }); const commandAuthorized = commandAccess.requested ? commandAccess.authorized : undefined; const effectiveDmAllowFrom = senderAccess.effectiveAllowFrom; @@ -522,9 +530,9 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { policy: { isGroup: !isDirectMessage, requireMention, - allowTextCommands: false, - hasControlCommand: false, - commandAuthorized: false, + allowTextCommands, + hasControlCommand: isControlCommand, + commandAuthorized: commandAuthorized === true, }, });