diff --git a/extensions/matrix/src/exec-approvals-handler.test.ts b/extensions/matrix/src/exec-approvals-handler.test.ts index 20e2e2427f7..f6d4397a6d1 100644 --- a/extensions/matrix/src/exec-approvals-handler.test.ts +++ b/extensions/matrix/src/exec-approvals-handler.test.ts @@ -1,6 +1,9 @@ import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { clearMatrixApprovalReactionTargetsForTest } from "./approval-reactions.js"; +import { + clearMatrixApprovalReactionTargetsForTest, + resolveMatrixApprovalReactionTarget, +} from "./approval-reactions.js"; import { MatrixExecApprovalHandler } from "./exec-approvals-handler.js"; const baseRequest = { @@ -436,6 +439,35 @@ describe("MatrixExecApprovalHandler", () => { ); }); + it("clears tracked approval anchors when the handler stops", async () => { + const cfg = { + channels: { + matrix: { + homeserver: "https://matrix.example.org", + userId: "@bot:example.org", + accessToken: "tok", + execApprovals: { + enabled: true, + approvers: ["@owner:example.org"], + target: "channel", + }, + }, + }, + } as OpenClawConfig; + const { handler } = createHandler(cfg); + + await handler.handleRequested(baseRequest); + await handler.stop(); + + expect( + resolveMatrixApprovalReactionTarget({ + roomId: "!ops:example.org", + eventId: "$m1", + reactionKey: "✅", + }), + ).toBeNull(); + }); + it("honors request decision constraints in pending approval text", async () => { const cfg = { channels: { diff --git a/extensions/matrix/src/exec-approvals-handler.ts b/extensions/matrix/src/exec-approvals-handler.ts index 57eea6fc8b4..5f115d069b5 100644 --- a/extensions/matrix/src/exec-approvals-handler.ts +++ b/extensions/matrix/src/exec-approvals-handler.ts @@ -1,5 +1,6 @@ import { buildExecApprovalPendingReplyPayload, + type ExecApprovalReplyDecision, getExecApprovalApproverDmNoticeText, resolveExecApprovalAllowedDecisions, resolveExecApprovalCommandDisplay, @@ -46,7 +47,11 @@ type PreparedMatrixTarget = { type PendingApprovalContent = { approvalId: string; text: string; - allowedDecisions: readonly ("allow-once" | "allow-always" | "deny")[]; + allowedDecisions: readonly ExecApprovalReplyDecision[]; +}; +type ReactionTargetRef = { + roomId: string; + eventId: string; }; export type MatrixExecApprovalHandlerOpts = { @@ -69,6 +74,23 @@ function normalizePendingMessageIds(entry: PendingMessage): string[] { return Array.from(new Set(entry.messageIds.map((messageId) => messageId.trim()).filter(Boolean))); } +function normalizeReactionTargetRef(params: ReactionTargetRef): ReactionTargetRef | null { + const roomId = params.roomId.trim(); + const eventId = params.eventId.trim(); + if (!roomId || !eventId) { + return null; + } + return { roomId, eventId }; +} + +function buildReactionTargetRefKey(params: ReactionTargetRef): string | null { + const normalized = normalizeReactionTargetRef(params); + if (!normalized) { + return null; + } + return `${normalized.roomId}\u0000${normalized.eventId}`; +} + function isHandlerConfigured(params: { cfg: OpenClawConfig; accountId: string }): boolean { return isMatrixExecApprovalClientEnabled(params); } @@ -126,6 +148,7 @@ function buildResolvedApprovalText(params: { export class MatrixExecApprovalHandler { private readonly runtime: ExecApprovalChannelRuntime; + private readonly trackedReactionTargets = new Map(); private readonly nowMs: () => number; private readonly sendMessage: typeof sendMessageMatrix; private readonly reactMessage: typeof reactMatrixMessage; @@ -209,7 +232,7 @@ export class MatrixExecApprovalHandler { ); const reactionEventId = result.primaryMessageId?.trim() || messageIds[0] || result.messageId.trim(); - registerMatrixApprovalReactionTarget({ + this.trackReactionTarget({ roomId: result.roomId, eventId: reactionEventId, approvalId: pendingContent.approvalId, @@ -247,6 +270,7 @@ export class MatrixExecApprovalHandler { async stop(): Promise { await this.runtime.stop(); + this.clearTrackedReactionTargets(); } async handleRequested(request: ApprovalRequest): Promise { @@ -300,7 +324,7 @@ export class MatrixExecApprovalHandler { const text = buildResolvedApprovalText({ request, resolved }); await Promise.allSettled( entries.map(async (entry) => { - unregisterMatrixApprovalReactionTarget({ + this.untrackReactionTarget({ roomId: entry.roomId, eventId: entry.reactionEventId, }); @@ -330,7 +354,7 @@ export class MatrixExecApprovalHandler { private async clearPending(entries: PendingMessage[]): Promise { await Promise.allSettled( entries.map(async (entry) => { - unregisterMatrixApprovalReactionTarget({ + this.untrackReactionTarget({ roomId: entry.roomId, eventId: entry.reactionEventId, }); @@ -347,4 +371,41 @@ export class MatrixExecApprovalHandler { }), ); } + + private trackReactionTarget( + params: ReactionTargetRef & { + approvalId: string; + allowedDecisions: readonly ExecApprovalReplyDecision[]; + }, + ): void { + const normalized = normalizeReactionTargetRef(params); + const key = normalized ? buildReactionTargetRefKey(normalized) : null; + if (!normalized || !key) { + return; + } + registerMatrixApprovalReactionTarget({ + roomId: normalized.roomId, + eventId: normalized.eventId, + approvalId: params.approvalId, + allowedDecisions: params.allowedDecisions, + }); + this.trackedReactionTargets.set(key, normalized); + } + + private untrackReactionTarget(params: ReactionTargetRef): void { + const normalized = normalizeReactionTargetRef(params); + const key = normalized ? buildReactionTargetRefKey(normalized) : null; + if (!normalized || !key) { + return; + } + unregisterMatrixApprovalReactionTarget(normalized); + this.trackedReactionTargets.delete(key); + } + + private clearTrackedReactionTargets(): void { + for (const target of this.trackedReactionTargets.values()) { + unregisterMatrixApprovalReactionTarget(target); + } + this.trackedReactionTargets.clear(); + } } diff --git a/extensions/matrix/src/matrix/monitor/reaction-events.test.ts b/extensions/matrix/src/matrix/monitor/reaction-events.test.ts index 41cc38ed662..e9e8493c178 100644 --- a/extensions/matrix/src/matrix/monitor/reaction-events.test.ts +++ b/extensions/matrix/src/matrix/monitor/reaction-events.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { clearMatrixApprovalReactionTargetsForTest, registerMatrixApprovalReactionTarget, + resolveMatrixApprovalReactionTarget, } from "../../approval-reactions.js"; import type { CoreConfig } from "../../types.js"; import { handleInboundMatrixReaction } from "./reaction-events.js"; @@ -204,4 +205,140 @@ describe("matrix approval reactions", () => { }); expect(core.system.enqueueSystemEvent).not.toHaveBeenCalled(); }); + + it("resolves registered approval reactions without fetching the target event", async () => { + const core = buildCore(); + registerMatrixApprovalReactionTarget({ + roomId: "!ops:example.org", + eventId: "$approval-msg", + approvalId: "req-123", + allowedDecisions: ["allow-once"], + }); + const client = { + getEvent: vi.fn().mockRejectedValue(new Error("boom")), + } as unknown as Parameters[0]["client"]; + + await handleInboundMatrixReaction({ + client, + core, + cfg: buildConfig(), + accountId: "default", + roomId: "!ops:example.org", + event: { + event_id: "$reaction-1", + origin_server_ts: 123, + content: { + "m.relates_to": { + rel_type: "m.annotation", + event_id: "$approval-msg", + key: "✅", + }, + }, + } as never, + senderId: "@owner:example.org", + senderLabel: "Owner", + selfUserId: "@bot:example.org", + isDirectMessage: false, + logVerboseMessage: vi.fn(), + }); + + expect(client.getEvent).not.toHaveBeenCalled(); + expect(resolveMatrixExecApproval).toHaveBeenCalledWith({ + cfg: buildConfig(), + approvalId: "req-123", + decision: "allow-once", + senderId: "@owner:example.org", + }); + expect(core.system.enqueueSystemEvent).not.toHaveBeenCalled(); + }); + + it("unregisters stale approval anchors after not-found resolution", async () => { + const core = buildCore(); + resolveMatrixExecApproval.mockRejectedValueOnce( + new Error("unknown or expired approval id req-123"), + ); + registerMatrixApprovalReactionTarget({ + roomId: "!ops:example.org", + eventId: "$approval-msg", + approvalId: "req-123", + allowedDecisions: ["deny"], + }); + const client = { + getEvent: vi.fn(), + } as unknown as Parameters[0]["client"]; + + await handleInboundMatrixReaction({ + client, + core, + cfg: buildConfig(), + accountId: "default", + roomId: "!ops:example.org", + event: { + event_id: "$reaction-1", + origin_server_ts: 123, + content: { + "m.relates_to": { + rel_type: "m.annotation", + event_id: "$approval-msg", + key: "❌", + }, + }, + } as never, + senderId: "@owner:example.org", + senderLabel: "Owner", + selfUserId: "@bot:example.org", + isDirectMessage: false, + logVerboseMessage: vi.fn(), + }); + + expect(client.getEvent).not.toHaveBeenCalled(); + expect( + resolveMatrixApprovalReactionTarget({ + roomId: "!ops:example.org", + eventId: "$approval-msg", + reactionKey: "❌", + }), + ).toBeNull(); + }); + + it("skips target fetches for ordinary reactions when notifications are off", async () => { + const core = buildCore(); + const cfg = buildConfig(); + const matrixCfg = cfg.channels?.matrix; + if (!matrixCfg) { + throw new Error("matrix config missing"); + } + matrixCfg.reactionNotifications = "off"; + const client = { + getEvent: vi.fn(), + } as unknown as Parameters[0]["client"]; + + await handleInboundMatrixReaction({ + client, + core, + cfg, + accountId: "default", + roomId: "!ops:example.org", + event: { + event_id: "$reaction-1", + origin_server_ts: 123, + content: { + "m.relates_to": { + rel_type: "m.annotation", + event_id: "$msg-1", + key: "👍", + }, + }, + } as never, + senderId: "@owner:example.org", + senderLabel: "Owner", + selfUserId: "@bot:example.org", + isDirectMessage: false, + logVerboseMessage: vi.fn(), + }); + + expect(client.getEvent).not.toHaveBeenCalled(); + expect(resolveMatrixExecApproval).not.toHaveBeenCalled(); + expect(core.system.enqueueSystemEvent).not.toHaveBeenCalled(); + }); }); diff --git a/extensions/matrix/src/matrix/monitor/reaction-events.ts b/extensions/matrix/src/matrix/monitor/reaction-events.ts index c4edfbdc6a8..f475f9ef975 100644 --- a/extensions/matrix/src/matrix/monitor/reaction-events.ts +++ b/extensions/matrix/src/matrix/monitor/reaction-events.ts @@ -1,5 +1,8 @@ import { getSessionBindingService } from "openclaw/plugin-sdk/conversation-runtime"; -import { resolveMatrixApprovalReactionTarget } from "../../approval-reactions.js"; +import { + resolveMatrixApprovalReactionTarget, + unregisterMatrixApprovalReactionTarget, +} from "../../approval-reactions.js"; import { isApprovalNotFoundError, resolveMatrixExecApproval, @@ -32,15 +35,12 @@ async function maybeResolveMatrixApprovalReaction(params: { cfg: CoreConfig; accountId: string; senderId: string; - roomId: string; - reactionKey: string; + target: ReturnType; targetEventId: string; - targetEvent: MatrixRawEvent | null; - targetSender: string; - selfUserId: string; + roomId: string; logVerboseMessage: (message: string) => void; }): Promise { - if (params.targetSender !== params.selfUserId) { + if (!params.target) { return false; } if ( @@ -52,34 +52,30 @@ async function maybeResolveMatrixApprovalReaction(params: { ) { return false; } - const target = resolveMatrixApprovalReactionTarget({ - roomId: params.roomId, - eventId: params.targetEventId, - reactionKey: params.reactionKey, - }); - if (!target) { - return false; - } try { await resolveMatrixExecApproval({ cfg: params.cfg, - approvalId: target.approvalId, - decision: target.decision, + approvalId: params.target.approvalId, + decision: params.target.decision, senderId: params.senderId, }); params.logVerboseMessage( - `matrix: approval reaction resolved id=${target.approvalId} sender=${params.senderId} decision=${target.decision}`, + `matrix: approval reaction resolved id=${params.target.approvalId} sender=${params.senderId} decision=${params.target.decision}`, ); return true; } catch (err) { if (isApprovalNotFoundError(err)) { + unregisterMatrixApprovalReactionTarget({ + roomId: params.roomId, + eventId: params.targetEventId, + }); params.logVerboseMessage( - `matrix: approval reaction ignored for expired approval id=${target.approvalId} sender=${params.senderId}`, + `matrix: approval reaction ignored for expired approval id=${params.target.approvalId} sender=${params.senderId}`, ); return true; } params.logVerboseMessage( - `matrix: approval reaction failed id=${target.approvalId} sender=${params.senderId}: ${String(err)}`, + `matrix: approval reaction failed id=${params.target.approvalId} sender=${params.senderId}: ${String(err)}`, ); return true; } @@ -105,29 +101,19 @@ export async function handleInboundMatrixReaction(params: { if (params.senderId === params.selfUserId) { return; } - - const targetEvent = await params.client.getEvent(params.roomId, reaction.eventId).catch((err) => { - params.logVerboseMessage( - `matrix: failed resolving reaction target room=${params.roomId} id=${reaction.eventId}: ${String(err)}`, - ); - return null; + const approvalTarget = resolveMatrixApprovalReactionTarget({ + roomId: params.roomId, + eventId: reaction.eventId, + reactionKey: reaction.key, }); - const targetSender = - targetEvent && typeof targetEvent.sender === "string" ? targetEvent.sender.trim() : ""; - if (!targetSender) { - return; - } if ( await maybeResolveMatrixApprovalReaction({ cfg: params.cfg, accountId: params.accountId, senderId: params.senderId, - roomId: params.roomId, - reactionKey: reaction.key, + target: approvalTarget, targetEventId: reaction.eventId, - targetEvent: targetEvent as MatrixRawEvent | null, - targetSender, - selfUserId: params.selfUserId, + roomId: params.roomId, logVerboseMessage: params.logVerboseMessage, }) ) { @@ -140,6 +126,18 @@ export async function handleInboundMatrixReaction(params: { if (notificationMode === "off") { return; } + + const targetEvent = await params.client.getEvent(params.roomId, reaction.eventId).catch((err) => { + params.logVerboseMessage( + `matrix: failed resolving reaction target room=${params.roomId} id=${reaction.eventId}: ${String(err)}`, + ); + return null; + }); + const targetSender = + targetEvent && typeof targetEvent.sender === "string" ? targetEvent.sender.trim() : ""; + if (!targetSender) { + return; + } if (notificationMode === "own" && targetSender !== params.selfUserId) { return; }