From 6bc5fe6952ff4ea79d4924b72c7150c688b11f78 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 26 Apr 2026 10:27:15 +0100 Subject: [PATCH] fix: harden plugin install and uninstall transactions --- CHANGELOG.md | 4 + docs/cli/plugins.md | 2 +- docs/tools/plugin.md | 5 +- src/auto-reply/reply/commands-plugins.test.ts | 2 + src/auto-reply/reply/commands-plugins.ts | 21 +-- src/cli/plugins-cli-test-helpers.ts | 47 +++++++ src/cli/plugins-cli.install.test.ts | 7 + src/cli/plugins-cli.ts | 80 +++++------ src/cli/plugins-cli.uninstall.test.ts | 96 +++++++++++-- src/cli/plugins-install-command.ts | 127 ++++++++++-------- src/cli/plugins-install-config.test.ts | 88 ++++-------- src/cli/plugins-install-persist.test.ts | 43 +++++- src/cli/plugins-install-persist.ts | 43 ++++-- src/plugins/uninstall.test.ts | 38 ++++++ src/plugins/uninstall.ts | 105 +++++++++++---- 15 files changed, 490 insertions(+), 218 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d58bf076fa0..60a9b8c0778 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ Docs: https://docs.openclaw.ai ## Unreleased +### Fixes + +- Plugins/CLI: make plugin install and uninstall config writes conflict-aware, clear stale denylist entries on explicit reinstall/removal, and delete managed plugin files only after config/index commit succeeds. Thanks @codex. + ## 2026.4.26 ### Fixes diff --git a/docs/cli/plugins.md b/docs/cli/plugins.md index 91b64876c9c..b7014fdb76e 100644 --- a/docs/cli/plugins.md +++ b/docs/cli/plugins.md @@ -226,7 +226,7 @@ openclaw plugins uninstall --dry-run openclaw plugins uninstall --keep-files ``` -`uninstall` removes plugin records from `plugins.entries`, the persisted plugin index, the plugin allowlist, and linked `plugins.load.paths` entries when applicable. Unless `--keep-files` is set, uninstall also removes the tracked managed install directory when it is inside OpenClaw's plugin extensions root. For active memory plugins, the memory slot resets to `memory-core`. +`uninstall` removes plugin records from `plugins.entries`, the persisted plugin index, plugin allow/deny list entries, and linked `plugins.load.paths` entries when applicable. Unless `--keep-files` is set, uninstall also removes the tracked managed install directory when it is inside OpenClaw's plugin extensions root. For active memory plugins, the memory slot resets to `memory-core`. `--keep-config` is supported as a deprecated alias for `--keep-files`. diff --git a/docs/tools/plugin.md b/docs/tools/plugin.md index ce415ca3413..6418ddb9577 100644 --- a/docs/tools/plugin.md +++ b/docs/tools/plugin.md @@ -337,8 +337,9 @@ plugins. It is not supported with `--link`, which reuses the source path instead of copying over a managed install target. When `plugins.allow` is already set, `openclaw plugins install` adds the -installed plugin id to that allowlist before enabling it, so installs are -immediately loadable after restart. +installed plugin id to that allowlist before enabling it. If the same plugin id +is present in `plugins.deny`, install removes that stale deny entry so the +explicit install is immediately loadable after restart. OpenClaw keeps a persisted local plugin registry as the cold read model for plugin inventory, contribution ownership, and startup planning. Install, update, diff --git a/src/auto-reply/reply/commands-plugins.test.ts b/src/auto-reply/reply/commands-plugins.test.ts index 829d1e19b94..819bf37e776 100644 --- a/src/auto-reply/reply/commands-plugins.test.ts +++ b/src/auto-reply/reply/commands-plugins.test.ts @@ -114,7 +114,9 @@ describe("handlePluginsCommand", () => { readConfigFileSnapshotMock.mockResolvedValue({ valid: true, path: "/tmp/openclaw.json", + sourceConfig: buildCfg(), resolved: buildCfg(), + hash: "config-1", }); validateConfigObjectWithPluginsMock.mockReturnValue({ ok: true, diff --git a/src/auto-reply/reply/commands-plugins.ts b/src/auto-reply/reply/commands-plugins.ts index e404d140e23..609562bccaf 100644 --- a/src/auto-reply/reply/commands-plugins.ts +++ b/src/auto-reply/reply/commands-plugins.ts @@ -7,6 +7,7 @@ import { resolveFileNpmSpecToLocalPath, } from "../../cli/plugins-command-helpers.js"; import { persistPluginInstall } from "../../cli/plugins-install-persist.js"; +import type { ConfigSnapshotForInstallPersist } from "../../cli/plugins-install-persist.js"; import { refreshPluginRegistryAfterConfigMutation } from "../../cli/plugins-registry-refresh.js"; import { readConfigFileSnapshot, @@ -162,7 +163,7 @@ function looksLikeLocalPluginInstallSpec(raw: string): boolean { async function installPluginFromPluginsCommand(params: { raw: string; - config: OpenClawConfig; + snapshot: ConfigSnapshotForInstallPersist; }): Promise<{ ok: true; pluginId: string } | { ok: false; error: string }> { const fileSpec = resolveFileNpmSpecToLocalPath(params.raw); if (fileSpec && !fileSpec.ok) { @@ -182,7 +183,7 @@ async function installPluginFromPluginsCommand(params: { clearPluginManifestRegistryCache(); const source: "archive" | "path" = resolveArchiveKind(resolved) ? "archive" : "path"; await persistPluginInstall({ - config: params.config, + snapshot: params.snapshot, pluginId: result.pluginId, install: { source, @@ -209,7 +210,7 @@ async function installPluginFromPluginsCommand(params: { } clearPluginManifestRegistryCache(); await persistPluginInstall({ - config: params.config, + snapshot: params.snapshot, pluginId: result.pluginId, install: { source: "clawhub", @@ -236,7 +237,7 @@ async function installPluginFromPluginsCommand(params: { if (clawhubResult.ok) { clearPluginManifestRegistryCache(); await persistPluginInstall({ - config: params.config, + snapshot: params.snapshot, pluginId: clawhubResult.pluginId, install: { source: "clawhub", @@ -273,7 +274,7 @@ async function installPluginFromPluginsCommand(params: { resolution: result.npmResolution, }); await persistPluginInstall({ - config: params.config, + snapshot: params.snapshot, pluginId: result.pluginId, install: installRecord, }); @@ -313,7 +314,8 @@ async function loadPluginCommandState( } async function loadPluginCommandConfig(): Promise< - { ok: true; path: string; config: OpenClawConfig } | { ok: false; path: string; error: string } + | { ok: true; path: string; snapshot: ConfigSnapshotForInstallPersist } + | { ok: false; path: string; error: string } > { const snapshot = await readConfigFileSnapshot(); if (!snapshot.valid) { @@ -326,7 +328,10 @@ async function loadPluginCommandConfig(): Promise< return { ok: true, path: snapshot.path, - config: structuredClone(snapshot.resolved), + snapshot: { + config: structuredClone(snapshot.sourceConfig), + baseHash: snapshot.hash, + }, }; } @@ -382,7 +387,7 @@ export const handlePluginsCommand: CommandHandler = async (params, allowTextComm } const installed = await installPluginFromPluginsCommand({ raw: pluginsCommand.spec, - config: loadedConfig.config, + snapshot: loadedConfig.snapshot, }); if (!installed.ok) { return { diff --git a/src/cli/plugins-cli-test-helpers.ts b/src/cli/plugins-cli-test-helpers.ts index 5241e319e13..a6a45cf9b4d 100644 --- a/src/cli/plugins-cli-test-helpers.ts +++ b/src/cli/plugins-cli-test-helpers.ts @@ -61,6 +61,8 @@ export const buildPluginCompatibilityNotices: UnknownMock = vi.fn(); export const inspectPluginRegistry: AsyncUnknownMock = vi.fn(); export const refreshPluginRegistry: AsyncUnknownMock = vi.fn(); export const applyExclusiveSlotSelection: UnknownMock = vi.fn(); +export const planPluginUninstall: UnknownMock = vi.fn(); +export const applyPluginUninstallDirectoryRemoval: AsyncUnknownMock = vi.fn(); export const uninstallPlugin: AsyncUnknownMock = vi.fn(); export const updateNpmInstalledPlugins: AsyncUnknownMock = vi.fn(); export const updateNpmInstalledHookPacks: AsyncUnknownMock = vi.fn(); @@ -314,6 +316,32 @@ vi.mock("../plugins/uninstall.js", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, + planPluginUninstall: (( + ...args: Parameters<(typeof import("../plugins/uninstall.js"))["planPluginUninstall"]> + ) => + invokeMock< + Parameters<(typeof import("../plugins/uninstall.js"))["planPluginUninstall"]>, + ReturnType<(typeof import("../plugins/uninstall.js"))["planPluginUninstall"]> + >( + planPluginUninstall, + ...args, + )) as (typeof import("../plugins/uninstall.js"))["planPluginUninstall"], + applyPluginUninstallDirectoryRemoval: (( + ...args: Parameters< + (typeof import("../plugins/uninstall.js"))["applyPluginUninstallDirectoryRemoval"] + > + ) => + invokeMock< + Parameters< + (typeof import("../plugins/uninstall.js"))["applyPluginUninstallDirectoryRemoval"] + >, + ReturnType< + (typeof import("../plugins/uninstall.js"))["applyPluginUninstallDirectoryRemoval"] + > + >( + applyPluginUninstallDirectoryRemoval, + ...args, + )) as (typeof import("../plugins/uninstall.js"))["applyPluginUninstallDirectoryRemoval"], uninstallPlugin: (( ...args: Parameters<(typeof import("../plugins/uninstall.js"))["uninstallPlugin"]> ) => @@ -496,6 +524,8 @@ export function resetPluginsCliTestState() { inspectPluginRegistry.mockReset(); refreshPluginRegistry.mockReset(); applyExclusiveSlotSelection.mockReset(); + planPluginUninstall.mockReset(); + applyPluginUninstallDirectoryRemoval.mockReset(); uninstallPlugin.mockReset(); updateNpmInstalledPlugins.mockReset(); updateNpmInstalledHookPacks.mockReset(); @@ -589,6 +619,23 @@ export function resetPluginsCliTestState() { config, warnings: [], })) as (...args: unknown[]) => unknown); + planPluginUninstall.mockImplementation((({ + config, + pluginId, + }: { + config: OpenClawConfig; + pluginId: string; + }) => ({ + ok: true, + config, + pluginId, + actions: createEmptyUninstallActions(), + directoryRemoval: null, + })) as (...args: unknown[]) => unknown); + applyPluginUninstallDirectoryRemoval.mockResolvedValue({ + directoryRemoved: false, + warnings: [], + }); uninstallPlugin.mockResolvedValue({ ok: true, config: {} as OpenClawConfig, diff --git a/src/cli/plugins-cli.install.test.ts b/src/cli/plugins-cli.install.test.ts index f8442141f3d..19c1f149249 100644 --- a/src/cli/plugins-cli.install.test.ts +++ b/src/cli/plugins-cli.install.test.ts @@ -21,6 +21,7 @@ import { recordHookInstall, recordPluginInstall, resetPluginsCliTestState, + replaceConfigFile, runPluginsCommand, runtimeErrors, runtimeLogs, @@ -336,6 +337,12 @@ describe("plugins cli install", () => { }), }); expect(writeConfigFile).toHaveBeenCalledWith(enabledCfg); + expect(replaceConfigFile).toHaveBeenCalledWith( + expect.objectContaining({ + baseHash: "mock", + nextConfig: enabledCfg, + }), + ); expect(runtimeLogs.some((line) => line.includes("slot adjusted"))).toBe(true); expect(runtimeLogs.some((line) => line.includes("Installed plugin: alpha"))).toBe(true); }); diff --git a/src/cli/plugins-cli.ts b/src/cli/plugins-cli.ts index 55b9bd57b1e..3df5753854a 100644 --- a/src/cli/plugins-cli.ts +++ b/src/cli/plugins-cli.ts @@ -25,12 +25,12 @@ import { } from "../plugins/status.js"; import type { PluginLogger } from "../plugins/types.js"; import { + applyPluginUninstallDirectoryRemoval, formatUninstallActionLabels, formatUninstallSlotResetPreview, + planPluginUninstall, resolveUninstallChannelConfigKeys, - resolveUninstallDirectoryTarget, UNINSTALL_ACTION_LABELS, - uninstallPlugin, } from "../plugins/uninstall.js"; import { defaultRuntime } from "../runtime.js"; import { formatDocsLink } from "../terminal/links.js"; @@ -614,50 +614,51 @@ export function registerPluginsCli(program: Command) { return defaultRuntime.exit(1); } - const install = cfg.plugins?.installs?.[pluginId]; - const isLinked = install?.source === "path"; + const channelIds = plugin?.status === "loaded" ? plugin.channelIds : undefined; + const plan = planPluginUninstall({ + config: cfg, + pluginId, + channelIds, + deleteFiles: !keepFiles, + extensionsDir, + }); + if (!plan.ok) { + defaultRuntime.error(plan.error); + return defaultRuntime.exit(1); + } + const preview: string[] = []; - if (hasEntry) { + if (plan.actions.entry) { preview.push(UNINSTALL_ACTION_LABELS.entry); } - if (hasInstall) { + if (plan.actions.install) { preview.push(UNINSTALL_ACTION_LABELS.install); } - if (cfg.plugins?.allow?.includes(pluginId)) { + if (plan.actions.allowlist) { preview.push(UNINSTALL_ACTION_LABELS.allowlist); } - if ( - isLinked && - install?.sourcePath && - cfg.plugins?.load?.paths?.includes(install.sourcePath) - ) { + if (plan.actions.denylist) { + preview.push(UNINSTALL_ACTION_LABELS.denylist); + } + if (plan.actions.loadPath) { preview.push(UNINSTALL_ACTION_LABELS.loadPath); } - if (cfg.plugins?.slots?.memory === pluginId) { + if (plan.actions.memorySlot) { preview.push(formatUninstallSlotResetPreview("memory")); } - if (cfg.plugins?.slots?.contextEngine === pluginId) { + if (plan.actions.contextEngineSlot) { preview.push(formatUninstallSlotResetPreview("contextEngine")); } - const channelIds = plugin?.status === "loaded" ? plugin.channelIds : undefined; const channels = cfg.channels as Record | undefined; - if (hasInstall && channels) { + if (plan.actions.channelConfig && hasInstall && channels) { for (const key of resolveUninstallChannelConfigKeys(pluginId, { channelIds })) { if (Object.hasOwn(channels, key)) { preview.push(`${UNINSTALL_ACTION_LABELS.channelConfig} (channels.${key})`); } } } - const deleteTarget = !keepFiles - ? resolveUninstallDirectoryTarget({ - pluginId, - hasInstall, - installRecord: install, - extensionsDir, - }) - : null; - if (deleteTarget) { - preview.push(`directory: ${shortenHomePath(deleteTarget)}`); + if (plan.directoryRemoval) { + preview.push(`directory: ${shortenHomePath(plan.directoryRemoval.target)}`); } const pluginName = plugin?.name || pluginId; @@ -679,24 +680,8 @@ export function registerPluginsCli(program: Command) { } } - const result = await uninstallPlugin({ - config: cfg, - pluginId, - channelIds, - deleteFiles: !keepFiles, - extensionsDir, - }); - - if (!result.ok) { - defaultRuntime.error(result.error); - return defaultRuntime.exit(1); - } - for (const warning of result.warnings) { - defaultRuntime.log(theme.warn(warning)); - } - const nextInstallRecords = removePluginInstallRecordFromRecords(installRecords, pluginId); - const nextConfig = withoutPluginInstallRecords(result.config); + const nextConfig = withoutPluginInstallRecords(plan.config); await commitPluginInstallRecordsWithConfig({ previousInstallRecords: installRecords, nextInstallRecords, @@ -711,8 +696,15 @@ export function registerPluginsCli(program: Command) { warn: (message) => defaultRuntime.log(theme.warn(message)), }, }); + const directoryResult = await applyPluginUninstallDirectoryRemoval(plan.directoryRemoval); + for (const warning of directoryResult.warnings) { + defaultRuntime.log(theme.warn(warning)); + } - const removed = formatUninstallActionLabels(result.actions); + const removed = formatUninstallActionLabels({ + ...plan.actions, + directory: directoryResult.directoryRemoved, + }); defaultRuntime.log( `Uninstalled plugin "${pluginId}". Removed: ${removed.length > 0 ? removed.join(", ") : "nothing"}.`, diff --git a/src/cli/plugins-cli.uninstall.test.ts b/src/cli/plugins-cli.uninstall.test.ts index 91452a5d114..86696df2fc2 100644 --- a/src/cli/plugins-cli.uninstall.test.ts +++ b/src/cli/plugins-cli.uninstall.test.ts @@ -2,8 +2,10 @@ import { beforeEach, describe, expect, it } from "vitest"; import { installedPluginRoot } from "../../test/helpers/bundled-plugin-paths.js"; import type { OpenClawConfig } from "../config/config.js"; import { + applyPluginUninstallDirectoryRemoval, buildPluginDiagnosticsReport, loadConfig, + planPluginUninstall, promptYesNo, refreshPluginRegistry, replaceConfigFile, @@ -12,7 +14,6 @@ import { runtimeErrors, runtimeLogs, setInstalledPluginIndexInstallRecords, - uninstallPlugin, writeConfigFile, writePersistedInstalledPluginIndexInstallRecords, } from "./plugins-cli-test-helpers.js"; @@ -49,10 +50,25 @@ describe("plugins cli uninstall", () => { plugins: [{ id: "alpha", name: "alpha" }], diagnostics: [], }); + planPluginUninstall.mockReturnValue({ + ok: true, + config: {} as OpenClawConfig, + actions: { + entry: true, + install: true, + allowlist: false, + denylist: false, + loadPath: false, + memorySlot: false, + contextEngineSlot: true, + directory: false, + }, + directoryRemoval: null, + }); await runPluginsCommand(["plugins", "uninstall", "alpha", "--dry-run"]); - expect(uninstallPlugin).not.toHaveBeenCalled(); + expect(planPluginUninstall).toHaveBeenCalled(); expect(writeConfigFile).not.toHaveBeenCalled(); expect(refreshPluginRegistry).not.toHaveBeenCalled(); expect(runtimeLogs.some((line) => line.includes("Dry run, no changes made."))).toBe(true); @@ -87,25 +103,26 @@ describe("plugins cli uninstall", () => { plugins: [{ id: "alpha", name: "alpha" }], diagnostics: [], }); - uninstallPlugin.mockResolvedValue({ + planPluginUninstall.mockReturnValue({ ok: true, config: nextConfig, - warnings: [], actions: { entry: true, install: true, allowlist: false, + denylist: false, loadPath: false, memorySlot: false, contextEngineSlot: false, directory: false, }, + directoryRemoval: null, }); await runPluginsCommand(["plugins", "uninstall", "alpha", "--force", "--keep-files"]); expect(promptYesNo).not.toHaveBeenCalled(); - expect(uninstallPlugin).toHaveBeenCalledWith( + expect(planPluginUninstall).toHaveBeenCalledWith( expect.objectContaining({ pluginId: "alpha", deleteFiles: false, @@ -157,19 +174,20 @@ describe("plugins cli uninstall", () => { plugins: [{ id: "alpha", name: "alpha" }], diagnostics: [], }); - uninstallPlugin.mockResolvedValue({ + planPluginUninstall.mockReturnValue({ ok: true, config: nextConfig, - warnings: [], actions: { entry: true, install: true, allowlist: false, + denylist: false, loadPath: false, memorySlot: false, contextEngineSlot: false, directory: false, }, + directoryRemoval: null, }); replaceConfigFile.mockRejectedValueOnce(new Error("config changed")); @@ -183,6 +201,68 @@ describe("plugins cli uninstall", () => { installRecords, ); expect(refreshPluginRegistry).not.toHaveBeenCalled(); + expect(applyPluginUninstallDirectoryRemoval).not.toHaveBeenCalled(); + }); + + it("removes plugin files only after config and index commit succeeds", async () => { + const installRecords = { + alpha: { + source: "npm", + spec: "alpha@1.0.0", + installPath: ALPHA_INSTALL_PATH, + }, + } as const; + const baseConfig = { + plugins: { + entries: { + alpha: { enabled: true }, + }, + installs: installRecords, + }, + } as OpenClawConfig; + const nextConfig = { + plugins: { + entries: {}, + installs: {}, + }, + } as OpenClawConfig; + + loadConfig.mockReturnValue(baseConfig); + setInstalledPluginIndexInstallRecords(installRecords); + buildPluginDiagnosticsReport.mockReturnValue({ + plugins: [{ id: "alpha", name: "alpha" }], + diagnostics: [], + }); + planPluginUninstall.mockReturnValue({ + ok: true, + config: nextConfig, + actions: { + entry: true, + install: true, + allowlist: false, + denylist: false, + loadPath: false, + memorySlot: false, + contextEngineSlot: false, + directory: false, + }, + directoryRemoval: { target: ALPHA_INSTALL_PATH }, + }); + applyPluginUninstallDirectoryRemoval.mockResolvedValue({ + directoryRemoved: true, + warnings: [], + }); + + await runPluginsCommand(["plugins", "uninstall", "alpha", "--force"]); + + const configWriteOrder = writeConfigFile.mock.invocationCallOrder[0] ?? 0; + const deleteOrder = + applyPluginUninstallDirectoryRemoval.mock.invocationCallOrder[0] ?? Number.MAX_SAFE_INTEGER; + expect(configWriteOrder).toBeGreaterThan(0); + expect(deleteOrder).toBeGreaterThan(configWriteOrder); + expect(applyPluginUninstallDirectoryRemoval).toHaveBeenCalledWith({ + target: ALPHA_INSTALL_PATH, + }); }); it("exits when uninstall target is not managed by plugin install records", async () => { @@ -202,6 +282,6 @@ describe("plugins cli uninstall", () => { ); expect(runtimeErrors.at(-1)).toContain("is not managed by plugins config/install records"); - expect(uninstallPlugin).not.toHaveBeenCalled(); + expect(planPluginUninstall).not.toHaveBeenCalled(); }); }); diff --git a/src/cli/plugins-install-command.ts b/src/cli/plugins-install-command.ts index d587fc1e556..f708acf8484 100644 --- a/src/cli/plugins-install-command.ts +++ b/src/cli/plugins-install-command.ts @@ -1,11 +1,10 @@ import fs from "node:fs"; import { collectChannelDoctorStaleConfigMutations } from "../commands/doctor/shared/channel-doctor.js"; -import { loadConfig, readConfigFileSnapshot } from "../config/config.js"; -import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { readConfigFileSnapshot } from "../config/config.js"; import { installHooksFromNpmSpec, installHooksFromPath } from "../hooks/install.js"; import { resolveArchiveKind } from "../infra/archive.js"; import { parseClawHubPluginSpec } from "../infra/clawhub.js"; -import { extractErrorCode, formatErrorMessage } from "../infra/errors.js"; +import { formatErrorMessage } from "../infra/errors.js"; import { type BundledPluginSource, findBundledPluginSource } from "../plugins/bundled-sources.js"; import { formatClawHubSpecifier, installPluginFromClawHub } from "../plugins/clawhub.js"; import type { InstallSafetyOverrides } from "../plugins/install-security-scan.js"; @@ -41,6 +40,7 @@ import { formatPluginInstallWithHookFallbackError, } from "./plugins-command-helpers.js"; import { persistHookPackInstall, persistPluginInstall } from "./plugins-install-persist.js"; +import type { ConfigSnapshotForInstallPersist } from "./plugins-install-persist.js"; function resolveInstallMode(force?: boolean): "install" | "update" { return force ? "update" : "install"; @@ -53,23 +53,26 @@ function resolveInstallSafetyOverrides(overrides: InstallSafetyOverrides): Insta } async function installBundledPluginSource(params: { - config: OpenClawConfig; + snapshot: ConfigSnapshotForInstallPersist; rawSpec: string; bundledSource: BundledPluginSource; warning: string; }) { - const existing = params.config.plugins?.load?.paths ?? []; + const existing = params.snapshot.config.plugins?.load?.paths ?? []; const mergedPaths = Array.from(new Set([...existing, params.bundledSource.localPath])); await persistPluginInstall({ - config: { - ...params.config, - plugins: { - ...params.config.plugins, - load: { - ...params.config.plugins?.load, - paths: mergedPaths, + snapshot: { + config: { + ...params.snapshot.config, + plugins: { + ...params.snapshot.config.plugins, + load: { + ...params.snapshot.config.plugins?.load, + paths: mergedPaths, + }, }, }, + baseHash: params.snapshot.baseHash, }, pluginId: params.bundledSource.pluginId, install: { @@ -83,7 +86,7 @@ async function installBundledPluginSource(params: { } async function tryInstallHookPackFromLocalPath(params: { - config: OpenClawConfig; + snapshot: ConfigSnapshotForInstallPersist; resolvedPath: string; installMode: "install" | "update"; safetyOverrides?: InstallSafetyOverrides; @@ -107,22 +110,25 @@ async function tryInstallHookPackFromLocalPath(params: { return probe; } - const existing = params.config.hooks?.internal?.load?.extraDirs ?? []; + const existing = params.snapshot.config.hooks?.internal?.load?.extraDirs ?? []; const merged = Array.from(new Set([...existing, params.resolvedPath])); await persistHookPackInstall({ - config: { - ...params.config, - hooks: { - ...params.config.hooks, - internal: { - ...params.config.hooks?.internal, - enabled: true, - load: { - ...params.config.hooks?.internal?.load, - extraDirs: merged, + snapshot: { + config: { + ...params.snapshot.config, + hooks: { + ...params.snapshot.config.hooks, + internal: { + ...params.snapshot.config.hooks?.internal, + enabled: true, + load: { + ...params.snapshot.config.hooks?.internal?.load, + extraDirs: merged, + }, }, }, }, + baseHash: params.snapshot.baseHash, }, hookPackId: probe.hookPackId, hooks: probe.hooks, @@ -149,7 +155,7 @@ async function tryInstallHookPackFromLocalPath(params: { const source: "archive" | "path" = resolveArchiveKind(params.resolvedPath) ? "archive" : "path"; await persistHookPackInstall({ - config: params.config, + snapshot: params.snapshot, hookPackId: result.hookPackId, hooks: result.hooks, install: { @@ -163,7 +169,7 @@ async function tryInstallHookPackFromLocalPath(params: { } async function tryInstallHookPackFromNpmSpec(params: { - config: OpenClawConfig; + snapshot: ConfigSnapshotForInstallPersist; installMode: "install" | "update"; spec: string; pin?: boolean; @@ -187,7 +193,7 @@ async function tryInstallHookPackFromNpmSpec(params: { theme.warn, ); await persistHookPackInstall({ - config: params.config, + snapshot: params.snapshot, hookPackId: result.hookPackId, hooks: result.hooks, install: installRecord, @@ -231,13 +237,13 @@ function buildInvalidPluginInstallConfigError(message: string): Error { async function loadConfigFromSnapshotForInstall( request: PluginInstallRequestContext, -): Promise { + snapshot: Awaited>, +): Promise { if (resolvePluginInstallInvalidConfigPolicy(request) !== "allow-bundled-recovery") { throw buildInvalidPluginInstallConfigError( "Config invalid; run `openclaw doctor --fix` before installing plugins.", ); } - const snapshot = await readConfigFileSnapshot(); const parsed = (snapshot.parsed ?? {}) as Record; if (!snapshot.exists || Object.keys(parsed).length === 0) { throw buildInvalidPluginInstallConfigError( @@ -260,20 +266,23 @@ async function loadConfigFromSnapshotForInstall( })) { nextConfig = mutation.config; } - return nextConfig; + return { + config: nextConfig, + baseHash: snapshot.hash, + }; } export async function loadConfigForInstall( request: PluginInstallRequestContext, -): Promise { - try { - return loadConfig(); - } catch (err) { - if (extractErrorCode(err) !== "INVALID_CONFIG") { - throw err; - } +): Promise { + const snapshot = await readConfigFileSnapshot(); + if (snapshot.valid) { + return { + config: snapshot.sourceConfig, + baseHash: snapshot.hash, + }; } - return loadConfigFromSnapshotForInstall(request); + return loadConfigFromSnapshotForInstall(request, snapshot); } export async function runPluginInstallCommand(params: { @@ -322,13 +331,14 @@ export async function runPluginInstallCommand(params: { return defaultRuntime.exit(1); } const request = requestResolution.request; - const cfg = await loadConfigForInstall(request).catch((error: unknown) => { + const snapshot = await loadConfigForInstall(request).catch((error: unknown) => { defaultRuntime.error(formatErrorMessage(error)); return null; }); - if (!cfg) { + if (!snapshot) { return defaultRuntime.exit(1); } + const cfg = snapshot.config; const installMode = resolveInstallMode(opts.force); const safetyOverrides = resolveInstallSafetyOverrides(opts); @@ -347,7 +357,7 @@ export async function runPluginInstallCommand(params: { clearPluginManifestRegistryCache(); await persistPluginInstall({ - config: cfg, + snapshot, pluginId: result.pluginId, install: { source: "marketplace", @@ -381,7 +391,7 @@ export async function runPluginInstallCommand(params: { return defaultRuntime.exit(1); } const hookFallback = await tryInstallHookPackFromLocalPath({ - config: cfg, + snapshot, installMode, resolvedPath: resolved, safetyOverrides, @@ -397,15 +407,18 @@ export async function runPluginInstallCommand(params: { } await persistPluginInstall({ - config: { - ...cfg, - plugins: { - ...cfg.plugins, - load: { - ...cfg.plugins?.load, - paths: merged, + snapshot: { + config: { + ...cfg, + plugins: { + ...cfg.plugins, + load: { + ...cfg.plugins?.load, + paths: merged, + }, }, }, + baseHash: snapshot.baseHash, }, pluginId: probe.pluginId, install: { @@ -431,7 +444,7 @@ export async function runPluginInstallCommand(params: { return defaultRuntime.exit(1); } const hookFallback = await tryInstallHookPackFromLocalPath({ - config: cfg, + snapshot, installMode, resolvedPath: resolved, safetyOverrides, @@ -448,7 +461,7 @@ export async function runPluginInstallCommand(params: { clearPluginManifestRegistryCache(); const source: "archive" | "path" = resolveArchiveKind(resolved) ? "archive" : "path"; await persistPluginInstall({ - config: cfg, + snapshot, pluginId: result.pluginId, install: { source, @@ -487,7 +500,7 @@ export async function runPluginInstallCommand(params: { }); if (bundledPreNpmPlan) { await installBundledPluginSource({ - config: cfg, + snapshot, rawSpec: raw, bundledSource: bundledPreNpmPlan.bundledSource, warning: bundledPreNpmPlan.warning, @@ -510,7 +523,7 @@ export async function runPluginInstallCommand(params: { clearPluginManifestRegistryCache(); await persistPluginInstall({ - config: cfg, + snapshot, pluginId: result.pluginId, install: { source: "clawhub", @@ -542,7 +555,7 @@ export async function runPluginInstallCommand(params: { if (clawhubResult.ok) { clearPluginManifestRegistryCache(); await persistPluginInstall({ - config: cfg, + snapshot, pluginId: clawhubResult.pluginId, install: { source: "clawhub", @@ -586,7 +599,7 @@ export async function runPluginInstallCommand(params: { }); if (!bundledFallbackPlan) { const hookFallback = await tryInstallHookPackFromNpmSpec({ - config: cfg, + snapshot, installMode, spec: raw, pin: opts.pin, @@ -601,7 +614,7 @@ export async function runPluginInstallCommand(params: { } await installBundledPluginSource({ - config: cfg, + snapshot, rawSpec: raw, bundledSource: bundledFallbackPlan.bundledSource, warning: bundledFallbackPlan.warning, @@ -620,7 +633,7 @@ export async function runPluginInstallCommand(params: { theme.warn, ); await persistPluginInstall({ - config: cfg, + snapshot, pluginId: result.pluginId, install: installRecord, }); diff --git a/src/cli/plugins-install-config.test.ts b/src/cli/plugins-install-config.test.ts index 0f76667bc6d..8e1447d2c1f 100644 --- a/src/cli/plugins-install-config.test.ts +++ b/src/cli/plugins-install-config.test.ts @@ -9,18 +9,15 @@ import { import { loadConfigForInstall } from "./plugins-install-command.js"; const hoisted = vi.hoisted(() => ({ - loadConfigMock: vi.fn<() => OpenClawConfig>(), readConfigFileSnapshotMock: vi.fn<() => Promise>(), collectChannelDoctorStaleConfigMutationsMock: vi.fn(), })); -const loadConfigMock = hoisted.loadConfigMock; const readConfigFileSnapshotMock = hoisted.readConfigFileSnapshotMock; const collectChannelDoctorStaleConfigMutationsMock = hoisted.collectChannelDoctorStaleConfigMutationsMock; vi.mock("../config/config.js", () => ({ - loadConfig: () => loadConfigMock(), readConfigFileSnapshot: () => readConfigFileSnapshotMock(), })); @@ -59,7 +56,6 @@ describe("loadConfigForInstall", () => { } satisfies PluginInstallRequestContext; beforeEach(() => { - loadConfigMock.mockReset(); readConfigFileSnapshotMock.mockReset(); collectChannelDoctorStaleConfigMutationsMock.mockReset(); @@ -71,31 +67,39 @@ describe("loadConfigForInstall", () => { ]); }); - it("returns the config directly when loadConfig succeeds", async () => { + it("returns the source config and base hash when the snapshot is valid", async () => { const cfg = { plugins: { entries: { matrix: { enabled: true } } } } as OpenClawConfig; - loadConfigMock.mockReturnValue(cfg); + readConfigFileSnapshotMock.mockResolvedValue( + makeSnapshot({ + valid: true, + sourceConfig: cfg, + config: { plugins: { entries: { matrix: { enabled: true } }, enabled: true } }, + hash: "config-1", + issues: [], + }), + ); const result = await loadConfigForInstall(matrixNpmRequest); - expect(result).toBe(cfg); - expect(readConfigFileSnapshotMock).not.toHaveBeenCalled(); + expect(result).toEqual({ config: cfg, baseHash: "config-1" }); }); it("does not run stale Matrix cleanup on the happy path", async () => { const cfg = { plugins: {} } as OpenClawConfig; - loadConfigMock.mockReturnValue(cfg); + readConfigFileSnapshotMock.mockResolvedValue( + makeSnapshot({ + valid: true, + sourceConfig: cfg, + config: cfg, + issues: [], + }), + ); const result = await loadConfigForInstall(matrixNpmRequest); expect(collectChannelDoctorStaleConfigMutationsMock).not.toHaveBeenCalled(); - expect(result).toBe(cfg); + expect(result.config).toBe(cfg); }); it("falls back to snapshot config for explicit bundled-plugin reinstall when issues match the known upgrade failure", async () => { - const invalidConfigErr = new Error("config invalid"); - (invalidConfigErr as { code?: string }).code = "INVALID_CONFIG"; - loadConfigMock.mockImplementation(() => { - throw invalidConfigErr; - }); - const snapshotCfg = { plugins: { installs: { matrix: { source: "path", installPath: "/gone" } } }, } as unknown as OpenClawConfig; @@ -113,16 +117,10 @@ describe("loadConfigForInstall", () => { const result = await loadConfigForInstall(matrixNpmRequest); expect(readConfigFileSnapshotMock).toHaveBeenCalled(); expect(collectChannelDoctorStaleConfigMutationsMock).toHaveBeenCalledWith(snapshotCfg); - expect(result).toBe(snapshotCfg); + expect(result).toEqual({ config: snapshotCfg, baseHash: "abc" }); }); it("allows explicit repo-checkout bundled-plugin reinstall recovery", async () => { - const invalidConfigErr = new Error("config invalid"); - (invalidConfigErr as { code?: string }).code = "INVALID_CONFIG"; - loadConfigMock.mockImplementation(() => { - throw invalidConfigErr; - }); - const snapshotCfg = { plugins: {} } as OpenClawConfig; readConfigFileSnapshotMock.mockResolvedValue( makeSnapshot({ @@ -142,16 +140,10 @@ describe("loadConfigForInstall", () => { ...repoRequest.request, resolvedPath: bundledPluginRootAt("/tmp/repo", "matrix"), }); - expect(result).toBe(snapshotCfg); + expect(result.config).toBe(snapshotCfg); }); it("rejects unrelated invalid config even during bundled-plugin reinstall recovery", async () => { - const invalidConfigErr = new Error("config invalid"); - (invalidConfigErr as { code?: string }).code = "INVALID_CONFIG"; - loadConfigMock.mockImplementation(() => { - throw invalidConfigErr; - }); - readConfigFileSnapshotMock.mockResolvedValue( makeSnapshot({ issues: [{ path: "models.default", message: "invalid model ref" }], @@ -164,11 +156,7 @@ describe("loadConfigForInstall", () => { }); it("rejects non-Matrix install requests when config is invalid", async () => { - const invalidConfigErr = new Error("config invalid"); - (invalidConfigErr as { code?: string }).code = "INVALID_CONFIG"; - loadConfigMock.mockImplementation(() => { - throw invalidConfigErr; - }); + readConfigFileSnapshotMock.mockResolvedValue(makeSnapshot()); await expect( loadConfigForInstall({ @@ -176,16 +164,9 @@ describe("loadConfigForInstall", () => { normalizedSpec: "alpha", }), ).rejects.toThrow("Config invalid; run `openclaw doctor --fix` before installing plugins."); - expect(readConfigFileSnapshotMock).not.toHaveBeenCalled(); }); - it("throws when loadConfig fails with INVALID_CONFIG and snapshot parsed is empty", async () => { - const invalidConfigErr = new Error("config invalid"); - (invalidConfigErr as { code?: string }).code = "INVALID_CONFIG"; - loadConfigMock.mockImplementation(() => { - throw invalidConfigErr; - }); - + it("throws when invalid snapshot parsed is empty", async () => { readConfigFileSnapshotMock.mockResolvedValue( makeSnapshot({ parsed: {}, @@ -198,30 +179,11 @@ describe("loadConfigForInstall", () => { ); }); - it("throws when loadConfig fails with INVALID_CONFIG and config file does not exist", async () => { - const invalidConfigErr = new Error("config invalid"); - (invalidConfigErr as { code?: string }).code = "INVALID_CONFIG"; - loadConfigMock.mockImplementation(() => { - throw invalidConfigErr; - }); - + it("throws when invalid snapshot config file does not exist", async () => { readConfigFileSnapshotMock.mockResolvedValue(makeSnapshot({ exists: false, parsed: {} })); await expect(loadConfigForInstall(matrixNpmRequest)).rejects.toThrow( "Config file could not be parsed; run `openclaw doctor` to repair it.", ); }); - - it("re-throws non-config errors from loadConfig", async () => { - const fsErr = new Error("EACCES: permission denied"); - (fsErr as { code?: string }).code = "EACCES"; - loadConfigMock.mockImplementation(() => { - throw fsErr; - }); - - await expect(loadConfigForInstall(matrixNpmRequest)).rejects.toThrow( - "EACCES: permission denied", - ); - expect(readConfigFileSnapshotMock).not.toHaveBeenCalled(); - }); }); diff --git a/src/cli/plugins-install-persist.test.ts b/src/cli/plugins-install-persist.test.ts index f7f686b379b..b4e26d69b0e 100644 --- a/src/cli/plugins-install-persist.test.ts +++ b/src/cli/plugins-install-persist.test.ts @@ -36,7 +36,10 @@ describe("persistPluginInstall", () => { }); const next = await persistPluginInstall({ - config: baseConfig, + snapshot: { + config: baseConfig, + baseHash: "config-1", + }, pluginId: "alpha", install: { source: "npm", @@ -66,4 +69,42 @@ describe("persistPluginInstall", () => { reason: "source-changed", }); }); + + it("removes stale denylist entries before enabling installed plugins", async () => { + const { persistPluginInstall } = await import("./plugins-install-persist.js"); + const baseConfig = { + plugins: { + deny: ["alpha", "other"], + }, + } as OpenClawConfig; + const enabledConfig = { + plugins: { + deny: ["other"], + entries: { + alpha: { enabled: true }, + }, + }, + } as OpenClawConfig; + enablePluginInConfig.mockImplementation((...args: unknown[]) => { + const [cfg, pluginId] = args as [OpenClawConfig, string]; + expect(pluginId).toBe("alpha"); + expect(cfg.plugins?.deny).toEqual(["other"]); + return { config: enabledConfig }; + }); + + const next = await persistPluginInstall({ + snapshot: { + config: baseConfig, + baseHash: "config-1", + }, + pluginId: "alpha", + install: { + source: "npm", + spec: "alpha@1.0.0", + installPath: "/tmp/alpha", + }, + }); + + expect(next).toEqual(enabledConfig); + }); }); diff --git a/src/cli/plugins-install-persist.ts b/src/cli/plugins-install-persist.ts index 7a1d6973f4a..9597e2d94af 100644 --- a/src/cli/plugins-install-persist.ts +++ b/src/cli/plugins-install-persist.ts @@ -33,18 +33,42 @@ function addInstalledPluginToAllowlist(cfg: OpenClawConfig, pluginId: string): O }; } -export async function persistPluginInstall(params: { +function removeInstalledPluginFromDenylist(cfg: OpenClawConfig, pluginId: string): OpenClawConfig { + const deny = cfg.plugins?.deny; + if (!Array.isArray(deny) || !deny.includes(pluginId)) { + return cfg; + } + const nextDeny = deny.filter((id) => id !== pluginId); + const plugins = { + ...cfg.plugins, + ...(nextDeny.length > 0 ? { deny: nextDeny } : {}), + }; + if (nextDeny.length === 0) { + delete plugins.deny; + } + return { + ...cfg, + plugins, + }; +} + +export type ConfigSnapshotForInstallPersist = { config: OpenClawConfig; - baseHash?: string; + baseHash: string | undefined; +}; + +export async function persistPluginInstall(params: { + snapshot: ConfigSnapshotForInstallPersist; pluginId: string; install: Omit; successMessage?: string; warningMessage?: string; }): Promise { - let next = enablePluginInConfig( - addInstalledPluginToAllowlist(params.config, params.pluginId), + const installConfig = removeInstalledPluginFromDenylist( + addInstalledPluginToAllowlist(params.snapshot.config, params.pluginId), params.pluginId, - ).config; + ); + let next = enablePluginInConfig(installConfig, params.pluginId).config; const installRecords = await loadInstalledPluginIndexInstallRecords(); const nextInstallRecords = recordPluginInstallInRecords(installRecords, { pluginId: params.pluginId, @@ -56,7 +80,7 @@ export async function persistPluginInstall(params: { previousInstallRecords: installRecords, nextInstallRecords, nextConfig: next, - ...(params.baseHash !== undefined ? { baseHash: params.baseHash } : {}), + baseHash: params.snapshot.baseHash, }); await refreshPluginRegistryAfterConfigMutation({ config: next, @@ -76,14 +100,13 @@ export async function persistPluginInstall(params: { } export async function persistHookPackInstall(params: { - config: OpenClawConfig; - baseHash?: string; + snapshot: ConfigSnapshotForInstallPersist; hookPackId: string; hooks: string[]; install: Omit; successMessage?: string; }): Promise { - let next = enableInternalHookEntries(params.config, params.hooks); + let next = enableInternalHookEntries(params.snapshot.config, params.hooks); next = recordHookInstall(next, { hookId: params.hookPackId, hooks: params.hooks, @@ -91,7 +114,7 @@ export async function persistHookPackInstall(params: { }); await replaceConfigFile({ nextConfig: next, - ...(params.baseHash !== undefined ? { baseHash: params.baseHash } : {}), + baseHash: params.snapshot.baseHash, }); defaultRuntime.log(params.successMessage ?? `Installed hook pack: ${params.hookPackId}`); logHookPackRestartHint(); diff --git a/src/plugins/uninstall.test.ts b/src/plugins/uninstall.test.ts index 5ad61a0f4e6..584baf928e4 100644 --- a/src/plugins/uninstall.test.ts +++ b/src/plugins/uninstall.test.ts @@ -9,7 +9,9 @@ import { makeTrackedTempDirAsync, } from "./test-helpers/fs-fixtures.js"; import { + applyPluginUninstallDirectoryRemoval, removePluginFromConfig, + planPluginUninstall, resolveUninstallChannelConfigKeys, resolveUninstallDirectoryTarget, uninstallPlugin, @@ -281,6 +283,17 @@ describe("removePluginFromConfig", () => { expect(actions.allowlist).toBe(true); }); + it("removes plugin from denylist", () => { + const config = createPluginConfig({ + deny: ["my-plugin", "other-plugin"], + }); + + const { config: result, actions } = removePluginFromConfig(config, "my-plugin"); + + expect(result.plugins?.deny).toEqual(["other-plugin"]); + expect(actions.denylist).toBe(true); + }); + it.each([ { name: "removes linked path from load.paths", @@ -700,6 +713,31 @@ describe("uninstallPlugin", () => { } }); + it("plans directory removal without deleting before commit", async () => { + const { pluginId, extensionsDir, pluginDir, config } = await createInstalledNpmPluginFixture({ + baseDir: tempDir, + }); + + const plan = planPluginUninstall({ + config, + pluginId, + deleteFiles: true, + extensionsDir, + }); + + expect(plan.ok).toBe(true); + if (!plan.ok) { + throw new Error(plan.error); + } + expect(plan.directoryRemoval).toEqual({ target: pluginDir }); + expect(plan.actions.directory).toBe(false); + await expect(fs.access(pluginDir)).resolves.toBeUndefined(); + + const applied = await applyPluginUninstallDirectoryRemoval(plan.directoryRemoval); + expect(applied).toEqual({ directoryRemoved: true, warnings: [] }); + await expect(fs.access(pluginDir)).rejects.toThrow(); + }); + it.each([ { name: "preserves directory for linked plugins", diff --git a/src/plugins/uninstall.ts b/src/plugins/uninstall.ts index 918dab1c0f4..93f2c66c276 100644 --- a/src/plugins/uninstall.ts +++ b/src/plugins/uninstall.ts @@ -11,6 +11,7 @@ export type UninstallActions = { entry: boolean; install: boolean; allowlist: boolean; + denylist: boolean; loadPath: boolean; memorySlot: boolean; contextEngineSlot: boolean; @@ -22,6 +23,7 @@ export const UNINSTALL_ACTION_LABELS = { entry: "config entry", install: "install record", allowlist: "allowlist entry", + denylist: "denylist entry", loadPath: "load path", memorySlot: "memory slot", contextEngineSlot: "context engine slot", @@ -33,6 +35,7 @@ const UNINSTALL_ACTION_ORDER = [ "entry", "install", "allowlist", + "denylist", "loadPath", "memorySlot", "contextEngineSlot", @@ -47,6 +50,7 @@ export function createEmptyUninstallActions( entry: false, install: false, allowlist: false, + denylist: false, loadPath: false, memorySlot: false, contextEngineSlot: false, @@ -82,6 +86,20 @@ export type UninstallPluginResult = } | { ok: false; error: string }; +export type PluginUninstallDirectoryRemoval = { + target: string; +}; + +export type PluginUninstallPlanResult = + | { + ok: true; + config: OpenClawConfig; + pluginId: string; + actions: UninstallActions; + directoryRemoval: PluginUninstallDirectoryRemoval | null; + } + | { ok: false; error: string }; + export function resolveUninstallDirectoryTarget(params: { pluginId: string; hasInstall: boolean; @@ -235,6 +253,17 @@ export function removePluginFromConfig( actions.allowlist = true; } + // Remove from denylist. An explicit uninstall should clear stale policy so a + // later reinstall can enable the plugin deterministically. + let deny = pluginsConfig.deny; + if (Array.isArray(deny) && deny.includes(pluginId)) { + deny = deny.filter((id) => id !== pluginId); + if (deny.length === 0) { + deny = undefined; + } + actions.denylist = true; + } + // Remove linked path from load.paths (for source === "path" plugins) let load = pluginsConfig.load; if (installRecord?.source === "path" && installRecord.sourcePath) { @@ -277,6 +306,7 @@ export function removePluginFromConfig( entries, installs, allow, + deny, load, slots, }; @@ -292,6 +322,9 @@ export function removePluginFromConfig( if (cleanedPlugins.allow === undefined) { delete cleanedPlugins.allow; } + if (cleanedPlugins.deny === undefined) { + delete cleanedPlugins.deny; + } if (cleanedPlugins.load === undefined) { delete cleanedPlugins.load; } @@ -335,12 +368,10 @@ export type UninstallPluginParams = { }; /** - * Uninstall a plugin by removing it from config and optionally deleting installed files. + * Plan a plugin uninstall by removing it from config and resolving a safe file-removal target. * Linked plugins (source === "path") never have their source directory deleted. */ -export async function uninstallPlugin( - params: UninstallPluginParams, -): Promise { +export function planPluginUninstall(params: UninstallPluginParams): PluginUninstallPlanResult { const { config, pluginId, channelIds, deleteFiles = true, extensionsDir } = params; // Validate plugin exists @@ -363,7 +394,6 @@ export async function uninstallPlugin( ...configActions, directory: false, }; - const warnings: string[] = []; const deleteTarget = deleteFiles && !isLinked @@ -375,29 +405,56 @@ export async function uninstallPlugin( }) : null; - // Delete installed directory if requested and safe. - if (deleteTarget) { - const existed = - (await fs - .access(deleteTarget) - .then(() => true) - .catch(() => false)) ?? false; - try { - await fs.rm(deleteTarget, { recursive: true, force: true }); - actions.directory = existed; - } catch (error) { - warnings.push( - `Failed to remove plugin directory ${deleteTarget}: ${formatErrorMessage(error)}`, - ); - // Directory deletion failure is not fatal; config is the source of truth. - } - } - return { ok: true, config: newConfig, pluginId, actions, - warnings, + directoryRemoval: deleteTarget ? { target: deleteTarget } : null, + }; +} + +export async function applyPluginUninstallDirectoryRemoval( + removal: PluginUninstallDirectoryRemoval | null, +): Promise<{ directoryRemoved: boolean; warnings: string[] }> { + if (!removal) { + return { directoryRemoved: false, warnings: [] }; + } + + const existed = + (await fs + .access(removal.target) + .then(() => true) + .catch(() => false)) ?? false; + try { + await fs.rm(removal.target, { recursive: true, force: true }); + return { directoryRemoved: existed, warnings: [] }; + } catch (error) { + return { + directoryRemoved: false, + warnings: [ + `Failed to remove plugin directory ${removal.target}: ${formatErrorMessage(error)}`, + ], + }; + } +} + +export async function uninstallPlugin( + params: UninstallPluginParams, +): Promise { + const plan = planPluginUninstall(params); + if (!plan.ok) { + return plan; + } + const directory = await applyPluginUninstallDirectoryRemoval(plan.directoryRemoval); + return { + ok: true, + config: plan.config, + pluginId: plan.pluginId, + actions: { + ...plan.actions, + directory: directory.directoryRemoved, + }, + warnings: directory.warnings, }; }