From ee0568c62ededdded9f96f266ed31a7ddd6f63e9 Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Thu, 12 Mar 2026 10:01:38 +0000 Subject: [PATCH] Matrix: tighten fallback resolution and ACP lookup --- .../matrix/src/matrix/client/storage.test.ts | 79 ++++++++++++++++++- .../matrix/src/matrix/client/storage.ts | 27 +++++++ .../matrix/src/matrix/config-update.test.ts | 27 +++++++ extensions/matrix/src/matrix/config-update.ts | 13 ++- .../src/matrix/monitor/auto-join.test.ts | 25 ++++++ .../matrix/src/matrix/monitor/auto-join.ts | 6 +- src/infra/outbound/message-action-params.ts | 17 +--- .../message-action-runner.threading.test.ts | 6 +- src/plugin-sdk/matrix.ts | 4 + 9 files changed, 183 insertions(+), 21 deletions(-) diff --git a/extensions/matrix/src/matrix/client/storage.test.ts b/extensions/matrix/src/matrix/client/storage.test.ts index 146aad14d87..6019ce0275c 100644 --- a/extensions/matrix/src/matrix/client/storage.test.ts +++ b/extensions/matrix/src/matrix/client/storage.test.ts @@ -30,10 +30,19 @@ describe("matrix client storage paths", () => { } }); - function setupStateDir(): string { + function setupStateDir( + cfg: Record = { + channels: { + matrix: {}, + }, + }, + ): string { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-matrix-storage-")); tempDirs.push(dir); setMatrixRuntime({ + config: { + loadConfig: () => cfg, + }, logging: { getChildLogger: () => ({ info: () => {}, @@ -157,6 +166,74 @@ describe("matrix client storage paths", () => { expect(fs.existsSync(path.join(legacyRoot, "crypto"))).toBe(true); }); + it("refuses fallback migration when multiple Matrix accounts need explicit selection", async () => { + const stateDir = setupStateDir({ + channels: { + matrix: { + accounts: { + ops: {}, + work: {}, + }, + }, + }, + }); + const storagePaths = resolveMatrixStoragePaths({ + homeserver: "https://matrix.example.org", + userId: "@bot:example.org", + accessToken: "secret-token", + accountId: "ops", + env: {}, + }); + const legacyRoot = path.join(stateDir, "matrix"); + fs.mkdirSync(path.join(legacyRoot, "crypto"), { recursive: true }); + fs.writeFileSync(path.join(legacyRoot, "bot-storage.json"), '{"legacy":true}'); + + await expect( + maybeMigrateLegacyStorage({ + storagePaths, + env: {}, + }), + ).rejects.toThrow(/defaultAccount is not set/i); + expect(maybeCreateMatrixMigrationSnapshotMock).not.toHaveBeenCalled(); + expect(fs.existsSync(path.join(legacyRoot, "bot-storage.json"))).toBe(true); + }); + + it("refuses fallback migration for a non-selected Matrix account", async () => { + const stateDir = setupStateDir({ + channels: { + matrix: { + defaultAccount: "ops", + homeserver: "https://matrix.default.example.org", + accessToken: "default-token", + accounts: { + ops: { + homeserver: "https://matrix.ops.example.org", + accessToken: "ops-token", + }, + }, + }, + }, + }); + const storagePaths = resolveMatrixStoragePaths({ + homeserver: "https://matrix.default.example.org", + userId: "@default:example.org", + accessToken: "default-token", + env: {}, + }); + const legacyRoot = path.join(stateDir, "matrix"); + fs.mkdirSync(path.join(legacyRoot, "crypto"), { recursive: true }); + fs.writeFileSync(path.join(legacyRoot, "bot-storage.json"), '{"legacy":true}'); + + await expect( + maybeMigrateLegacyStorage({ + storagePaths, + env: {}, + }), + ).rejects.toThrow(/targets account "ops"/i); + expect(maybeCreateMatrixMigrationSnapshotMock).not.toHaveBeenCalled(); + expect(fs.existsSync(path.join(legacyRoot, "bot-storage.json"))).toBe(true); + }); + it("reuses an existing token-hash storage root after the access token changes", () => { const stateDir = setupStateDir(); const oldStoragePaths = resolveMatrixStoragePaths({ diff --git a/extensions/matrix/src/matrix/client/storage.ts b/extensions/matrix/src/matrix/client/storage.ts index d5768aa3c6a..311f86c17d0 100644 --- a/extensions/matrix/src/matrix/client/storage.ts +++ b/extensions/matrix/src/matrix/client/storage.ts @@ -3,7 +3,10 @@ import os from "node:os"; import path from "node:path"; import { maybeCreateMatrixMigrationSnapshot, + normalizeAccountId, + requiresExplicitMatrixDefaultAccount, resolveMatrixAccountStorageRoot, + resolveMatrixDefaultOrOnlyAccountId, resolveMatrixLegacyFlatStoragePaths, } from "openclaw/plugin-sdk/matrix"; import { getMatrixRuntime } from "../../runtime.js"; @@ -31,6 +34,26 @@ function resolveLegacyStoragePaths(env: NodeJS.ProcessEnv = process.env): { return { storagePath: legacy.storagePath, cryptoPath: legacy.cryptoPath }; } +function assertLegacyMigrationAccountSelection(params: { accountKey: string }): void { + const cfg = getMatrixRuntime().config.loadConfig(); + if (!cfg.channels?.matrix || typeof cfg.channels.matrix !== "object") { + return; + } + if (requiresExplicitMatrixDefaultAccount(cfg)) { + throw new Error( + "Legacy Matrix client storage cannot be migrated automatically because multiple Matrix accounts are configured and channels.matrix.defaultAccount is not set.", + ); + } + + const selectedAccountId = normalizeAccountId(resolveMatrixDefaultOrOnlyAccountId(cfg)); + const currentAccountId = normalizeAccountId(params.accountKey); + if (selectedAccountId !== currentAccountId) { + throw new Error( + `Legacy Matrix client storage targets account "${selectedAccountId}", but the current client is starting account "${currentAccountId}". Start the selected account first so flat legacy storage is not migrated into the wrong account directory.`, + ); + } +} + function scoreStorageRoot(rootDir: string): number { let score = 0; if (fs.existsSync(path.join(rootDir, "bot-storage.json"))) { @@ -175,6 +198,10 @@ export async function maybeMigrateLegacyStorage(params: { return; } + assertLegacyMigrationAccountSelection({ + accountKey: params.storagePaths.accountKey, + }); + const logger = getMatrixRuntime().logging.getChildLogger({ module: "matrix-storage" }); await maybeCreateMatrixMigrationSnapshot({ trigger: "matrix-client-fallback", diff --git a/extensions/matrix/src/matrix/config-update.test.ts b/extensions/matrix/src/matrix/config-update.test.ts index f2a6e1941b3..a5428e833e2 100644 --- a/extensions/matrix/src/matrix/config-update.test.ts +++ b/extensions/matrix/src/matrix/config-update.test.ts @@ -121,4 +121,31 @@ describe("updateMatrixAccountConfig", () => { }); expect(updated.channels?.["matrix"]?.accounts?.ops?.rooms).toBeUndefined(); }); + + it("reuses and canonicalizes non-normalized account entries when updating", () => { + const cfg = { + channels: { + matrix: { + accounts: { + Ops: { + homeserver: "https://matrix.ops.example.org", + accessToken: "ops-token", + }, + }, + }, + }, + } as CoreConfig; + + const updated = updateMatrixAccountConfig(cfg, "ops", { + deviceName: "Ops Bot", + }); + + expect(updated.channels?.["matrix"]?.accounts?.Ops).toBeUndefined(); + expect(updated.channels?.["matrix"]?.accounts?.ops).toMatchObject({ + homeserver: "https://matrix.ops.example.org", + accessToken: "ops-token", + deviceName: "Ops Bot", + enabled: true, + }); + }); }); diff --git a/extensions/matrix/src/matrix/config-update.ts b/extensions/matrix/src/matrix/config-update.ts index 27035bfbe41..452f9e38722 100644 --- a/extensions/matrix/src/matrix/config-update.ts +++ b/extensions/matrix/src/matrix/config-update.ts @@ -1,6 +1,7 @@ import { DEFAULT_ACCOUNT_ID } from "openclaw/plugin-sdk/account-id"; import { normalizeAccountId } from "openclaw/plugin-sdk/matrix"; import type { CoreConfig, MatrixConfig } from "../types.js"; +import { findMatrixAccountConfig } from "./account-config.js"; export type MatrixAccountPatch = { name?: string | null; @@ -113,7 +114,7 @@ export function updateMatrixAccountConfig( ): CoreConfig { const matrix = cfg.channels?.matrix ?? {}; const normalizedAccountId = normalizeAccountId(accountId); - const existingAccount = (matrix.accounts?.[normalizedAccountId] ?? + const existingAccount = (findMatrixAccountConfig(cfg, normalizedAccountId) ?? (normalizedAccountId === DEFAULT_ACCOUNT_ID ? matrix : {})) as MatrixConfig; const nextAccount: Record = { ...existingAccount }; @@ -191,6 +192,14 @@ export function updateMatrixAccountConfig( } } + const nextAccounts = Object.fromEntries( + Object.entries(matrix.accounts ?? {}).filter( + ([rawAccountId]) => + rawAccountId === normalizedAccountId || + normalizeAccountId(rawAccountId) !== normalizedAccountId, + ), + ); + if (shouldStoreMatrixAccountAtTopLevel(cfg, normalizedAccountId)) { const { accounts: _ignoredAccounts, defaultAccount, ...baseMatrix } = matrix; return { @@ -215,7 +224,7 @@ export function updateMatrixAccountConfig( ...matrix, enabled: true, accounts: { - ...matrix.accounts, + ...nextAccounts, [normalizedAccountId]: nextAccount as MatrixConfig, }, }, diff --git a/extensions/matrix/src/matrix/monitor/auto-join.test.ts b/extensions/matrix/src/matrix/monitor/auto-join.test.ts index ebe36144a5e..f3a27db8598 100644 --- a/extensions/matrix/src/matrix/monitor/auto-join.test.ts +++ b/extensions/matrix/src/matrix/monitor/auto-join.test.ts @@ -122,6 +122,31 @@ describe("registerMatrixAutoJoin", () => { expect(joinRoom).toHaveBeenCalledWith("!room:example.org"); }); + it("retries alias resolution after an unresolved lookup", async () => { + const { client, getInviteHandler, joinRoom, resolveRoom } = createClientStub(); + resolveRoom.mockResolvedValueOnce(null).mockResolvedValueOnce("!room:example.org"); + + registerMatrixAutoJoin({ + client, + accountConfig: { + autoJoin: "allowlist", + autoJoinAllowlist: ["#allowed:example.org"], + }, + runtime: { + log: vi.fn(), + error: vi.fn(), + } as unknown as import("openclaw/plugin-sdk/matrix").RuntimeEnv, + }); + + const inviteHandler = getInviteHandler(); + expect(inviteHandler).toBeTruthy(); + await inviteHandler!("!room:example.org", {}); + await inviteHandler!("!room:example.org", {}); + + expect(resolveRoom).toHaveBeenCalledTimes(2); + expect(joinRoom).toHaveBeenCalledWith("!room:example.org"); + }); + it("does not trust room-provided alias claims for allowlist joins", async () => { const { client, getInviteHandler, joinRoom, resolveRoom } = createClientStub(); resolveRoom.mockResolvedValue("!different-room:example.org"); diff --git a/extensions/matrix/src/matrix/monitor/auto-join.ts b/extensions/matrix/src/matrix/monitor/auto-join.ts index 4357975ad5d..f92b568e2bd 100644 --- a/extensions/matrix/src/matrix/monitor/auto-join.ts +++ b/extensions/matrix/src/matrix/monitor/auto-join.ts @@ -23,7 +23,7 @@ export function registerMatrixAutoJoin(params: { const autoJoinAllowlist = new Set(rawAllowlist); const allowedRoomIds = new Set(rawAllowlist.filter((entry) => entry.startsWith("!"))); const allowedAliases = rawAllowlist.filter((entry) => entry.startsWith("#")); - const resolvedAliasRoomIds = new Map(); + const resolvedAliasRoomIds = new Map(); if (autoJoin === "off") { return; @@ -40,7 +40,9 @@ export function registerMatrixAutoJoin(params: { return resolvedAliasRoomIds.get(alias) ?? null; } const resolved = await params.client.resolveRoom(alias); - resolvedAliasRoomIds.set(alias, resolved); + if (resolved) { + resolvedAliasRoomIds.set(alias, resolved); + } return resolved; }; diff --git a/src/infra/outbound/message-action-params.ts b/src/infra/outbound/message-action-params.ts index bdcdd89af7c..291a2d00381 100644 --- a/src/infra/outbound/message-action-params.ts +++ b/src/infra/outbound/message-action-params.ts @@ -83,11 +83,6 @@ function normalizeMatrixThreadTarget(raw: string): string | undefined { return normalized || undefined; } -function normalizeMatrixDirectUserTarget(raw: string): string | undefined { - const normalized = normalizeMatrixThreadTarget(raw); - return normalized?.startsWith("@") ? normalized : undefined; -} - export function resolveMatrixAutoThreadId(params: { to: string; toolContext?: ChannelThreadingToolContext; @@ -101,15 +96,11 @@ export function resolveMatrixAutoThreadId(params: { if (!target || !currentChannel) { return undefined; } + // Matrix user:@ targets resolve to a DM room at send time, which can differ + // from the current room after DM recreation or stale m.direct ordering. + // Only auto-thread when the explicit room target already matches. if (target.toLowerCase() !== currentChannel.toLowerCase()) { - const directTarget = normalizeMatrixDirectUserTarget(params.to); - const currentDirectUserId = normalizeMatrixDirectUserTarget(context.currentDirectUserId ?? ""); - if (!directTarget || !currentDirectUserId) { - return undefined; - } - if (directTarget.toLowerCase() !== currentDirectUserId.toLowerCase()) { - return undefined; - } + return undefined; } return context.currentThreadTs; } diff --git a/src/infra/outbound/message-action-runner.threading.test.ts b/src/infra/outbound/message-action-runner.threading.test.ts index e8639029e0f..6a3c6fcc599 100644 --- a/src/infra/outbound/message-action-runner.threading.test.ts +++ b/src/infra/outbound/message-action-runner.threading.test.ts @@ -309,7 +309,7 @@ describe("runMessageAction threading auto-injection", () => { expect(call?.ctx?.params?.threadId).toBe("$explicit"); }); - it("injects threadId for matching Matrix dm user target", async () => { + it("skips threadId for Matrix dm user targets until the resolved room matches", async () => { mockHandledSendAction(); const call = await runThreadingAction({ @@ -322,8 +322,8 @@ describe("runMessageAction threading auto-injection", () => { toolContext: defaultMatrixDmToolContext, }); - expect(call?.threadId).toBe("$thread"); - expect(call?.ctx?.params?.threadId).toBe("$thread"); + expect(call?.threadId).toBeUndefined(); + expect(call?.ctx?.params?.threadId).toBeUndefined(); }); it("skips threadId for different Matrix dm user target", async () => { diff --git a/src/plugin-sdk/matrix.ts b/src/plugin-sdk/matrix.ts index b727f388e5e..cd9a08ca3f5 100644 --- a/src/plugin-sdk/matrix.ts +++ b/src/plugin-sdk/matrix.ts @@ -115,6 +115,10 @@ export { resolveMatrixLegacyFlatStoragePaths, sanitizeMatrixPathSegment, } from "../infra/matrix-storage-paths.js"; +export { + requiresExplicitMatrixDefaultAccount, + resolveMatrixDefaultOrOnlyAccountId, +} from "../infra/matrix-account-selection.js"; export { hasActionableMatrixMigration, hasPendingMatrixMigration,