fix: harden plugin install and uninstall transactions

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",

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<UninstallPluginResult> {
+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<UninstallPluginResult> {
+  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,
   };
 }