fix(plugins): make uninstall teardown idempotent

vincentkoc · vincentkoc · commit 336303e48bf8 · 2026-05-02T10:51:02.000-07:00
diff --git a/src/cli/plugins-cli.uninstall.test.ts b/src/cli/plugins-cli.uninstall.test.ts
@@ -271,6 +271,53 @@ describe("plugins cli uninstall", () => {
     });
   });
 
+  it("cleans stale policy refs even when plugin is absent from the current registry", async () => {
+    const baseConfig = {
+      plugins: {
+        allow: ["alpha", "beta"],
+        deny: ["alpha"],
+      },
+    } as OpenClawConfig;
+    const nextConfig = {
+      plugins: {
+        allow: ["beta"],
+      },
+    } as OpenClawConfig;
+
+    loadConfig.mockReturnValue(baseConfig);
+    buildPluginSnapshotReport.mockReturnValue({
+      plugins: [],
+      diagnostics: [],
+    });
+    planPluginUninstall.mockReturnValue({
+      ok: true,
+      config: nextConfig,
+      actions: {
+        entry: false,
+        install: false,
+        allowlist: true,
+        denylist: true,
+        loadPath: false,
+        memorySlot: false,
+        contextEngineSlot: false,
+        channelConfig: false,
+        directory: false,
+      },
+      directoryRemoval: null,
+    });
+
+    await runPluginsCommand(["plugins", "uninstall", "alpha", "--force"]);
+
+    expect(planPluginUninstall).toHaveBeenCalledWith(
+      expect.objectContaining({
+        pluginId: "alpha",
+        deleteFiles: true,
+      }),
+    );
+    expect(writeConfigFile).toHaveBeenCalledWith(nextConfig);
+    expect(runtimeLogs.at(-2)).toContain('Uninstalled plugin "alpha"');
+  });
+
   it("exits when uninstall target is not managed by plugin install records", async () => {
     loadConfig.mockReturnValue({
       plugins: {
@@ -282,12 +329,16 @@ describe("plugins cli uninstall", () => {
       plugins: [{ id: "alpha", name: "alpha" }],
       diagnostics: [],
     });
+    planPluginUninstall.mockReturnValue({
+      ok: false,
+      error: "Plugin not found: alpha",
+    });
 
     await expect(runPluginsCommand(["plugins", "uninstall", "alpha", "--force"])).rejects.toThrow(
       "__exit__:1",
     );
 
     expect(runtimeErrors.at(-1)).toContain("is not managed by plugins config/install records");
-    expect(planPluginUninstall).not.toHaveBeenCalled();
+    expect(planPluginUninstall).toHaveBeenCalled();
   });
 });
diff --git a/src/cli/plugins-uninstall-command.ts b/src/cli/plugins-uninstall-command.ts
@@ -74,21 +74,6 @@ export async function runPluginUninstallCommand(
     config: cfg,
     plugins: report.plugins,
   });
-  const hasEntry = pluginId in (cfg.plugins?.entries ?? {});
-  const hasInstall = pluginId in (cfg.plugins?.installs ?? {});
-
-  if (!hasEntry && !hasInstall) {
-    if (plugin) {
-      runtime.error(
-        `Plugin "${pluginId}" is not managed by plugins config/install records and cannot be uninstalled.`,
-      );
-    } else {
-      runtime.error(`Plugin not found: ${id}`);
-    }
-    runtime.exit(1);
-    return;
-  }
-
   const channelIds = plugin?.status === "loaded" ? plugin.channelIds : undefined;
   const plan = planPluginUninstall({
     config: cfg,
@@ -98,10 +83,17 @@ export async function runPluginUninstallCommand(
     extensionsDir,
   });
   if (!plan.ok) {
-    runtime.error(plan.error);
+    if (plugin) {
+      runtime.error(
+        `Plugin "${pluginId}" is not managed by plugins config/install records and cannot be uninstalled.`,
+      );
+    } else {
+      runtime.error(plan.error);
+    }
     runtime.exit(1);
     return;
   }
+  const hasInstall = pluginId in (cfg.plugins?.installs ?? {});
 
   const preview: string[] = [];
   if (plan.actions.entry) {
diff --git a/src/plugins/uninstall.test.ts b/src/plugins/uninstall.test.ts
@@ -704,6 +704,37 @@ describe("uninstallPlugin", () => {
     }
   });
 
+  it("cleans stale policy references even when plugin code and install records are gone", async () => {
+    const result = await uninstallPlugin({
+      config: createPluginConfig({
+        allow: ["missing-plugin", "other-plugin"],
+        deny: ["missing-plugin"],
+        slots: {
+          memory: "missing-plugin",
+        },
+      }),
+      pluginId: "missing-plugin",
+      deleteFiles: true,
+    });
+
+    const successfulResult = expectSuccessfulUninstall(result);
+    expect(successfulResult.actions).toEqual({
+      entry: false,
+      install: false,
+      allowlist: true,
+      denylist: true,
+      loadPath: false,
+      memorySlot: true,
+      contextEngineSlot: false,
+      channelConfig: false,
+      directory: false,
+    });
+    expect(successfulResult.config.plugins?.allow).toEqual(["other-plugin"]);
+    expect(successfulResult.config.plugins?.deny).toBeUndefined();
+    expect(successfulResult.config.plugins?.slots?.memory).toBe("memory-core");
+    expect(runCommandWithTimeoutMock).not.toHaveBeenCalled();
+  });
+
   it("removes config entries", async () => {
     const config = createPluginConfig({
       entries: createSinglePluginEntries(),
@@ -829,6 +860,24 @@ describe("uninstallPlugin", () => {
     await expect(fs.access(pluginDir)).rejects.toThrow();
   });
 
+  it("skips npm cleanup when the managed package directory is already absent", async () => {
+    const stateDir = path.join(tempDir, "state");
+    const npmRoot = path.join(stateDir, "npm");
+    const pluginDir = path.join(npmRoot, "node_modules", "missing-plugin");
+
+    const applied = await applyPluginUninstallDirectoryRemoval({
+      target: pluginDir,
+      cleanup: {
+        kind: "npm",
+        npmRoot,
+        packageName: "missing-plugin",
+      },
+    });
+
+    expect(applied).toEqual({ directoryRemoved: false, warnings: [] });
+    expect(runCommandWithTimeoutMock).not.toHaveBeenCalled();
+  });
+
   it("warns and still removes npm package dirs when npm prune cleanup fails", async () => {
     runCommandWithTimeoutMock.mockResolvedValueOnce({
       code: 1,
diff --git a/src/plugins/uninstall.ts b/src/plugins/uninstall.ts
@@ -77,6 +77,10 @@ export function formatUninstallActionLabels(actions: UninstallActions): string[]
   );
 }
 
+function hasUninstallAction(actions: Omit<UninstallActions, "directory">): boolean {
+  return Object.values(actions).some(Boolean);
+}
+
 export function formatUninstallSlotResetPreview(slotKey: "memory" | "contextEngine"): string {
   const actionKey = slotKey === "memory" ? "memorySlot" : "contextEngineSlot";
   return `${UNINSTALL_ACTION_LABELS[actionKey]} (will reset to "${defaultSlotIdForKey(slotKey)}")`;
@@ -489,14 +493,8 @@ export type UninstallPluginParams = {
 export function planPluginUninstall(params: UninstallPluginParams): PluginUninstallPlanResult {
   const { config, pluginId, channelIds, deleteFiles = true, extensionsDir } = params;
 
-  // Validate plugin exists
   const hasEntry = pluginId in (config.plugins?.entries ?? {});
   const hasInstall = pluginId in (config.plugins?.installs ?? {});
-
-  if (!hasEntry && !hasInstall) {
-    return { ok: false, error: `Plugin not found: ${pluginId}` };
-  }
-
   const installRecord = config.plugins?.installs?.[pluginId];
   const isLinked = isLinkedPathInstallRecord(installRecord);
 
@@ -505,6 +503,10 @@ export function planPluginUninstall(params: UninstallPluginParams): PluginUninst
     channelIds,
   });
 
+  if (!hasEntry && !hasInstall && !hasUninstallAction(configActions)) {
+    return { ok: false, error: `Plugin not found: ${pluginId}` };
+  }
+
   const actions: UninstallActions = {
     ...configActions,
     directory: false,
@@ -563,6 +565,10 @@ export async function applyPluginUninstallDirectoryRemoval(
       .then(() => true)
       .catch(() => false)) ?? false;
   const warnings: string[] = [];
+  if (!existed) {
+    return { directoryRemoved: false, warnings };
+  }
+
   if (removal.cleanup?.kind === "npm") {
     const uninstall = await runCommandWithTimeout(
       [
