fix(plugins): make uninstall teardown idempotent

This commit is contained in:
Vincent Koc
2026-05-02 10:43:32 -07:00
parent edfef73ffc
commit 336303e48b
4 changed files with 121 additions and 23 deletions

View File

@@ -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();
});
});

View File

@@ -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) {

View File

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

View File

@@ -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(
[