diff --git a/CHANGELOG.md b/CHANGELOG.md index c66544475..f93fbf781 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. diff --git a/src/cli/plugin-install-plan.test.ts b/src/cli/plugin-install-plan.test.ts index b81ef7642..9aca36493 100644 --- a/src/cli/plugin-install-plan.test.ts +++ b/src/cli/plugin-install-plan.test.ts @@ -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", diff --git a/src/cli/plugin-install-plan.ts b/src/cli/plugin-install-plan.ts index fbb399a48..6c2467c15 100644 --- a/src/cli/plugin-install-plan.ts +++ b/src/cli/plugin-install-plan.ts @@ -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; diff --git a/src/commands/onboarding/plugin-install.test.ts b/src/commands/onboarding/plugin-install.test.ts index fbc204968..b04769dcb 100644 --- a/src/commands/onboarding/plugin-install.test.ts +++ b/src/commands/onboarding/plugin-install.test.ts @@ -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(); + 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; + 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 () => "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 () => {}); diff --git a/src/commands/onboarding/plugin-install.ts b/src/commands/onboarding/plugin-install.ts index 54a23c297..14245461e 100644 --- a/src/commands/onboarding/plugin-install.ts +++ b/src/commands/onboarding/plugin-install.ts @@ -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, diff --git a/src/plugins/bundled-sources.test.ts b/src/plugins/bundled-sources.test.ts index 7aace6f62..691dec466 100644 --- a/src/plugins/bundled-sources.test.ts +++ b/src/plugins/bundled-sources.test.ts @@ -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"); + }); }); diff --git a/src/plugins/bundled-sources.ts b/src/plugins/bundled-sources.ts index 4814246e1..a011227c2 100644 --- a/src/plugins/bundled-sources.ts +++ b/src/plugins/bundled-sources.ts @@ -11,6 +11,25 @@ export type BundledPluginLookup = | { kind: "npmSpec"; value: string } | { kind: "pluginId"; value: string }; +export function findBundledPluginSourceInMap(params: { + bundled: ReadonlyMap; + 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 { @@ -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, + }); } diff --git a/src/plugins/update.test.ts b/src/plugins/update.test.ts index 07e1dc359..07a2b6555 100644 --- a/src/plugins/update.test.ts +++ b/src/plugins/update.test.ts @@ -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(); + }); +}); diff --git a/src/plugins/update.ts b/src/plugins/update.ts index 553867425..a17c34b90 100644 --- a/src/plugins/update.ts +++ b/src/plugins/update.ts @@ -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>; - 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); } }