From f00db91590ccb0ca42820d676a995bc61dcf5c96 Mon Sep 17 00:00:00 2001 From: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> Date: Sat, 14 Mar 2026 21:08:32 -0500 Subject: [PATCH] fix(plugins): prefer explicit installs over bundled duplicates (#46722) * fix(plugins): prefer explicit installs over bundled duplicates * test(feishu): mock structured card sends in outbound tests * fix(plugins): align duplicate diagnostics with loader precedence --- CHANGELOG.md | 1 + extensions/feishu/src/outbound.test.ts | 9 ++- src/plugins/loader.test.ts | 48 +++++++++++++++ src/plugins/loader.ts | 83 +++++++++++++++++++++++++- src/plugins/manifest-registry.test.ts | 70 ++++++++++++++++++++++ src/plugins/manifest-registry.ts | 65 +++++++++++++++++++- 6 files changed, 269 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f901df8e27..de21281fbde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Docs: https://docs.openclaw.ai - Control UI/chat sessions: show human-readable labels in the grouped session dropdown again, keep unique scoped fallbacks when metadata is missing, and disambiguate duplicate labels only when needed. (#45130) thanks @luzhidong. - Configure/startup: move outbound send-deps resolution into a lightweight helper so `openclaw configure` no longer stalls after the banner while eagerly loading channel plugins. (#46301) thanks @scoootscooob. - Zalo Personal/group gating: stop reapplying `dmPolicy.allowFrom` as a sender gate for already-allowlisted groups when `groupAllowFrom` is unset, so any member of an allowed group can trigger replies while DMs stay restricted. (#40146) +- Plugins/install precedence: keep bundled plugins ahead of auto-discovered globals by default, but let an explicitly installed plugin record win its own duplicate-id tie so installed channel plugins load from `~/.openclaw/extensions` after `openclaw plugins install`. ### Fixes diff --git a/extensions/feishu/src/outbound.test.ts b/extensions/feishu/src/outbound.test.ts index 39b7c1e4a63..64420f0a573 100644 --- a/extensions/feishu/src/outbound.test.ts +++ b/extensions/feishu/src/outbound.test.ts @@ -6,6 +6,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const sendMediaFeishuMock = vi.hoisted(() => vi.fn()); const sendMessageFeishuMock = vi.hoisted(() => vi.fn()); const sendMarkdownCardFeishuMock = vi.hoisted(() => vi.fn()); +const sendStructuredCardFeishuMock = vi.hoisted(() => vi.fn()); vi.mock("./media.js", () => ({ sendMediaFeishu: sendMediaFeishuMock, @@ -14,6 +15,7 @@ vi.mock("./media.js", () => ({ vi.mock("./send.js", () => ({ sendMessageFeishu: sendMessageFeishuMock, sendMarkdownCardFeishu: sendMarkdownCardFeishuMock, + sendStructuredCardFeishu: sendStructuredCardFeishuMock, })); vi.mock("./runtime.js", () => ({ @@ -33,6 +35,7 @@ function resetOutboundMocks() { vi.clearAllMocks(); sendMessageFeishuMock.mockResolvedValue({ messageId: "text_msg" }); sendMarkdownCardFeishuMock.mockResolvedValue({ messageId: "card_msg" }); + sendStructuredCardFeishuMock.mockResolvedValue({ messageId: "card_msg" }); sendMediaFeishuMock.mockResolvedValue({ messageId: "media_msg" }); } @@ -132,7 +135,7 @@ describe("feishuOutbound.sendText local-image auto-convert", () => { accountId: "main", }); - expect(sendMarkdownCardFeishuMock).toHaveBeenCalledWith( + expect(sendStructuredCardFeishuMock).toHaveBeenCalledWith( expect.objectContaining({ to: "chat_1", text: "| a | b |\n| - | - |", @@ -207,7 +210,7 @@ describe("feishuOutbound.sendText replyToId forwarding", () => { ); }); - it("forwards replyToId to sendMarkdownCardFeishu when renderMode=card", async () => { + it("forwards replyToId to sendStructuredCardFeishu when renderMode=card", async () => { await sendText({ cfg: { channels: { @@ -222,7 +225,7 @@ describe("feishuOutbound.sendText replyToId forwarding", () => { accountId: "main", }); - expect(sendMarkdownCardFeishuMock).toHaveBeenCalledWith( + expect(sendStructuredCardFeishuMock).toHaveBeenCalledWith( expect.objectContaining({ replyToMessageId: "om_reply_target", }), diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index 6be4992821c..4771d98aa31 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -1543,6 +1543,54 @@ describe("loadOpenClawPlugins", () => { }); }); + it("prefers an explicitly installed global plugin over a bundled duplicate", () => { + const bundledDir = makeTempDir(); + writePlugin({ + id: "zalouser", + body: `module.exports = { id: "zalouser", register() {} };`, + dir: bundledDir, + filename: "index.cjs", + }); + process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = bundledDir; + + const stateDir = makeTempDir(); + withEnv({ OPENCLAW_STATE_DIR: stateDir, CLAWDBOT_STATE_DIR: undefined }, () => { + const globalDir = path.join(stateDir, "extensions", "zalouser"); + mkdirSafe(globalDir); + writePlugin({ + id: "zalouser", + body: `module.exports = { id: "zalouser", register() {} };`, + dir: globalDir, + filename: "index.cjs", + }); + + const registry = loadOpenClawPlugins({ + cache: false, + config: { + plugins: { + allow: ["zalouser"], + installs: { + zalouser: { + source: "npm", + installPath: globalDir, + }, + }, + entries: { + zalouser: { enabled: true }, + }, + }, + }, + }); + + const entries = registry.plugins.filter((entry) => entry.id === "zalouser"); + const loaded = entries.find((entry) => entry.status === "loaded"); + const overridden = entries.find((entry) => entry.status === "disabled"); + expect(loaded?.origin).toBe("global"); + expect(overridden?.origin).toBe("bundled"); + expect(overridden?.error).toContain("overridden by global plugin"); + }); + }); + it("warns when plugins.allow is empty and non-bundled plugins are discoverable", () => { useNoBundledPlugins(); const plugin = writePlugin({ diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index 18c0b4bfee2..698918964f9 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -453,6 +453,78 @@ function isTrackedByProvenance(params: { return matchesPathMatcher(params.index.loadPathMatcher, sourcePath); } +function matchesExplicitInstallRule(params: { + pluginId: string; + source: string; + index: PluginProvenanceIndex; + env: NodeJS.ProcessEnv; +}): boolean { + const sourcePath = resolveUserPath(params.source, params.env); + const installRule = params.index.installRules.get(params.pluginId); + if (!installRule || installRule.trackedWithoutPaths) { + return false; + } + return matchesPathMatcher(installRule.matcher, sourcePath); +} + +function resolveCandidateDuplicateRank(params: { + candidate: ReturnType["candidates"][number]; + manifestByRoot: Map["plugins"][number]>; + provenance: PluginProvenanceIndex; + env: NodeJS.ProcessEnv; +}): number { + const manifestRecord = params.manifestByRoot.get(params.candidate.rootDir); + const pluginId = manifestRecord?.id; + const isExplicitInstall = + params.candidate.origin === "global" && + pluginId !== undefined && + matchesExplicitInstallRule({ + pluginId, + source: params.candidate.source, + index: params.provenance, + env: params.env, + }); + + switch (params.candidate.origin) { + case "config": + return 0; + case "workspace": + return 1; + case "global": + return isExplicitInstall ? 2 : 4; + case "bundled": + return 3; + } +} + +function compareDuplicateCandidateOrder(params: { + left: ReturnType["candidates"][number]; + right: ReturnType["candidates"][number]; + manifestByRoot: Map["plugins"][number]>; + provenance: PluginProvenanceIndex; + env: NodeJS.ProcessEnv; +}): number { + const leftPluginId = params.manifestByRoot.get(params.left.rootDir)?.id; + const rightPluginId = params.manifestByRoot.get(params.right.rootDir)?.id; + if (!leftPluginId || leftPluginId !== rightPluginId) { + return 0; + } + return ( + resolveCandidateDuplicateRank({ + candidate: params.left, + manifestByRoot: params.manifestByRoot, + provenance: params.provenance, + env: params.env, + }) - + resolveCandidateDuplicateRank({ + candidate: params.right, + manifestByRoot: params.manifestByRoot, + provenance: params.provenance, + env: params.env, + }) + ); +} + function warnWhenAllowlistIsOpen(params: { logger: PluginLogger; pluginsEnabled: boolean; @@ -644,13 +716,22 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi const manifestByRoot = new Map( manifestRegistry.plugins.map((record) => [record.rootDir, record]), ); + const orderedCandidates = [...discovery.candidates].toSorted((left, right) => { + return compareDuplicateCandidateOrder({ + left, + right, + manifestByRoot, + provenance, + env, + }); + }); const seenIds = new Map(); const memorySlot = normalized.slots.memory; let selectedMemoryPluginId: string | null = null; let memorySlotMatched = false; - for (const candidate of discovery.candidates) { + for (const candidate of orderedCandidates) { const manifestRecord = manifestByRoot.get(candidate.rootDir); if (!manifestRecord) { continue; diff --git a/src/plugins/manifest-registry.test.ts b/src/plugins/manifest-registry.test.ts index 3675dd56f5c..bbdc8020d6e 100644 --- a/src/plugins/manifest-registry.test.ts +++ b/src/plugins/manifest-registry.test.ts @@ -155,6 +155,76 @@ describe("loadPluginManifestRegistry", () => { expect(countDuplicateWarnings(loadRegistry(candidates))).toBe(1); }); + it("reports explicit installed globals as the effective duplicate winner", () => { + const bundledDir = makeTempDir(); + const globalDir = makeTempDir(); + const manifest = { id: "zalouser", configSchema: { type: "object" } }; + writeManifest(bundledDir, manifest); + writeManifest(globalDir, manifest); + + const registry = loadPluginManifestRegistry({ + cache: false, + config: { + plugins: { + installs: { + zalouser: { + source: "npm", + installPath: globalDir, + }, + }, + }, + }, + candidates: [ + createPluginCandidate({ + idHint: "zalouser", + rootDir: bundledDir, + origin: "bundled", + }), + createPluginCandidate({ + idHint: "zalouser", + rootDir: globalDir, + origin: "global", + }), + ], + }); + + expect( + registry.diagnostics.some((diag) => + diag.message.includes("bundled plugin will be overridden by global plugin"), + ), + ).toBe(true); + }); + + it("reports bundled plugins as the duplicate winner for auto-discovered globals", () => { + const bundledDir = makeTempDir(); + const globalDir = makeTempDir(); + const manifest = { id: "feishu", configSchema: { type: "object" } }; + writeManifest(bundledDir, manifest); + writeManifest(globalDir, manifest); + + const registry = loadPluginManifestRegistry({ + cache: false, + candidates: [ + createPluginCandidate({ + idHint: "feishu", + rootDir: bundledDir, + origin: "bundled", + }), + createPluginCandidate({ + idHint: "feishu", + rootDir: globalDir, + origin: "global", + }), + ], + }); + + expect( + registry.diagnostics.some((diag) => + diag.message.includes("global plugin will be overridden by bundled plugin"), + ), + ).toBe(true); + }); + it("suppresses duplicate warning when candidates share the same physical directory via symlink", () => { const realDir = makeTempDir(); const manifest = { id: "feishu", configSchema: { type: "object" } }; diff --git a/src/plugins/manifest-registry.ts b/src/plugins/manifest-registry.ts index 7b6a0ca4bfb..79fb3facf8e 100644 --- a/src/plugins/manifest-registry.ts +++ b/src/plugins/manifest-registry.ts @@ -1,9 +1,10 @@ import fs from "node:fs"; import type { OpenClawConfig } from "../config/config.js"; +import { resolveUserPath } from "../utils.js"; import { normalizePluginsConfig, type NormalizedPluginsConfig } from "./config-state.js"; import { discoverOpenClawPlugins, type PluginCandidate } from "./discovery.js"; import { loadPluginManifest, type PluginManifest } from "./manifest.js"; -import { safeRealpathSync } from "./path-safety.js"; +import { isPathInside, safeRealpathSync } from "./path-safety.js"; import { resolvePluginCacheInputs } from "./roots.js"; import type { PluginConfigUiHint, PluginDiagnostic, PluginKind, PluginOrigin } from "./types.js"; @@ -12,7 +13,7 @@ type SeenIdEntry = { recordIndex: number; }; -// Precedence: config > workspace > global > bundled +// Precedence: config > workspace > explicit-install global > bundled > auto-discovered global const PLUGIN_ORIGIN_RANK: Readonly> = { config: 0, workspace: 1, @@ -135,6 +136,50 @@ function buildRecord(params: { }; } +function matchesInstalledPluginRecord(params: { + pluginId: string; + candidate: PluginCandidate; + config?: OpenClawConfig; + env: NodeJS.ProcessEnv; +}): boolean { + if (params.candidate.origin !== "global") { + return false; + } + const record = params.config?.plugins?.installs?.[params.pluginId]; + if (!record) { + return false; + } + const candidateSource = resolveUserPath(params.candidate.source, params.env); + const trackedPaths = [record.installPath, record.sourcePath] + .filter((entry): entry is string => typeof entry === "string" && entry.trim().length > 0) + .map((entry) => resolveUserPath(entry, params.env)); + if (trackedPaths.length === 0) { + return false; + } + return trackedPaths.some((trackedPath) => { + return candidateSource === trackedPath || isPathInside(trackedPath, candidateSource); + }); +} + +function resolveDuplicatePrecedenceRank(params: { + pluginId: string; + candidate: PluginCandidate; + config?: OpenClawConfig; + env: NodeJS.ProcessEnv; +}): number { + if (params.candidate.origin === "global") { + return matchesInstalledPluginRecord({ + pluginId: params.pluginId, + candidate: params.candidate, + config: params.config, + env: params.env, + }) + ? 2 + : 4; + } + return PLUGIN_ORIGIN_RANK[params.candidate.origin]; +} + export function loadPluginManifestRegistry(params: { config?: OpenClawConfig; workspaceDir?: string; @@ -237,7 +282,21 @@ export function loadPluginManifestRegistry(params: { level: "warn", pluginId: manifest.id, source: candidate.source, - message: `duplicate plugin id detected; later plugin may be overridden (${candidate.source})`, + message: + resolveDuplicatePrecedenceRank({ + pluginId: manifest.id, + candidate, + config, + env, + }) < + resolveDuplicatePrecedenceRank({ + pluginId: manifest.id, + candidate: existing.candidate, + config, + env, + }) + ? `duplicate plugin id detected; ${existing.candidate.origin} plugin will be overridden by ${candidate.origin} plugin (${candidate.source})` + : `duplicate plugin id detected; ${candidate.origin} plugin will be overridden by ${existing.candidate.origin} plugin (${candidate.source})`, }); } else { seenIds.set(manifest.id, { candidate, recordIndex: records.length });