fix: prefer bundled channel plugins over npm duplicates (#40094)

* fix: prefer bundled channel plugins over npm duplicates

* fix: tighten bundled plugin review follow-ups

* fix: address check gate follow-ups

* docs: add changelog for bundled plugin install fix

* fix: align lifecycle test formatting with CI oxfmt
This commit is contained in:
Tak Hoffman
2026-03-08 13:00:24 -05:00
committed by GitHub
parent 6c9b49a10b
commit 74624e619d
9 changed files with 343 additions and 50 deletions

View File

@@ -13,6 +13,7 @@ Docs: https://docs.openclaw.ai
### Fixes
- Plugins/channel onboarding: prefer bundled channel plugins over duplicate npm-installed copies during onboarding and release-channel sync, preventing bundled plugins from being shadowed by npm installs with the same plugin ID. (#40092)
- macOS app/chat UI: route browser proxy through the local node browser service, preserve plain-text paste semantics, strip completed assistant trace/debug wrapper noise from transcripts, refresh permission state after returning from System Settings, and tolerate malformed cron rows in the macOS tab. (#39516) Thanks @Imhermes1.
- Mattermost replies: keep `root_id` pinned to the existing thread root when an agent replies inside a thread, while still using reply-target threading for top-level posts. (#27744) thanks @hnykda.
- Agents/failover: detect Amazon Bedrock `Too many tokens per day` quota errors as rate limits across fallback, cron retry, and memory embeddings while keeping context-window `too many tokens per request` errors out of the rate-limit lane. (#39377) Thanks @gambletan.

View File

@@ -1,6 +1,7 @@
import { describe, expect, it, vi } from "vitest";
import { PLUGIN_INSTALL_ERROR_CODE } from "../plugins/install.js";
import {
resolveBundledInstallPlanForCatalogEntry,
resolveBundledInstallPlanBeforeNpm,
resolveBundledInstallPlanForNpmFailure,
} from "./plugin-install-plan.js";
@@ -34,6 +35,53 @@ describe("plugin install plan helpers", () => {
expect(result).toBeNull();
});
it("prefers bundled catalog plugin by id before npm spec", () => {
const findBundledSource = vi
.fn()
.mockImplementation(({ kind, value }: { kind: "pluginId" | "npmSpec"; value: string }) => {
if (kind === "pluginId" && value === "voice-call") {
return {
pluginId: "voice-call",
localPath: "/tmp/extensions/voice-call",
npmSpec: "@openclaw/voice-call",
};
}
return undefined;
});
const result = resolveBundledInstallPlanForCatalogEntry({
pluginId: "voice-call",
npmSpec: "@openclaw/voice-call",
findBundledSource,
});
expect(findBundledSource).toHaveBeenCalledWith({ kind: "pluginId", value: "voice-call" });
expect(result?.bundledSource.localPath).toBe("/tmp/extensions/voice-call");
});
it("rejects npm-spec matches that resolve to a different plugin id", () => {
const findBundledSource = vi
.fn()
.mockImplementation(({ kind }: { kind: "pluginId" | "npmSpec"; value: string }) => {
if (kind === "npmSpec") {
return {
pluginId: "not-voice-call",
localPath: "/tmp/extensions/not-voice-call",
npmSpec: "@openclaw/voice-call",
};
}
return undefined;
});
const result = resolveBundledInstallPlanForCatalogEntry({
pluginId: "voice-call",
npmSpec: "@openclaw/voice-call",
findBundledSource,
});
expect(result).toBeNull();
});
it("uses npm-spec bundled fallback only for package-not-found", () => {
const findBundledSource = vi.fn().mockReturnValue({
pluginId: "voice-call",

View File

@@ -12,6 +12,36 @@ function isBareNpmPackageName(spec: string): boolean {
return /^[a-z0-9][a-z0-9-._~]*$/.test(trimmed);
}
export function resolveBundledInstallPlanForCatalogEntry(params: {
pluginId: string;
npmSpec: string;
findBundledSource: BundledLookup;
}): { bundledSource: BundledPluginSource } | null {
const pluginId = params.pluginId.trim();
const npmSpec = params.npmSpec.trim();
if (!pluginId || !npmSpec) {
return null;
}
const bundledById = params.findBundledSource({
kind: "pluginId",
value: pluginId,
});
if (bundledById?.pluginId === pluginId) {
return { bundledSource: bundledById };
}
const bundledBySpec = params.findBundledSource({
kind: "npmSpec",
value: npmSpec,
});
if (bundledBySpec?.pluginId === pluginId) {
return { bundledSource: bundledBySpec };
}
return null;
}
export function resolveBundledInstallPlanBeforeNpm(params: {
rawSpec: string;
findBundledSource: BundledLookup;

View File

@@ -1,17 +1,50 @@
import path from "node:path";
import { beforeEach, describe, expect, it, vi } from "vitest";
vi.mock("node:fs", () => ({
default: {
existsSync: vi.fn(),
},
}));
vi.mock("node:fs", async (importOriginal) => {
const actual = await importOriginal<typeof import("node:fs")>();
const existsSync = vi.fn();
return {
...actual,
existsSync,
default: {
...actual,
existsSync,
},
};
});
const installPluginFromNpmSpec = vi.fn();
vi.mock("../../plugins/install.js", () => ({
installPluginFromNpmSpec: (...args: unknown[]) => installPluginFromNpmSpec(...args),
}));
const resolveBundledPluginSources = vi.fn();
vi.mock("../../plugins/bundled-sources.js", () => ({
findBundledPluginSourceInMap: ({
bundled,
lookup,
}: {
bundled: ReadonlyMap<string, { pluginId: string; localPath: string; npmSpec?: string }>;
lookup: { kind: "pluginId" | "npmSpec"; value: string };
}) => {
const targetValue = lookup.value.trim();
if (!targetValue) {
return undefined;
}
if (lookup.kind === "pluginId") {
return bundled.get(targetValue);
}
for (const source of bundled.values()) {
if (source.npmSpec === targetValue) {
return source;
}
}
return undefined;
},
resolveBundledPluginSources: (...args: unknown[]) => resolveBundledPluginSources(...args),
}));
vi.mock("../../plugins/loader.js", () => ({
loadOpenClawPlugins: vi.fn(),
}));
@@ -41,6 +74,7 @@ const baseEntry: ChannelPluginCatalogEntry = {
beforeEach(() => {
vi.clearAllMocks();
resolveBundledPluginSources.mockReturnValue(new Map());
});
function mockRepoLocalPathExists() {
@@ -136,6 +170,45 @@ describe("ensureOnboardingPluginInstalled", () => {
expect(await runInitialValueForChannel("beta")).toBe("npm");
});
it("defaults to bundled local path on beta channel when available", async () => {
const runtime = makeRuntime();
const select = vi.fn((async <T extends string>() => "skip" as T) as WizardPrompter["select"]);
const prompter = makePrompter({ select: select as unknown as WizardPrompter["select"] });
const cfg: OpenClawConfig = { update: { channel: "beta" } };
vi.mocked(fs.existsSync).mockReturnValue(false);
resolveBundledPluginSources.mockReturnValue(
new Map([
[
"zalo",
{
pluginId: "zalo",
localPath: "/opt/openclaw/extensions/zalo",
npmSpec: "@openclaw/zalo",
},
],
]),
);
await ensureOnboardingPluginInstalled({
cfg,
entry: baseEntry,
prompter,
runtime,
});
expect(select).toHaveBeenCalledWith(
expect.objectContaining({
initialValue: "local",
options: expect.arrayContaining([
expect.objectContaining({
value: "local",
hint: "/opt/openclaw/extensions/zalo",
}),
]),
}),
);
});
it("falls back to local path after npm install failure", async () => {
const runtime = makeRuntime();
const note = vi.fn(async () => {});

View File

@@ -2,8 +2,13 @@ import fs from "node:fs";
import path from "node:path";
import { resolveAgentWorkspaceDir, resolveDefaultAgentId } from "../../agents/agent-scope.js";
import type { ChannelPluginCatalogEntry } from "../../channels/plugins/catalog.js";
import { resolveBundledInstallPlanForCatalogEntry } from "../../cli/plugin-install-plan.js";
import type { OpenClawConfig } from "../../config/config.js";
import { createSubsystemLogger } from "../../logging/subsystem.js";
import {
findBundledPluginSourceInMap,
resolveBundledPluginSources,
} from "../../plugins/bundled-sources.js";
import { enablePluginInConfig } from "../../plugins/enable.js";
import { installPluginFromNpmSpec } from "../../plugins/install.js";
import { buildNpmResolutionInstallFields, recordPluginInstall } from "../../plugins/installs.js";
@@ -107,8 +112,12 @@ function resolveInstallDefaultChoice(params: {
cfg: OpenClawConfig;
entry: ChannelPluginCatalogEntry;
localPath?: string | null;
bundledLocalPath?: string | null;
}): InstallChoice {
const { cfg, entry, localPath } = params;
const { cfg, entry, localPath, bundledLocalPath } = params;
if (bundledLocalPath) {
return "local";
}
const updateChannel = cfg.update?.channel;
if (updateChannel === "dev") {
return localPath ? "local" : "npm";
@@ -136,11 +145,20 @@ export async function ensureOnboardingPluginInstalled(params: {
const { entry, prompter, runtime, workspaceDir } = params;
let next = params.cfg;
const allowLocal = hasGitWorkspace(workspaceDir);
const localPath = resolveLocalPath(entry, workspaceDir, allowLocal);
const bundledSources = resolveBundledPluginSources({ workspaceDir });
const bundledLocalPath =
resolveBundledInstallPlanForCatalogEntry({
pluginId: entry.id,
npmSpec: entry.install.npmSpec,
findBundledSource: (lookup) =>
findBundledPluginSourceInMap({ bundled: bundledSources, lookup }),
})?.bundledSource.localPath ?? null;
const localPath = bundledLocalPath ?? resolveLocalPath(entry, workspaceDir, allowLocal);
const defaultChoice = resolveInstallDefaultChoice({
cfg: next,
entry,
localPath,
bundledLocalPath,
});
const choice = await promptInstallChoice({
entry,

View File

@@ -1,5 +1,9 @@
import { beforeEach, describe, expect, it, vi } from "vitest";
import { findBundledPluginSource, resolveBundledPluginSources } from "./bundled-sources.js";
import {
findBundledPluginSource,
findBundledPluginSourceInMap,
resolveBundledPluginSources,
} from "./bundled-sources.js";
const discoverOpenClawPluginsMock = vi.fn();
const loadPluginManifestMock = vi.fn();
@@ -124,4 +128,34 @@ describe("bundled plugin sources", () => {
expect(resolved?.localPath).toBe("/app/extensions/diffs");
expect(missing).toBeUndefined();
});
it("reuses a pre-resolved bundled map for repeated lookups", () => {
const bundled = new Map([
[
"feishu",
{
pluginId: "feishu",
localPath: "/app/extensions/feishu",
npmSpec: "@openclaw/feishu",
},
],
]);
expect(
findBundledPluginSourceInMap({
bundled,
lookup: { kind: "pluginId", value: "feishu" },
}),
).toEqual({
pluginId: "feishu",
localPath: "/app/extensions/feishu",
npmSpec: "@openclaw/feishu",
});
expect(
findBundledPluginSourceInMap({
bundled,
lookup: { kind: "npmSpec", value: "@openclaw/feishu" },
})?.pluginId,
).toBe("feishu");
});
});

View File

@@ -11,6 +11,25 @@ export type BundledPluginLookup =
| { kind: "npmSpec"; value: string }
| { kind: "pluginId"; value: string };
export function findBundledPluginSourceInMap(params: {
bundled: ReadonlyMap<string, BundledPluginSource>;
lookup: BundledPluginLookup;
}): BundledPluginSource | undefined {
const targetValue = params.lookup.value.trim();
if (!targetValue) {
return undefined;
}
if (params.lookup.kind === "pluginId") {
return params.bundled.get(targetValue);
}
for (const source of params.bundled.values()) {
if (source.npmSpec === targetValue) {
return source;
}
}
return undefined;
}
export function resolveBundledPluginSources(params: {
workspaceDir?: string;
}): Map<string, BundledPluginSource> {
@@ -49,18 +68,9 @@ export function findBundledPluginSource(params: {
lookup: BundledPluginLookup;
workspaceDir?: string;
}): BundledPluginSource | undefined {
const targetValue = params.lookup.value.trim();
if (!targetValue) {
return undefined;
}
const bundled = resolveBundledPluginSources({ workspaceDir: params.workspaceDir });
if (params.lookup.kind === "pluginId") {
return bundled.get(targetValue);
}
for (const source of bundled.values()) {
if (source.npmSpec === targetValue) {
return source;
}
}
return undefined;
return findBundledPluginSourceInMap({
bundled,
lookup: params.lookup,
});
}

View File

@@ -1,6 +1,7 @@
import { beforeEach, describe, expect, it, vi } from "vitest";
const installPluginFromNpmSpecMock = vi.fn();
const resolveBundledPluginSourcesMock = vi.fn();
vi.mock("./install.js", () => ({
installPluginFromNpmSpec: (...args: unknown[]) => installPluginFromNpmSpecMock(...args),
@@ -10,9 +11,14 @@ vi.mock("./install.js", () => ({
},
}));
vi.mock("./bundled-sources.js", () => ({
resolveBundledPluginSources: (...args: unknown[]) => resolveBundledPluginSourcesMock(...args),
}));
describe("updateNpmInstalledPlugins", () => {
beforeEach(() => {
installPluginFromNpmSpecMock.mockReset();
resolveBundledPluginSourcesMock.mockReset();
});
it("skips integrity drift checks for unpinned npm specs during dry-run updates", async () => {
@@ -151,3 +157,92 @@ describe("updateNpmInstalledPlugins", () => {
]);
});
});
describe("syncPluginsForUpdateChannel", () => {
beforeEach(() => {
installPluginFromNpmSpecMock.mockReset();
resolveBundledPluginSourcesMock.mockReset();
});
it("keeps bundled path installs on beta without reinstalling from npm", async () => {
resolveBundledPluginSourcesMock.mockReturnValue(
new Map([
[
"feishu",
{
pluginId: "feishu",
localPath: "/app/extensions/feishu",
npmSpec: "@openclaw/feishu",
},
],
]),
);
const { syncPluginsForUpdateChannel } = await import("./update.js");
const result = await syncPluginsForUpdateChannel({
channel: "beta",
config: {
plugins: {
load: { paths: ["/app/extensions/feishu"] },
installs: {
feishu: {
source: "path",
sourcePath: "/app/extensions/feishu",
installPath: "/app/extensions/feishu",
spec: "@openclaw/feishu",
},
},
},
},
});
expect(installPluginFromNpmSpecMock).not.toHaveBeenCalled();
expect(result.changed).toBe(false);
expect(result.summary.switchedToNpm).toEqual([]);
expect(result.config.plugins?.load?.paths).toEqual(["/app/extensions/feishu"]);
expect(result.config.plugins?.installs?.feishu?.source).toBe("path");
});
it("repairs bundled install metadata when the load path is re-added", async () => {
resolveBundledPluginSourcesMock.mockReturnValue(
new Map([
[
"feishu",
{
pluginId: "feishu",
localPath: "/app/extensions/feishu",
npmSpec: "@openclaw/feishu",
},
],
]),
);
const { syncPluginsForUpdateChannel } = await import("./update.js");
const result = await syncPluginsForUpdateChannel({
channel: "beta",
config: {
plugins: {
load: { paths: [] },
installs: {
feishu: {
source: "path",
sourcePath: "/app/extensions/feishu",
installPath: "/tmp/old-feishu",
spec: "@openclaw/feishu",
},
},
},
},
});
expect(result.changed).toBe(true);
expect(result.config.plugins?.load?.paths).toEqual(["/app/extensions/feishu"]);
expect(result.config.plugins?.installs?.feishu).toMatchObject({
source: "path",
sourcePath: "/app/extensions/feishu",
installPath: "/app/extensions/feishu",
spec: "@openclaw/feishu",
});
expect(installPluginFromNpmSpecMock).not.toHaveBeenCalled();
});
});

View File

@@ -459,42 +459,26 @@ export async function syncPluginsForUpdateChannel(params: {
if (!pathsEqual(record.sourcePath, bundledInfo.localPath)) {
continue;
}
const spec = record.spec ?? bundledInfo.npmSpec;
if (!spec) {
summary.warnings.push(`Missing npm spec for ${pluginId}; keeping local path.`);
continue;
}
let result: Awaited<ReturnType<typeof installPluginFromNpmSpec>>;
try {
result = await installPluginFromNpmSpec({
spec,
mode: "update",
expectedPluginId: pluginId,
logger: params.logger,
});
} catch (err) {
summary.errors.push(`Failed to install ${pluginId}: ${String(err)}`);
continue;
}
if (!result.ok) {
summary.errors.push(`Failed to install ${pluginId}: ${result.error}`);
// Keep explicit bundled installs on release channels. Replacing them with
// npm installs can reintroduce duplicate-id shadowing and packaging drift.
loadHelpers.addPath(bundledInfo.localPath);
const alreadyBundled =
record.source === "path" &&
pathsEqual(record.sourcePath, bundledInfo.localPath) &&
pathsEqual(record.installPath, bundledInfo.localPath);
if (alreadyBundled) {
continue;
}
next = recordPluginInstall(next, {
pluginId,
source: "npm",
spec,
installPath: result.targetDir,
version: result.version,
...buildNpmResolutionInstallFields(result.npmResolution),
sourcePath: undefined,
source: "path",
sourcePath: bundledInfo.localPath,
installPath: bundledInfo.localPath,
spec: record.spec ?? bundledInfo.npmSpec,
version: record.version,
});
summary.switchedToNpm.push(pluginId);
changed = true;
loadHelpers.removePath(bundledInfo.localPath);
}
}