From 0fe49c5731f31737b0e7ceab52528d93b01ceddd Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 9 Aug 2026 08:12:05 -0700 Subject: [PATCH] fix(plugins): prevent uninstalled npm plugins from coming back (#113902) * fix(plugins): preserve retained uninstall files * test(plugins): track retention temp dirs * fix(plugins): retain rollback marker snapshots --- scripts/e2e/lib/plugins/assertions.mjs | 18 +++ scripts/e2e/lib/plugins/sweep.sh | 10 ++ .../server-retained-plugin-cleanup.test.ts | 34 +++++ .../install-record-commit.retention.test.ts | 141 ++++++++++++++++++ src/plugins/install-record-commit.test.ts | 112 +++++++++++++- src/plugins/install-record-commit.ts | 108 +++++++++++--- src/plugins/managed-npm-retention-contract.ts | 2 + src/plugins/managed-npm-retention.test.ts | 37 ++++- src/plugins/managed-npm-retention.ts | 17 ++- 9 files changed, 458 insertions(+), 21 deletions(-) create mode 100644 src/gateway/server-retained-plugin-cleanup.test.ts create mode 100644 src/plugins/install-record-commit.retention.test.ts create mode 100644 src/plugins/managed-npm-retention-contract.ts diff --git a/scripts/e2e/lib/plugins/assertions.mjs b/scripts/e2e/lib/plugins/assertions.mjs index 799bf2ed1861..ee31385bbd9d 100644 --- a/scripts/e2e/lib/plugins/assertions.mjs +++ b/scripts/e2e/lib/plugins/assertions.mjs @@ -749,6 +749,23 @@ function assertNpmPluginRemoved() { } } +function assertNpmPluginRetained() { + const installPath = fs.readFileSync(scratchFile("plugins-npm-install-path.txt"), "utf8").trim(); + const dependencyPackagePath = fs + .readFileSync(scratchFile("plugins-npm-dependency-path.txt"), "utf8") + .trim(); + assertPluginRemoved({ + pluginId: "demo-plugin-npm", + listFile: scratchFile("plugins-npm-retained.json"), + }); + if (!fs.existsSync(installPath)) { + throw new Error(`npm managed package was deleted by --keep-files: ${installPath}`); + } + if (!fs.existsSync(dependencyPackagePath)) { + throw new Error(`npm managed dependency was deleted by --keep-files: ${dependencyPackagePath}`); + } +} + function assertInvalidOpenClawExtensionsRejected() { const pluginId = "demo-plugin-invalid-metadata"; for (const expected of ["openclaw.extensions[1]", "non-empty string"]) { @@ -1025,6 +1042,7 @@ const commands = { "plugin-file-removed": assertPluginFileRemoved, "plugin-npm": assertNpmPlugin, "plugin-npm-update": assertNpmPluginUpdateUnchanged, + "plugin-npm-retained": assertNpmPluginRetained, "plugin-npm-removed": assertNpmPluginRemoved, "invalid-openclaw-extensions": assertInvalidOpenClawExtensionsRejected, "bundle-disabled": assertClaudeBundleDisabled, diff --git a/scripts/e2e/lib/plugins/sweep.sh b/scripts/e2e/lib/plugins/sweep.sh index d673fd1624c1..972a8a49d934 100644 --- a/scripts/e2e/lib/plugins/sweep.sh +++ b/scripts/e2e/lib/plugins/sweep.sh @@ -223,6 +223,16 @@ node scripts/e2e/lib/plugins/assertions.mjs plugin-npm openclaw_e2e_maybe_timeout "$OPENCLAW_PLUGINS_CLI_TIMEOUT" node "$OPENCLAW_ENTRY" plugins update demo-plugin-npm >"$OPENCLAW_PLUGINS_TMP_DIR/plugins-npm-update.log" 2>&1 node scripts/e2e/lib/plugins/assertions.mjs plugin-npm-update +run_plugins_openclaw_logged uninstall-npm-retained plugins uninstall demo-plugin-npm --force --keep-files +run_plugins_openclaw_capture "$OPENCLAW_PLUGINS_TMP_DIR/plugins-npm-retained.json" plugins list --json +node scripts/e2e/lib/plugins/assertions.mjs plugin-npm-retained + +run_plugins_openclaw_logged reinstall-npm plugins install "npm:@openclaw/demo-plugin-npm@0.0.1" --force +run_plugins_openclaw_capture "$OPENCLAW_PLUGINS_TMP_DIR/plugins-npm.json" plugins list --json +run_plugins_openclaw_capture "$OPENCLAW_PLUGINS_TMP_DIR/plugins-npm-inspect.json" plugins inspect demo-plugin-npm --runtime --json +run_plugins_shell_logged exec-reinstalled-npm-plugin-cli 'node "$OPENCLAW_ENTRY" demo-npm ping >"$OPENCLAW_PLUGINS_TMP_DIR/plugins-npm-cli.txt"' +node scripts/e2e/lib/plugins/assertions.mjs plugin-npm + run_plugins_openclaw_logged uninstall-npm plugins uninstall demo-plugin-npm --force run_plugins_openclaw_capture "$OPENCLAW_PLUGINS_TMP_DIR/plugins-npm-uninstalled.json" plugins list --json node scripts/e2e/lib/plugins/assertions.mjs plugin-npm-removed diff --git a/src/gateway/server-retained-plugin-cleanup.test.ts b/src/gateway/server-retained-plugin-cleanup.test.ts new file mode 100644 index 000000000000..0040a7316b13 --- /dev/null +++ b/src/gateway/server-retained-plugin-cleanup.test.ts @@ -0,0 +1,34 @@ +import fs from "node:fs"; +import { expect, it, vi } from "vitest"; +import { RETAINED_MANAGED_NPM_KEEP_FILES_REASON } from "../plugins/managed-npm-retention-contract.js"; +import { + hasRetainedManagedNpmInstallMarker, + markRetainedManagedNpmInstall, +} from "../plugins/managed-npm-retention.js"; +import { writeManagedNpmPlugin } from "../plugins/test-helpers/managed-npm-plugin.js"; +import { withOpenClawTestState } from "../test-utils/openclaw-test-state.js"; +import { cleanupRetainedPluginInstallGenerations } from "./server-retained-plugin-cleanup.js"; + +it("preserves package files retained by plugin uninstall", async () => { + await withOpenClawTestState({ label: "gateway-retained-plugin-cleanup" }, async (state) => { + const packageDir = writeManagedNpmPlugin({ + stateDir: state.stateDir, + packageName: "@openclaw/kept-plugin", + pluginId: "kept-plugin", + version: "1.0.0", + }); + await markRetainedManagedNpmInstall({ + packageDir, + pluginId: "kept-plugin", + reason: RETAINED_MANAGED_NPM_KEEP_FILES_REASON, + }); + const log = { info: vi.fn(), warn: vi.fn() }; + + await cleanupRetainedPluginInstallGenerations({ log }); + + expect(fs.existsSync(packageDir)).toBe(true); + expect(hasRetainedManagedNpmInstallMarker(packageDir)).toBe(true); + expect(log.info).not.toHaveBeenCalled(); + expect(log.warn).not.toHaveBeenCalled(); + }); +}); diff --git a/src/plugins/install-record-commit.retention.test.ts b/src/plugins/install-record-commit.retention.test.ts new file mode 100644 index 000000000000..d48b774f7610 --- /dev/null +++ b/src/plugins/install-record-commit.retention.test.ts @@ -0,0 +1,141 @@ +import fs from "node:fs"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import type { PluginInstallRecord } from "../config/types.plugins.js"; +import { withOpenClawTestState } from "../test-utils/openclaw-test-state.js"; +import { commitPluginInstallRecordsWithConfig } from "./install-record-commit.js"; +import { listRecoveredManagedNpmInstallCandidates } from "./installed-plugin-index-record-reader.js"; +import { + cleanupRetainedManagedNpmInstallGenerations, + hasRetainedManagedNpmInstallMarker, +} from "./managed-npm-retention.js"; +import { writeManagedNpmPlugin } from "./test-helpers/managed-npm-plugin.js"; + +function npmRecord(packageName: string, installPath: string): PluginInstallRecord { + return { source: "npm", spec: `${packageName}@1.0.0`, installPath }; +} + +describe("retained managed npm record commits", () => { + it("suppresses recovery when a retained install record is removed", async () => { + await withOpenClawTestState({ label: "retained-record-removal" }, async (state) => { + const packageName = "@openclaw/retained-demo"; + const installPath = writeManagedNpmPlugin({ + stateDir: state.stateDir, + packageName, + pluginId: "retained-demo", + version: "1.0.0", + }); + expect( + listRecoveredManagedNpmInstallCandidates({ stateDir: state.stateDir }).map( + (candidate) => candidate.pluginId, + ), + ).toContain("retained-demo"); + + await commitPluginInstallRecordsWithConfig({ + previousInstallRecords: { "retained-demo": npmRecord(packageName, installPath) }, + nextInstallRecords: {}, + nextConfig: {}, + }); + + expect(hasRetainedManagedNpmInstallMarker(installPath)).toBe(true); + expect( + listRecoveredManagedNpmInstallCandidates({ stateDir: state.stateDir }).map( + (candidate) => candidate.pluginId, + ), + ).not.toContain("retained-demo"); + }); + }); + + it.each(["direct", "symlink"] as const)( + "does not retire a package still used by a %s active install path", + async (activePathKind) => { + await withOpenClawTestState({ label: `retained-active-${activePathKind}` }, async (state) => { + const packageName = "@openclaw/retained-active"; + const installPath = writeManagedNpmPlugin({ + stateDir: state.stateDir, + packageName, + pluginId: "retained-active", + version: "1.0.0", + }); + let activePath = installPath; + if (activePathKind === "symlink") { + activePath = state.statePath("active", "retained-active"); + fs.mkdirSync(path.dirname(activePath), { recursive: true }); + fs.symlinkSync(installPath, activePath, "dir"); + } + + await commitPluginInstallRecordsWithConfig({ + previousInstallRecords: { "retained-active": npmRecord(packageName, installPath) }, + nextInstallRecords: { + "active-alias": { + source: "path", + sourcePath: activePath, + installPath: activePath, + }, + }, + nextConfig: {}, + }); + + expect(hasRetainedManagedNpmInstallMarker(installPath)).toBe(false); + }); + }, + ); + + it("does not retire a removed npm record outside the managed npm root", async () => { + await withOpenClawTestState({ label: "retained-outside-root" }, async (state) => { + const outsideRoot = state.path("outside-root"); + try { + const packageName = "@openclaw/outside-retained"; + const installPath = writeManagedNpmPlugin({ + stateDir: outsideRoot, + packageName, + pluginId: "outside-retained", + version: "1.0.0", + }); + await commitPluginInstallRecordsWithConfig({ + previousInstallRecords: { "outside-retained": npmRecord(packageName, installPath) }, + nextInstallRecords: {}, + nextConfig: {}, + }); + expect(hasRetainedManagedNpmInstallMarker(installPath)).toBe(false); + } finally { + fs.rmSync(outsideRoot, { recursive: true, force: true }); + } + }); + }); + + it("keeps npm-to-local source changes cleanup-eligible", async () => { + await withOpenClawTestState({ label: "retained-source-change" }, async (state) => { + const packageName = "@openclaw/moved-local"; + const installPath = writeManagedNpmPlugin({ + stateDir: state.stateDir, + packageName, + pluginId: "moved-local", + version: "1.0.0", + }); + const localInstallPath = state.statePath("extensions", "moved-local"); + fs.mkdirSync(localInstallPath, { recursive: true }); + + await commitPluginInstallRecordsWithConfig({ + previousInstallRecords: { "moved-local": npmRecord(packageName, installPath) }, + nextInstallRecords: { + "moved-local": { + source: "path", + sourcePath: localInstallPath, + installPath: localInstallPath, + }, + }, + nextConfig: {}, + }); + + expect(hasRetainedManagedNpmInstallMarker(installPath)).toBe(true); + await expect( + cleanupRetainedManagedNpmInstallGenerations({ + activeInstallPaths: [localInstallPath], + }), + ).resolves.toBe(1); + expect(fs.existsSync(installPath)).toBe(false); + expect(fs.existsSync(localInstallPath)).toBe(true); + }); + }); +}); diff --git a/src/plugins/install-record-commit.test.ts b/src/plugins/install-record-commit.test.ts index 6aa164cfb277..0f5e0c97a83c 100644 --- a/src/plugins/install-record-commit.test.ts +++ b/src/plugins/install-record-commit.test.ts @@ -2,7 +2,8 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; import { createPluginInstallRecordMap, getPluginInstallRecordMapEntry, @@ -11,11 +12,16 @@ import { import type { OpenClawConfig } from "../config/types.openclaw.js"; import type { PluginInstallRecord } from "../config/types.plugins.js"; import { withEnvAsync } from "../test-utils/env.js"; +import { listRecoveredManagedNpmInstallCandidates } from "./installed-plugin-index-record-reader.js"; import type { InstalledPluginIndex } from "./installed-plugin-index.js"; import { hasRetainedManagedNpmInstallMarker, markRetainedManagedNpmInstall, + resolveRetainedManagedNpmInstallMarkerPath, } from "./managed-npm-retention.js"; +import { writeManagedNpmPlugin } from "./test-helpers/managed-npm-plugin.js"; + +const retentionTempDirs = useAutoCleanupTempDirTracker(afterEach); const mocks = vi.hoisted(() => { const lease = { @@ -480,6 +486,49 @@ describe("commitConfigWithPendingPluginInstalls", () => { } }); + it("removes a new retirement marker when the leased config commit rolls back", async () => { + const stateDir = retentionTempDirs.make("openclaw-record-commit-"); + const installPath = writeManagedNpmPlugin({ + stateDir, + packageName: "@openclaw/retained-rollback", + pluginId: "retained-rollback", + version: "1.0.0", + }); + const previousInstallRecords: Record = { + "retained-rollback": { + source: "npm", + spec: "@openclaw/retained-rollback@1.0.0", + installPath, + }, + }; + mocks.replaceConfigFile.mockRejectedValueOnce(new Error("config changed")); + + try { + await withEnvAsync({ OPENCLAW_STATE_DIR: stateDir }, async () => { + await expect( + commitPluginInstallRecordsWithConfig({ + previousInstallRecords, + nextInstallRecords: {}, + nextConfig: {}, + }), + ).rejects.toThrow("config changed"); + + expect(hasRetainedManagedNpmInstallMarker(installPath)).toBe(false); + expect( + listRecoveredManagedNpmInstallCandidates({ stateDir }).map( + (candidate) => candidate.pluginId, + ), + ).toContain("retained-rollback"); + expect(mocks.restorePersistedInstalledPluginIndexIfCurrent).toHaveBeenCalledWith(null, 1, { + filePath: mocks.lease.databasePath, + lease: mocks.lease, + }); + }); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } + }); + it("does not mark arbitrary npm paths outside the managed npm root", async () => { const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-record-commit-")); const outsideRoot = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-record-outside-")); @@ -805,6 +854,67 @@ describe("commitConfigWithPendingPluginInstalls", () => { } }); + it("restores earlier active markers when clearing a later marker fails", async () => { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-record-commit-")); + const installPaths = ["codex", "voice-call"].map((pluginId) => + path.join( + stateDir, + "npm", + "projects", + `${pluginId}-v2`, + "node_modules", + "@openclaw", + pluginId, + ), + ); + for (const [index, installPath] of installPaths.entries()) { + fs.mkdirSync(installPath, { recursive: true }); + await markRetainedManagedNpmInstall({ + packageDir: installPath, + pluginId: index === 0 ? "codex" : "voice-call", + retainedAt: "2026-04-25T00:00:00.000Z", + reason: "test-retained-generation", + }); + } + const laterMarkerPath = resolveRetainedManagedNpmInstallMarkerPath(installPaths[1] ?? ""); + const realRm = fs.promises.rm.bind(fs.promises); + const rmSpy = vi.spyOn(fs.promises, "rm").mockImplementation(async (target, options) => { + if (String(target) === laterMarkerPath) { + const error = new Error("marker clear failed") as NodeJS.ErrnoException; + error.code = "EIO"; + throw error; + } + return await realRm(target, options); + }); + + try { + await expect( + commitPluginInstallRecordsWithConfig({ + previousInstallRecords: {}, + nextInstallRecords: Object.fromEntries( + installPaths.map((installPath, index) => { + const pluginId = index === 0 ? "codex" : "voice-call"; + return [ + pluginId, + { + source: "npm", + spec: `@openclaw/${pluginId}@2.0.0`, + installPath, + }, + ]; + }), + ), + nextConfig: {}, + }), + ).rejects.toThrow("marker clear failed"); + + expect(installPaths.every(hasRetainedManagedNpmInstallMarker)).toBe(true); + } finally { + rmSpy.mockRestore(); + fs.rmSync(stateDir, { recursive: true, force: true }); + } + }); + it("rolls back plugin index writes when the config write fails", async () => { const existingRecords: Record = { existing: { diff --git a/src/plugins/install-record-commit.ts b/src/plugins/install-record-commit.ts index c2bb5966cdca..8c68c8e50c0a 100644 --- a/src/plugins/install-record-commit.ts +++ b/src/plugins/install-record-commit.ts @@ -21,6 +21,7 @@ import { import type { OpenClawConfig } from "../config/types.openclaw.js"; import type { PluginInstallRecord } from "../config/types.plugins.js"; import { isPathInside } from "../infra/path-guards.js"; +import { resolveDefaultPluginNpmDir, resolvePluginNpmProjectsDir } from "./install-paths.js"; import { loadInstalledPluginIndexInstallRecords, PLUGIN_INSTALLS_CONFIG_PATH, @@ -32,6 +33,7 @@ import { restorePersistedInstalledPluginIndexIfCurrent, type InstalledPluginIndexWriteReceipt, } from "./installed-plugin-index-store.js"; +import { RETAINED_MANAGED_NPM_KEEP_FILES_REASON } from "./managed-npm-retention-contract.js"; import { clearRetainedManagedNpmInstallMarker, markRetainedManagedNpmInstall, @@ -131,9 +133,26 @@ function mergeAfterWrite( }; } +function isMissingInstallPathError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException).code; + return code === "ENOENT" || code === "ENOTDIR"; +} + +function resolveExistingInstallPath(installPath: string): string { + const resolvedPath = path.resolve(installPath); + try { + return fs.realpathSync(resolvedPath); + } catch (error) { + if (isMissingInstallPathError(error)) { + return resolvedPath; + } + throw error; + } +} + function installPathsOverlap(left: string, right: string): boolean { - const resolvedLeft = path.resolve(left); - const resolvedRight = path.resolve(right); + const resolvedLeft = resolveExistingInstallPath(left); + const resolvedRight = resolveExistingInstallPath(right); return ( resolvedLeft === resolvedRight || isPathInside(resolvedLeft, resolvedRight) || @@ -146,18 +165,52 @@ function resolveRetainedManagedNpmInstallMarkerTarget(params: { previousRecord?: PluginInstallRecord; nextRecord?: PluginInstallRecord; }): string | null { - if (params.previousRecord?.source !== "npm" || params.nextRecord?.source !== "npm") { + if (params.previousRecord?.source !== "npm") { return null; } const previousInstallPath = params.previousRecord.installPath?.trim(); - const nextInstallPath = params.nextRecord.installPath?.trim(); - if (!previousInstallPath || !nextInstallPath) { + const nextInstallPath = params.nextRecord?.installPath?.trim(); + if (!previousInstallPath) { return null; } - if (installPathsOverlap(previousInstallPath, nextInstallPath)) { + if ( + params.nextRecord && + (!nextInstallPath || installPathsOverlap(previousInstallPath, nextInstallPath)) + ) { return null; } + if (params.nextRecord?.source !== "npm") { + const packageInfo = resolveRetainedManagedNpmInstallPackageInfo(previousInstallPath); + if (!packageInfo) { + return null; + } + try { + const configuredNpmRoot = path.resolve(resolveDefaultPluginNpmDir()); + const npmRoot = fs.realpathSync(configuredNpmRoot); + const configuredProjectRoot = path.resolve(packageInfo.projectRoot); + const projectRoot = fs.realpathSync(configuredProjectRoot); + const packageDir = fs.realpathSync(previousInstallPath); + if ( + path.relative(configuredNpmRoot, configuredProjectRoot) !== + path.relative(npmRoot, projectRoot) || + path.relative(configuredProjectRoot, path.resolve(previousInstallPath)) !== + path.relative(projectRoot, packageDir) + ) { + return null; + } + if (projectRoot === npmRoot) { + return previousInstallPath; + } + const projectsRoot = fs.realpathSync(resolvePluginNpmProjectsDir(npmRoot)); + return path.dirname(projectRoot) === projectsRoot ? previousInstallPath : null; + } catch (error) { + if (isMissingInstallPathError(error)) { + return null; + } + throw error; + } + } const installs = createPluginInstallRecordMap(); setPluginInstallRecordMapEntry(installs, params.pluginId, params.previousRecord); const plan = planPluginUninstall({ @@ -177,7 +230,7 @@ function resolveRetainedManagedNpmInstallMarkerTarget(params: { ) { return null; } - if (installPathsOverlap(plan.directoryRemoval.target, nextInstallPath)) { + if (nextInstallPath && installPathsOverlap(plan.directoryRemoval.target, nextInstallPath)) { return null; } return plan.directoryRemoval.target; @@ -206,17 +259,30 @@ function findReplacementNpmRecordForRemovedRecord(params: { return null; } -async function markRetainedReplacedManagedNpmInstallRecords(params: { +async function markRetiredManagedNpmInstallRecords(params: { previousInstallRecords: Record; nextInstallRecords: Record; createdMarkerPaths: string[]; }): Promise { const markedPreviousPluginIds = new Set(); - const markReplacement = async ( + const activeInstallPaths = Object.values(params.nextInstallRecords).flatMap((record) => { + const installPath = record.installPath?.trim(); + return installPath ? [installPath] : []; + }); + const markRetiredInstall = async ( pluginId: string, previousRecord: PluginInstallRecord | undefined, nextRecord: PluginInstallRecord | undefined, ) => { + const previousInstallPath = previousRecord?.installPath?.trim(); + if ( + previousInstallPath && + activeInstallPaths.some((installPath) => + installPathsOverlap(previousInstallPath, installPath), + ) + ) { + return; + } const packageDir = resolveRetainedManagedNpmInstallMarkerTarget({ pluginId, previousRecord, @@ -230,7 +296,12 @@ async function markRetainedReplacedManagedNpmInstallRecords(params: { const marked = await markRetainedManagedNpmInstall({ packageDir, pluginId, - reason: "replaced-by-managed-npm-generation-update", + reason: + nextRecord?.source === "npm" + ? "replaced-by-managed-npm-generation-update" + : nextRecord + ? "replaced-by-plugin-source-change" + : RETAINED_MANAGED_NPM_KEEP_FILES_REASON, }); if (marked && !markerAlreadyExisted) { // Record each marker immediately so a later filesystem failure can roll it back. @@ -240,7 +311,7 @@ async function markRetainedReplacedManagedNpmInstallRecords(params: { }; for (const [pluginId, nextRecord] of Object.entries(params.nextInstallRecords)) { - await markReplacement( + await markRetiredInstall( pluginId, getPluginInstallRecordMapEntry(params.previousInstallRecords, pluginId), nextRecord, @@ -253,7 +324,7 @@ async function markRetainedReplacedManagedNpmInstallRecords(params: { ) { continue; } - await markReplacement( + await markRetiredInstall( pluginId, previousRecord, findReplacementNpmRecordForRemovedRecord({ @@ -272,8 +343,8 @@ async function removeCreatedRetainedManagedNpmInstallMarkers(markerPaths: string async function clearActiveRetainedManagedNpmInstallMarkers( nextInstallRecords: Record, -): Promise> { - const clearedMarkers: Array<{ markerPath: string; contents: string }> = []; + clearedMarkers: Array<{ markerPath: string; contents: string }>, +): Promise { for (const record of Object.values(nextInstallRecords)) { if (record.source !== "npm" || !record.installPath?.trim()) { continue; @@ -295,10 +366,10 @@ async function clearActiveRetainedManagedNpmInstallMarkers( } const cleared = await clearRetainedManagedNpmInstallMarker(record.installPath); if (cleared) { + // Record each cleared marker immediately so a later filesystem failure can roll it back. clearedMarkers.push({ markerPath, contents }); } } - return clearedMarkers; } async function restoreClearedRetainedManagedNpmInstallMarkers( @@ -337,14 +408,15 @@ async function commitPluginInstallRecordsWithWriter(params: { lease, }, ); - await markRetainedReplacedManagedNpmInstallRecords({ + await markRetiredManagedNpmInstallRecords({ previousInstallRecords: prepared.previousInstallRecords, nextInstallRecords: prepared.nextInstallRecords, // Keep partial progress visible to the rollback path. createdMarkerPaths: retainedMarkerPaths, }); - clearedMarkerSnapshots.push( - ...(await clearActiveRetainedManagedNpmInstallMarkers(prepared.nextInstallRecords)), + await clearActiveRetainedManagedNpmInstallMarkers( + prepared.nextInstallRecords, + clearedMarkerSnapshots, ); const installRecordsChanged = !pluginInstallRecordMapsEqual( prepared.previousInstallRecords, diff --git a/src/plugins/managed-npm-retention-contract.ts b/src/plugins/managed-npm-retention-contract.ts new file mode 100644 index 000000000000..7f5b00bc02b1 --- /dev/null +++ b/src/plugins/managed-npm-retention-contract.ts @@ -0,0 +1,2 @@ +/** Marker reason for packages preserved by an explicit `plugins uninstall --keep-files`. */ +export const RETAINED_MANAGED_NPM_KEEP_FILES_REASON = "removed-managed-npm-install-retained"; diff --git a/src/plugins/managed-npm-retention.test.ts b/src/plugins/managed-npm-retention.test.ts index 3069ee49511c..57d22863faf0 100644 --- a/src/plugins/managed-npm-retention.test.ts +++ b/src/plugins/managed-npm-retention.test.ts @@ -1,8 +1,12 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; import { resolvePluginNpmGenerationProjectDir } from "./install-paths.js"; +import { RETAINED_MANAGED_NPM_KEEP_FILES_REASON } from "./managed-npm-retention-contract.js"; + +const retentionTempDirs = useAutoCleanupTempDirTracker(afterEach); import { cleanupRetainedManagedNpmInstallGenerations, hasRetainedManagedNpmInstallMarker, @@ -72,4 +76,35 @@ describe("managed npm retention", () => { fs.rmSync(stateDir, { recursive: true, force: true }); } }); + + it.each(["project", "legacy"] as const)( + "preserves %s packages retained by an explicit keep-files uninstall", + async (layout) => { + const stateDir = retentionTempDirs.make("openclaw-retention-"); + const npmDir = path.join(stateDir, "npm"); + const projectRoot = + layout === "legacy" + ? npmDir + : resolvePluginNpmGenerationProjectDir({ + npmDir, + packageName: "@openclaw/kept-plugin", + generationKey: "kept-plugin-v1", + }); + const packageDir = path.join(projectRoot, "node_modules", "@openclaw", "kept-plugin"); + fs.mkdirSync(packageDir, { recursive: true }); + await markRetainedManagedNpmInstall({ + packageDir, + pluginId: "kept-plugin", + reason: RETAINED_MANAGED_NPM_KEEP_FILES_REASON, + }); + + try { + await expect(cleanupRetainedManagedNpmInstallGenerations({ npmDir })).resolves.toBe(0); + expect(fs.existsSync(packageDir)).toBe(true); + expect(hasRetainedManagedNpmInstallMarker(packageDir)).toBe(true); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } + }, + ); }); diff --git a/src/plugins/managed-npm-retention.ts b/src/plugins/managed-npm-retention.ts index e083941b8571..28d8e14a8bb1 100644 --- a/src/plugins/managed-npm-retention.ts +++ b/src/plugins/managed-npm-retention.ts @@ -1,12 +1,23 @@ -// Marks retained managed npm package trees that should stay importable but not recoverable. +// Marks managed npm packages excluded from recovery and classifies cleanup eligibility. import fs from "node:fs"; import path from "node:path"; +import { isRecord } from "@openclaw/normalization-core/record-coerce"; import { safePathSegmentHashed } from "../infra/install-safe-path.js"; import { resolveDefaultPluginNpmDir, resolvePluginNpmProjectsDir } from "./install-paths.js"; +import { RETAINED_MANAGED_NPM_KEEP_FILES_REASON } from "./managed-npm-retention-contract.js"; import { listManagedPluginNpmRootsSync } from "./npm-project-roots.js"; const RETAINED_MANAGED_NPM_INSTALL_MARKER_DIR = ".openclaw-retained-npm-installs"; +function markerPreservesPackageFiles(markerPath: string): boolean { + try { + const marker: unknown = JSON.parse(fs.readFileSync(markerPath, "utf8")); + return isRecord(marker) && marker.reason === RETAINED_MANAGED_NPM_KEEP_FILES_REASON; + } catch { + return false; + } +} + export function resolveRetainedManagedNpmInstallPackageInfo(packageDir: string): { packageName: string; projectRoot: string; @@ -150,6 +161,7 @@ async function cleanupRetainedLegacyNpmPackages(params: { for (const packageDir of listManagedNpmPackageDirs(params.npmRoot)) { if ( !hasRetainedManagedNpmInstallMarker(packageDir) || + markerPreservesPackageFiles(resolveRetainedManagedNpmInstallMarkerPath(packageDir)) || params.activeInstallPaths.some((installPath) => isPathEqualOrInside(packageDir, installPath)) ) { continue; @@ -205,6 +217,9 @@ export async function cleanupRetainedManagedNpmInstallGenerations( } if ( markerEntries.length === 0 || + markerEntries.some((entry) => + markerPreservesPackageFiles(path.join(markerDir, entry.name)), + ) || !isPathEqualOrInside(projectsDir, projectRoot) || activeInstallPaths.some((installPath) => isPathEqualOrInside(projectRoot, installPath)) ) {