From 84d0a794ec9b3960025369174a84c081865c4ad8 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 00:14:46 +0000 Subject: [PATCH] fix: harden matrix startup errors + add regressions (#31023) (thanks @efe-arv) --- CHANGELOG.md | 1 + .../matrix/src/matrix/client/shared.test.ts | 85 +++++++++++++++++++ extensions/matrix/src/matrix/client/shared.ts | 7 +- .../matrix/src/matrix/monitor/direct.test.ts | 63 ++++++++++++++ .../matrix/src/matrix/monitor/events.test.ts | 34 ++++++++ .../matrix/src/matrix/monitor/index.test.ts | 18 ++++ extensions/matrix/src/matrix/monitor/index.ts | 9 +- 7 files changed, 212 insertions(+), 5 deletions(-) create mode 100644 extensions/matrix/src/matrix/client/shared.test.ts create mode 100644 extensions/matrix/src/matrix/monitor/direct.test.ts create mode 100644 extensions/matrix/src/matrix/monitor/index.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 21db9cd6340..f518bdd691b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,7 @@ Docs: https://docs.openclaw.ai - Plugins/Discovery precedence: load bundled plugins before auto-discovered global extensions so bundled channel plugins win duplicate-ID resolution by default (explicit `plugins.load.paths` overrides remain highest precedence), with loader regression coverage. Landed from contributor PR #29710 by @Sid-Qin. Thanks @Sid-Qin. - Discord/Reconnect integrity: release Discord message listener lane immediately while preserving serialized handler execution, add HELLO-stall resume-first recovery with bounded fresh-identify fallback after repeated stalls, and extend lifecycle/listener regression coverage for forced reconnect scenarios. Landed from contributor PR #29508 by @cgdusek. Thanks @cgdusek. +- Matrix/Conduit compatibility: avoid blocking startup on non-resolving Matrix sync start, preserve startup error propagation, prevent duplicate monitor listener registration, remove unreliable 2-member DM heuristics, accept `!room` IDs without alias resolution, and add matrix monitor/client regression coverage. Landed from contributor PR #31023 by @efe-arv. Thanks @efe-arv. - Security/Skills: harden skill installer metadata parsing by rejecting unsafe installer specs (brew/node/go/uv/download) and constrain plugin-declared skill directories to the plugin root (including symlink-escape checks), with regression coverage. - Discord/DM command auth: unify DM allowlist + pairing-store authorization across message preflight and native command interactions so DM command gating is consistent for `open`/`pairing`/`allowlist` policies. - Sessions/Usage accounting: persist `cacheRead`/`cacheWrite` from the latest call snapshot (`lastCallUsage`) instead of accumulated multi-call totals, preventing inflated token/cost reporting in long tool/compaction runs. (#31005) diff --git a/extensions/matrix/src/matrix/client/shared.test.ts b/extensions/matrix/src/matrix/client/shared.test.ts new file mode 100644 index 00000000000..356e45a3542 --- /dev/null +++ b/extensions/matrix/src/matrix/client/shared.test.ts @@ -0,0 +1,85 @@ +import type { MatrixClient } from "@vector-im/matrix-bot-sdk"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { resolveSharedMatrixClient, stopSharedClient } from "./shared.js"; +import type { MatrixAuth } from "./types.js"; + +const createMatrixClientMock = vi.hoisted(() => vi.fn()); + +vi.mock("./create-client.js", () => ({ + createMatrixClient: (...args: unknown[]) => createMatrixClientMock(...args), +})); + +function makeAuth(suffix: string): MatrixAuth { + return { + homeserver: "https://matrix.example.org", + userId: `@bot-${suffix}:example.org`, + accessToken: `token-${suffix}`, + encryption: false, + }; +} + +function createMockClient(startImpl: () => Promise): MatrixClient { + return { + start: vi.fn(startImpl), + stop: vi.fn(), + getJoinedRooms: vi.fn().mockResolvedValue([]), + crypto: undefined, + } as unknown as MatrixClient; +} + +describe("resolveSharedMatrixClient startup behavior", () => { + afterEach(() => { + stopSharedClient(); + createMatrixClientMock.mockReset(); + vi.useRealTimers(); + }); + + it("propagates the original start error during initialization", async () => { + vi.useFakeTimers(); + const startError = new Error("bad token"); + const client = createMockClient( + () => + new Promise((_resolve, reject) => { + setTimeout(() => reject(startError), 1); + }), + ); + createMatrixClientMock.mockResolvedValue(client); + + const startPromise = resolveSharedMatrixClient({ + auth: makeAuth("start-error"), + }); + const startExpectation = expect(startPromise).rejects.toBe(startError); + + await vi.advanceTimersByTimeAsync(2001); + await startExpectation; + }); + + it("retries start after a late start-loop failure", async () => { + vi.useFakeTimers(); + let rejectFirstStart: ((err: unknown) => void) | undefined; + const firstStart = new Promise((_resolve, reject) => { + rejectFirstStart = reject; + }); + const secondStart = new Promise(() => {}); + const startMock = vi.fn().mockReturnValueOnce(firstStart).mockReturnValueOnce(secondStart); + const client = createMockClient(startMock); + createMatrixClientMock.mockResolvedValue(client); + + const firstResolve = resolveSharedMatrixClient({ + auth: makeAuth("late-failure"), + }); + await vi.advanceTimersByTimeAsync(2000); + await expect(firstResolve).resolves.toBe(client); + expect(startMock).toHaveBeenCalledTimes(1); + + rejectFirstStart?.(new Error("late failure")); + await Promise.resolve(); + + const secondResolve = resolveSharedMatrixClient({ + auth: makeAuth("late-failure"), + }); + await vi.advanceTimersByTimeAsync(2000); + await expect(secondResolve).resolves.toBe(client); + expect(startMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/extensions/matrix/src/matrix/client/shared.ts b/extensions/matrix/src/matrix/client/shared.ts index 3144b34d326..29cac45d6b4 100644 --- a/extensions/matrix/src/matrix/client/shared.ts +++ b/extensions/matrix/src/matrix/client/shared.ts @@ -92,18 +92,19 @@ async function ensureSharedClientStarted(params: { // resolveSharedMatrixClient() calls know to retry. const startPromiseInner = client.start(); let settled = false; + let startError: unknown = undefined; startPromiseInner.catch((err: unknown) => { settled = true; + startError = err; params.state.started = false; LogService.error("MatrixClientLite", "client.start() error:", err); }); // Give the sync loop a moment to initialize before marking ready - await new Promise(resolve => setTimeout(resolve, 2000)); + await new Promise((resolve) => setTimeout(resolve, 2000)); if (settled) { - throw new Error("Matrix client.start() failed during initialization"); + throw startError; } params.state.started = true; - })(); sharedClientStartPromises.set(key, startPromise); try { diff --git a/extensions/matrix/src/matrix/monitor/direct.test.ts b/extensions/matrix/src/matrix/monitor/direct.test.ts new file mode 100644 index 00000000000..3c5ee878065 --- /dev/null +++ b/extensions/matrix/src/matrix/monitor/direct.test.ts @@ -0,0 +1,63 @@ +import type { MatrixClient } from "@vector-im/matrix-bot-sdk"; +import { describe, expect, it, vi } from "vitest"; +import { createDirectRoomTracker } from "./direct.js"; + +function createMockClient(params: { + isDm?: boolean; + senderDirect?: boolean; + selfDirect?: boolean; + members?: string[]; +}) { + const members = params.members ?? ["@alice:example.org", "@bot:example.org"]; + return { + dms: { + update: vi.fn().mockResolvedValue(undefined), + isDm: vi.fn().mockReturnValue(params.isDm === true), + }, + getUserId: vi.fn().mockResolvedValue("@bot:example.org"), + getJoinedRoomMembers: vi.fn().mockResolvedValue(members), + getRoomStateEvent: vi + .fn() + .mockImplementation(async (_roomId: string, _event: string, stateKey: string) => { + if (stateKey === "@alice:example.org") { + return { is_direct: params.senderDirect === true }; + } + if (stateKey === "@bot:example.org") { + return { is_direct: params.selfDirect === true }; + } + return {}; + }), + } as unknown as MatrixClient; +} + +describe("createDirectRoomTracker", () => { + it("treats m.direct rooms as DMs", async () => { + const tracker = createDirectRoomTracker(createMockClient({ isDm: true })); + await expect( + tracker.isDirectMessage({ + roomId: "!room:example.org", + senderId: "@alice:example.org", + }), + ).resolves.toBe(true); + }); + + it("does not classify 2-member rooms as DMs without direct flags", async () => { + const tracker = createDirectRoomTracker(createMockClient({ isDm: false })); + await expect( + tracker.isDirectMessage({ + roomId: "!room:example.org", + senderId: "@alice:example.org", + }), + ).resolves.toBe(false); + }); + + it("uses is_direct member flags when present", async () => { + const tracker = createDirectRoomTracker(createMockClient({ senderDirect: true })); + await expect( + tracker.isDirectMessage({ + roomId: "!room:example.org", + senderId: "@alice:example.org", + }), + ).resolves.toBe(true); + }); +}); diff --git a/extensions/matrix/src/matrix/monitor/events.test.ts b/extensions/matrix/src/matrix/monitor/events.test.ts index 3754cfd178e..f0fa9eba8e9 100644 --- a/extensions/matrix/src/matrix/monitor/events.test.ts +++ b/extensions/matrix/src/matrix/monitor/events.test.ts @@ -138,4 +138,38 @@ describe("registerMatrixMonitorEvents", () => { expect(getUserId).toHaveBeenCalledTimes(1); expect(sendReadReceiptMatrixMock).not.toHaveBeenCalled(); }); + + it("skips duplicate listener registration for the same client", () => { + const handlers = new Map void>(); + const onMock = vi.fn((event: string, handler: (...args: unknown[]) => void) => { + handlers.set(event, handler); + }); + const client = { + on: onMock, + getUserId: vi.fn().mockResolvedValue("@bot:example.org"), + crypto: undefined, + } as unknown as MatrixClient; + const params = { + client, + auth: { encryption: false } as MatrixAuth, + logVerboseMessage: vi.fn(), + warnedEncryptedRooms: new Set(), + warnedCryptoMissingRooms: new Set(), + logger: { warn: vi.fn() } as unknown as RuntimeLogger, + formatNativeDependencyHint: (() => + "") as PluginRuntime["system"]["formatNativeDependencyHint"], + onRoomMessage: vi.fn(), + }; + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + registerMatrixMonitorEvents(params); + const initialCallCount = onMock.mock.calls.length; + registerMatrixMonitorEvents(params); + + expect(onMock).toHaveBeenCalledTimes(initialCallCount); + expect(errorSpy).toHaveBeenCalledWith( + "[matrix] skipping duplicate listener registration for client", + ); + errorSpy.mockRestore(); + }); }); diff --git a/extensions/matrix/src/matrix/monitor/index.test.ts b/extensions/matrix/src/matrix/monitor/index.test.ts new file mode 100644 index 00000000000..89ae5188e9c --- /dev/null +++ b/extensions/matrix/src/matrix/monitor/index.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from "vitest"; +import { DEFAULT_STARTUP_GRACE_MS, isConfiguredMatrixRoomEntry } from "./index.js"; + +describe("monitorMatrixProvider helpers", () => { + it("treats !-prefixed room IDs as configured room entries", () => { + expect(isConfiguredMatrixRoomEntry("!abc123")).toBe(true); + expect(isConfiguredMatrixRoomEntry("!RoomMixedCase")).toBe(true); + }); + + it("requires a homeserver suffix for # aliases", () => { + expect(isConfiguredMatrixRoomEntry("#alias:example.org")).toBe(true); + expect(isConfiguredMatrixRoomEntry("#alias")).toBe(false); + }); + + it("uses a non-zero startup grace window", () => { + expect(DEFAULT_STARTUP_GRACE_MS).toBe(5000); + }); +}); diff --git a/extensions/matrix/src/matrix/monitor/index.ts b/extensions/matrix/src/matrix/monitor/index.ts index f282cf798a3..a2e118a0ace 100644 --- a/extensions/matrix/src/matrix/monitor/index.ts +++ b/extensions/matrix/src/matrix/monitor/index.ts @@ -36,6 +36,11 @@ export type MonitorMatrixOpts = { }; const DEFAULT_MEDIA_MAX_MB = 20; +export const DEFAULT_STARTUP_GRACE_MS = 5000; + +export function isConfiguredMatrixRoomEntry(entry: string): boolean { + return entry.startsWith("!") || (entry.startsWith("#") && entry.includes(":")); +} export async function monitorMatrixProvider(opts: MonitorMatrixOpts = {}): Promise { if (isBunRuntime()) { @@ -153,7 +158,7 @@ export async function monitorMatrixProvider(opts: MonitorMatrixOpts = {}): Promi continue; } const cleaned = normalizeRoomEntry(trimmed); - if (cleaned.startsWith("!") || (cleaned.startsWith("#") && cleaned.includes(":"))) { + if (isConfiguredMatrixRoomEntry(cleaned)) { if (!nextRooms[cleaned]) { nextRooms[cleaned] = roomConfig; } @@ -268,7 +273,7 @@ export async function monitorMatrixProvider(opts: MonitorMatrixOpts = {}): Promi const mediaMaxMb = opts.mediaMaxMb ?? accountConfig.mediaMaxMb ?? DEFAULT_MEDIA_MAX_MB; const mediaMaxBytes = Math.max(1, mediaMaxMb) * 1024 * 1024; const startupMs = Date.now(); - const startupGraceMs = 5000; // 5s grace for slow homeservers (e.g. Conduit filter M_NOT_FOUND retry) + const startupGraceMs = DEFAULT_STARTUP_GRACE_MS; const directTracker = createDirectRoomTracker(client, { log: logVerboseMessage }); registerMatrixAutoJoin({ client, cfg, runtime }); const warnedEncryptedRooms = new Set();