From a7c5b2c6e6bd0004fe135bb4e32f02cd8cf78a58 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 10 Jul 2026 06:07:41 +0100 Subject: [PATCH] fix(onboarding): preserve plugin ownership from included config (#103372) * fix(onboarding): preserve included plugin install records * fix(config): validate inherited plugin install records * fix(onboarding): preserve canonical plugin install ownership * test(onboarding): cover completed install migrations --- src/cli/plugins-install-record-commit.test.ts | 89 +++++++++++++++++++ src/cli/plugins-install-record-commit.ts | 21 ++++- .../chat-engine.channel-hooks.test.ts | 2 +- src/crestodian/chat-engine.ts | 1 + src/crestodian/setup-apply.ts | 5 +- src/wizard/setup.shared.test.ts | 77 ++++++++++++++++ src/wizard/setup.shared.ts | 8 ++ 7 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 src/wizard/setup.shared.test.ts diff --git a/src/cli/plugins-install-record-commit.test.ts b/src/cli/plugins-install-record-commit.test.ts index 6e546f0c6781..3e27e19e14df 100644 --- a/src/cli/plugins-install-record-commit.test.ts +++ b/src/cli/plugins-install-record-commit.test.ts @@ -14,11 +14,14 @@ import { withEnvAsync } from "../test-utils/env.js"; const mocks = vi.hoisted(() => ({ loadInstalledPluginIndexInstallRecords: vi.fn(), replaceConfigFile: vi.fn(), + transformConfigFileWithRetry: vi.fn(), writePersistedInstalledPluginIndexInstallRecords: vi.fn(), })); vi.mock("../config/config.js", () => ({ replaceConfigFile: mocks.replaceConfigFile, + resolveConfigWriteAfterWrite: (value?: unknown) => value ?? { mode: "auto" }, + transformConfigFileWithRetry: mocks.transformConfigFileWithRetry, })); vi.mock("../plugins/installed-plugin-index-records.js", async (importOriginal) => { @@ -37,6 +40,7 @@ import { commitConfigWriteWithPendingPluginInstalls, commitPluginInstallRecordsWithConfig, stripPendingPluginInstallRecords, + transformConfigWithPendingPluginInstalls, unchangedPendingPluginInstallRecordIds, } from "./plugins-install-record-commit.js"; @@ -119,6 +123,91 @@ describe("commitConfigWithPendingPluginInstalls", () => { }); }); + it("migrates source records below the canonical index and explicit pending records", async () => { + const sourceConfig: OpenClawConfig = { + plugins: { + installs: { + stale: { source: "npm", spec: "stale@1.0.0" }, + missing: { source: "npm", spec: "missing@1.0.0" }, + codex: { source: "npm", spec: "codex@1.0.0" }, + }, + }, + }; + const existingRecords: Record = { + stale: { source: "npm", spec: "stale@2.0.0" }, + codex: { source: "npm", spec: "codex@2.0.0" }, + }; + const nextConfig: OpenClawConfig = { + plugins: { + installs: { + ...sourceConfig.plugins?.installs, + codex: { source: "npm", spec: "codex@3.0.0" }, + concurrent: { source: "npm", spec: "concurrent@1.0.0" }, + }, + }, + }; + const commit = vi.fn(async () => undefined); + mocks.loadInstalledPluginIndexInstallRecords.mockResolvedValue(existingRecords); + + const result = await commitConfigWriteWithPendingPluginInstalls({ + nextConfig, + sourceConfig, + commit, + }); + + expect(mocks.writePersistedInstalledPluginIndexInstallRecords).toHaveBeenCalledWith({ + stale: existingRecords.stale, + missing: sourceConfig.plugins?.installs?.missing, + codex: nextConfig.plugins?.installs?.codex, + concurrent: nextConfig.plugins?.installs?.concurrent, + }); + expect(commit).toHaveBeenCalledWith({}, { + afterWrite: { mode: "restart", reason: "plugin source changed" }, + unsetPaths: [["plugins", "installs"]], + }); + expect(result.installRecords).toStrictEqual({ + stale: existingRecords.stale, + missing: sourceConfig.plugins?.installs?.missing, + codex: nextConfig.plugins?.installs?.codex, + concurrent: nextConfig.plugins?.installs?.concurrent, + }); + }); + + it("preserves source records omitted by a transform callback", async () => { + const sourceConfig: OpenClawConfig = { + plugins: { + installs: { + other: { source: "npm", spec: "other@1.0.0" }, + }, + }, + }; + const codexRecord: PluginInstallRecord = { source: "npm", spec: "codex@2.0.0" }; + const snapshot = { sourceConfig }; + mocks.transformConfigFileWithRetry.mockImplementationOnce(async (params: unknown) => { + const transformParams = params as { + transform: ( + config: OpenClawConfig, + context: { snapshot: typeof snapshot }, + ) => { nextConfig: OpenClawConfig }; + commit: (input: unknown) => Promise; + }; + const transformed = transformParams.transform(sourceConfig, { snapshot }); + await transformParams.commit({ nextConfig: transformed.nextConfig, snapshot }); + return {}; + }); + + await transformConfigWithPendingPluginInstalls({ + transform: () => ({ + nextConfig: { plugins: { installs: { codex: codexRecord } } }, + }), + }); + + expect(mocks.writePersistedInstalledPluginIndexInstallRecords).toHaveBeenCalledWith({ + other: sourceConfig.plugins?.installs?.other, + codex: codexRecord, + }); + }); + it("strips only selected pending plugin install records", () => { const config: OpenClawConfig = { plugins: { diff --git a/src/cli/plugins-install-record-commit.ts b/src/cli/plugins-install-record-commit.ts index 6a0248213cf1..9f2ab519f6ba 100644 --- a/src/cli/plugins-install-record-commit.ts +++ b/src/cli/plugins-install-record-commit.ts @@ -12,6 +12,7 @@ import { type TransformConfigFileWithRetryParams, } from "../config/config.js"; import type { ConfigWriteOptions } from "../config/io.js"; +import { extractShippedPluginInstallConfigRecords } from "../config/plugin-install-config-migration.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import type { PluginInstallRecord } from "../config/types.plugins.js"; import { isPathInside } from "../infra/path-guards.js"; @@ -349,6 +350,8 @@ export async function commitPluginInstallRecordsWithConfig(params: { /** Commit config while migrating any pending install records into the install index. */ export async function commitConfigWriteWithPendingPluginInstalls(params: { nextConfig: OpenClawConfig; + /** Source snapshot whose transient records migrate below the canonical index. */ + sourceConfig?: OpenClawConfig; writeOptions?: ConfigWriteOptions; commit: ConfigCommit; }): Promise<{ @@ -357,7 +360,19 @@ export async function commitConfigWriteWithPendingPluginInstalls(params: { movedInstallRecords: boolean; persistedHash: string | null; }> { - if (!hasPendingPluginInstallRecords(params.nextConfig)) { + const sourceInstallRecords = extractShippedPluginInstallConfigRecords(params.sourceConfig); + const nextPendingConfig = params.sourceConfig + ? stripPendingPluginInstallRecords( + params.nextConfig, + unchangedPendingPluginInstallRecordIds(params.nextConfig, { + plugins: { installs: sourceInstallRecords }, + }), + ) + : params.nextConfig; + if ( + Object.keys(sourceInstallRecords).length === 0 && + !hasPendingPluginInstallRecords(nextPendingConfig) + ) { const committed = params.writeOptions ? await params.commit(params.nextConfig, params.writeOptions) : await params.commit(params.nextConfig); @@ -369,9 +384,10 @@ export async function commitConfigWriteWithPendingPluginInstalls(params: { }; } - const pendingInstallRecords = params.nextConfig.plugins?.installs ?? {}; + const pendingInstallRecords = nextPendingConfig.plugins?.installs ?? {}; const previousInstallRecords = await loadInstalledPluginIndexInstallRecords(); const nextInstallRecords = { + ...sourceInstallRecords, ...previousInstallRecords, ...pendingInstallRecords, }; @@ -423,6 +439,7 @@ export async function transformConfigWithPendingPluginInstalls( const requestedAfterWrite = params.afterWrite ?? params.writeOptions?.afterWrite; const committed = await commitConfigWriteWithPendingPluginInstalls({ nextConfig, + sourceConfig: snapshot.sourceConfig, ...(writeOptions ? { writeOptions: mergeAfterWrite(writeOptions, params.afterWrite) } : {}), commit: async (config, commitWriteOptions) => { return await replaceConfigFile({ diff --git a/src/crestodian/chat-engine.channel-hooks.test.ts b/src/crestodian/chat-engine.channel-hooks.test.ts index fc25dc5f7861..b1e552c3777a 100644 --- a/src/crestodian/chat-engine.channel-hooks.test.ts +++ b/src/crestodian/chat-engine.channel-hooks.test.ts @@ -88,7 +88,7 @@ describe("Crestodian chat channel setup", () => { expect(reply.text).toContain("matrix is configured"); expect(mocks.writeWizardConfigFile).toHaveBeenCalledWith( { channels: { matrix: { enabled: true } } }, - { allowConfigSizeDrop: false }, + { allowConfigSizeDrop: false, migrationBaseConfig: {} }, ); expect(mocks.runCollectedChannelOnboardingPostWriteHooks).toHaveBeenCalledWith({ hooks: [mocks.hook], diff --git a/src/crestodian/chat-engine.ts b/src/crestodian/chat-engine.ts index b620c1b0bbd2..7b2ba99504c3 100644 --- a/src/crestodian/chat-engine.ts +++ b/src/crestodian/chat-engine.ts @@ -141,6 +141,7 @@ function defaultChannelSetupWizardRunner( }); const committedConfig = await writeWizardConfigFile(nextConfig, { allowConfigSizeDrop: false, + migrationBaseConfig: baseConfig, }); await runCollectedChannelOnboardingPostWriteHooks({ hooks: postWriteHooks.drain(), diff --git a/src/crestodian/setup-apply.ts b/src/crestodian/setup-apply.ts index 0bce66ea8432..42614998fd4f 100644 --- a/src/crestodian/setup-apply.ts +++ b/src/crestodian/setup-apply.ts @@ -183,7 +183,10 @@ export async function applyCrestodianSetup( command: "onboard", mode: "local", }); - nextConfig = await writeWizardConfigFile(nextConfig, { allowConfigSizeDrop: false }); + nextConfig = await writeWizardConfigFile(nextConfig, { + allowConfigSizeDrop: false, + migrationBaseConfig: baseConfig, + }); await onboardHelpers.ensureWorkspaceAndSessions(workspace, runtime, { skipBootstrap: Boolean(nextConfig.agents?.defaults?.skipBootstrap), diff --git a/src/wizard/setup.shared.test.ts b/src/wizard/setup.shared.test.ts new file mode 100644 index 000000000000..a6343775d6bb --- /dev/null +++ b/src/wizard/setup.shared.test.ts @@ -0,0 +1,77 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { withoutPluginInstallRecords } from "../plugins/installed-plugin-index-records.js"; + +const mocks = vi.hoisted(() => ({ + commitConfigWriteWithPendingPluginInstalls: vi.fn(), +})); + +vi.mock("../cli/plugins-install-record-commit.js", async (importOriginal) => ({ + ...(await importOriginal()), + commitConfigWriteWithPendingPluginInstalls: + mocks.commitConfigWriteWithPendingPluginInstalls, +})); + +import { writeWizardConfigFile } from "./setup.shared.js"; + +describe("writeWizardConfigFile pending install ownership", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.commitConfigWriteWithPendingPluginInstalls.mockImplementation( + async (params: { nextConfig: OpenClawConfig }) => ({ + config: withoutPluginInstallRecords(params.nextConfig), + installRecords: {}, + movedInstallRecords: true, + persistedHash: "test-hash", + }), + ); + }); + + it("rejects a normal write with pending records but no migration base", async () => { + const config: OpenClawConfig = { + plugins: { installs: { demo: { source: "npm", spec: "demo@1.0.0" } } }, + }; + + await expect( + writeWizardConfigFile(config, { allowConfigSizeDrop: false }), + ).rejects.toThrow("declare migration ownership"); + expect(mocks.commitConfigWriteWithPendingPluginInstalls).not.toHaveBeenCalled(); + }); + + it("migrates the baseline as source before the final wizard write", async () => { + const baseConfig: OpenClawConfig = { + plugins: { installs: { demo: { source: "npm", spec: "demo@1.0.0" } } }, + }; + + await writeWizardConfigFile(baseConfig, { + allowConfigSizeDrop: false, + migrationBaseConfig: baseConfig, + }); + + expect(mocks.commitConfigWriteWithPendingPluginInstalls).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + nextConfig: baseConfig, + sourceConfig: baseConfig, + writeOptions: { allowConfigSizeDrop: true }, + }), + ); + expect(mocks.commitConfigWriteWithPendingPluginInstalls).toHaveBeenCalledTimes(2); + }); + + it("commits fresh pending records after baseline migration is complete", async () => { + const config: OpenClawConfig = { + plugins: { installs: { fresh: { source: "npm", spec: "fresh@1.0.0" } } }, + }; + + await writeWizardConfigFile(config, { + allowConfigSizeDrop: false, + migrationBaseConfig: undefined, + }); + + expect(mocks.commitConfigWriteWithPendingPluginInstalls).toHaveBeenCalledOnce(); + expect(mocks.commitConfigWriteWithPendingPluginInstalls).toHaveBeenCalledWith( + expect.objectContaining({ nextConfig: config }), + ); + }); +}); diff --git a/src/wizard/setup.shared.ts b/src/wizard/setup.shared.ts index 7d447abb5289..c2a2073d6ac5 100644 --- a/src/wizard/setup.shared.ts +++ b/src/wizard/setup.shared.ts @@ -63,10 +63,18 @@ export async function writeWizardConfigFile( let config = configInput; const allowConfigSizeDrop = opts.allowConfigSizeDrop === true; if (!allowConfigSizeDrop && hasPendingPluginInstallRecords(config)) { + // Explicit undefined means this writer already migrated its baseline; an omitted + // key cannot distinguish fresh pending records from stale authored metadata. + if (!Object.hasOwn(opts, "migrationBaseConfig")) { + throw new Error( + "Wizard config writes with pending plugin installs must declare migration ownership.", + ); + } const migrationBaseConfig = opts.migrationBaseConfig; if (migrationBaseConfig && hasPendingPluginInstallRecords(migrationBaseConfig)) { await commitConfigWriteWithPendingPluginInstalls({ nextConfig: migrationBaseConfig, + sourceConfig: migrationBaseConfig, writeOptions: { allowConfigSizeDrop: true }, commit: async (nextConfig, writeOptions) => { return await replaceConfigFile({