From 227230024d06dc4367dc0d572767e2e5214aa2bd Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 18 Aug 2026 02:42:09 -0700 Subject: [PATCH] fix(configure): persist changes before side effects (#125751) Local mode is persisted before health and daemon actions, and failed actions no longer receive a bare success outro. --- src/commands/configure.channels.test.ts | 16 +- src/commands/configure.channels.ts | 9 +- src/commands/configure.daemon.test.ts | 23 ++- src/commands/configure.daemon.ts | 9 +- .../configure.wizard.persistence.test.ts | 151 ++++++++++++++++++ src/commands/configure.wizard.test.ts | 1 + src/commands/configure.wizard.ts | 57 +++++-- 7 files changed, 225 insertions(+), 41 deletions(-) create mode 100644 src/commands/configure.wizard.persistence.test.ts diff --git a/src/commands/configure.channels.test.ts b/src/commands/configure.channels.test.ts index 0a2950054ba3..dae0208e008f 100644 --- a/src/commands/configure.channels.test.ts +++ b/src/commands/configure.channels.test.ts @@ -84,8 +84,8 @@ function expectUnknownChannelRemovalPrompt(unsafeChannel: string, label: string) `Delete ${label} configuration from ~/.openclaw/openclaw.json?`, ); expect(note).toHaveBeenCalledWith( - `${label} removed from config.\nNote: credentials/sessions on disk are unchanged.`, - "Channel removed", + `${label} selected for removal from config.\nNote: credentials/sessions on disk are unchanged.`, + "Channel removal", ); } @@ -143,8 +143,8 @@ describe("removeChannelConfigWizard", () => { ); expect(next.channels).toEqual({ twitch: { token: "secret" } }); expect(note).toHaveBeenCalledWith( - "Telegram removed from config.\nNote: credentials/sessions on disk are unchanged.", - "Channel removed", + "Telegram selected for removal from config.\nNote: credentials/sessions on disk are unchanged.", + "Channel removal", ); }); @@ -164,8 +164,8 @@ describe("removeChannelConfigWizard", () => { expect(confirmArg().message).toBe("Delete done configuration from ~/.openclaw/openclaw.json?"); expect(next.channels).toEqual({ telegram: { token: "secret" } }); expect(note).toHaveBeenCalledWith( - "done removed from config.\nNote: credentials/sessions on disk are unchanged.", - "Channel removed", + "done selected for removal from config.\nNote: credentials/sessions on disk are unchanged.", + "Channel removal", ); }); @@ -231,8 +231,8 @@ describe("removeChannelConfigWizard", () => { "Delete Telegram\\nBot configuration from ~/.openclaw/openclaw.json?", ); expect(note).toHaveBeenCalledWith( - "Telegram\\nBot removed from config.\nNote: credentials/sessions on disk are unchanged.", - "Channel removed", + "Telegram\\nBot selected for removal from config.\nNote: credentials/sessions on disk are unchanged.", + "Channel removal", ); }); diff --git a/src/commands/configure.channels.ts b/src/commands/configure.channels.ts index 1cd99dffbde9..ea50958a0936 100644 --- a/src/commands/configure.channels.ts +++ b/src/commands/configure.channels.ts @@ -130,10 +130,11 @@ export async function removeChannelConfigWizard( } note( - [`${label} removed from config.`, "Note: credentials/sessions on disk are unchanged."].join( - "\n", - ), - "Channel removed", + [ + `${label} selected for removal from config.`, + "Note: credentials/sessions on disk are unchanged.", + ].join("\n"), + "Channel removal", ); } } diff --git a/src/commands/configure.daemon.test.ts b/src/commands/configure.daemon.test.ts index f0dd9a9f25fc..0d927e6c24a3 100644 --- a/src/commands/configure.daemon.test.ts +++ b/src/commands/configure.daemon.test.ts @@ -121,11 +121,12 @@ describe("maybeInstallDaemon", () => { warnings: [], }); - await maybeInstallDaemon({ + const outcome = await maybeInstallDaemon({ runtime: { log: vi.fn(), error: vi.fn(), exit: vi.fn() }, port: 18789, }); + expect(outcome).toBe("failed"); expect(note).toHaveBeenCalledWith( "Gateway service install failed: Gateway install blocked: gateway.auth.token SecretRef is configured but unresolved (boom). Fix gateway auth config/token input and rerun configure.", "Gateway", @@ -139,12 +140,10 @@ describe("maybeInstallDaemon", () => { new Error("systemctl is-enabled unavailable: Failed to connect to bus"), ); - await expect( - maybeInstallDaemon({ - runtime: { log: vi.fn(), error: vi.fn(), exit: vi.fn() }, - port: 18789, - }), - ).resolves.toBeUndefined(); + await maybeInstallDaemon({ + runtime: { log: vi.fn(), error: vi.fn(), exit: vi.fn() }, + port: 18789, + }); expect(serviceInstall).toHaveBeenCalledTimes(1); }); @@ -169,12 +168,10 @@ describe("maybeInstallDaemon", () => { new Error("systemctl --user unavailable: Failed to connect to bus: No medium found"), ); - await expect( - maybeInstallDaemon({ - runtime: { log: vi.fn(), error: vi.fn(), exit: vi.fn() }, - port: 18789, - }), - ).resolves.toBeUndefined(); + await maybeInstallDaemon({ + runtime: { log: vi.fn(), error: vi.fn(), exit: vi.fn() }, + port: 18789, + }); expect(serviceInstall).toHaveBeenCalledTimes(1); }); diff --git a/src/commands/configure.daemon.ts b/src/commands/configure.daemon.ts index b7d12d293312..ea469e73b190 100644 --- a/src/commands/configure.daemon.ts +++ b/src/commands/configure.daemon.ts @@ -17,12 +17,14 @@ import { resolveGatewayInstallToken } from "./gateway-install-token.js"; import { guardCancel } from "./onboard-helpers.js"; import { ensureSystemdUserLingerInteractive } from "./systemd-linger.js"; +export type DaemonSetupOutcome = "succeeded" | "failed" | "skipped"; + /** Prompt to install, reinstall, restart, or skip the local Gateway service. */ export async function maybeInstallDaemon(params: { runtime: RuntimeEnv; port: number; daemonRuntime?: GatewayDaemonRuntime; -}) { +}): Promise { const service = resolveGatewayService(); let loaded; try { @@ -67,7 +69,7 @@ export async function maybeInstallDaemon(params: { shouldInstall = false; } if (action === "skip") { - return; + return "skipped"; } if (action === "reinstall") { await withProgress( @@ -149,7 +151,7 @@ export async function maybeInstallDaemon(params: { if (installError) { note("Gateway service install failed: ".concat(installError), "Gateway"); note(gatewayInstallErrorHint(), "Gateway"); - return; + return "failed"; } shouldCheckLinger = true; } @@ -166,4 +168,5 @@ export async function maybeInstallDaemon(params: { requireConfirm: true, }); } + return "succeeded"; } diff --git a/src/commands/configure.wizard.persistence.test.ts b/src/commands/configure.wizard.persistence.test.ts new file mode 100644 index 000000000000..b86ca5119560 --- /dev/null +++ b/src/commands/configure.wizard.persistence.test.ts @@ -0,0 +1,151 @@ +// Configure wizard persistence tests protect config writes before local side effects. +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { OpenClawConfig } from "../config/types.openclaw.js"; + +const mocks = vi.hoisted(() => ({ + intro: vi.fn(), + outro: vi.fn(), + select: vi.fn(), + text: vi.fn(), + note: vi.fn(), + readConfigFileSnapshotForWrite: vi.fn(), + writeWizardConfigFile: vi.fn(), + probeGatewayReachable: vi.fn(), + waitForGatewayReachable: vi.fn(), + healthCommand: vi.fn(), + maybeInstallDaemon: vi.fn(), +})); + +vi.mock("./configure.shared.js", () => ({ + CONFIGURE_SECTION_OPTIONS: [ + { value: "daemon", label: "Daemon", hint: "Manage the background service" }, + { value: "health", label: "Health check", hint: "Run gateway checks" }, + ], + confirm: vi.fn(), + intro: mocks.intro, + outro: mocks.outro, + select: mocks.select, + text: mocks.text, +})); + +vi.mock("../config/config.js", () => ({ + readConfigFileSnapshotForWrite: mocks.readConfigFileSnapshotForWrite, + resolveGatewayPort: () => 18789, +})); + +vi.mock("../config/logging.js", () => ({ logConfigUpdated: vi.fn() })); +vi.mock("../../packages/terminal-core/src/note.js", () => ({ note: mocks.note })); +vi.mock("../wizard/clack-prompter.js", () => ({ createClackPrompter: () => ({}) })); +vi.mock("../wizard/setup.shared.js", () => ({ + writeWizardConfigFile: mocks.writeWizardConfigFile, +})); +vi.mock("../wizard/setup.secret-input.js", () => ({ + resolveSetupSecretInputString: vi.fn(async () => undefined), +})); + +vi.mock("./onboard-helpers.js", () => ({ + DEFAULT_WORKSPACE: "/tmp/openclaw-workspace", + applyWizardMetadata: (config: OpenClawConfig) => config, + guardCancel: (value: unknown) => value, + probeGatewayReachable: mocks.probeGatewayReachable, + resolveAdvertisedControlUiLinks: vi.fn(async () => ({ + httpUrl: "http://127.0.0.1:18789/", + wsUrl: "ws://127.0.0.1:18789", + })), + resolveLocalControlUiProbeLinks: vi.fn(() => ({ + httpUrl: "http://127.0.0.1:18789/", + wsUrl: "ws://127.0.0.1:18789", + })), + summarizeExistingConfig: vi.fn(() => "Gateway: remote"), + waitForGatewayReachable: mocks.waitForGatewayReachable, +})); + +vi.mock("./onboard-agent-target.js", () => ({ + ensureOnboardingAgentWorkspace: vi.fn(), + resolveOnboardingAgentTarget: () => ({ + agentId: "main", + agentDir: "/tmp/openclaw-agent", + workspaceDir: "/tmp/openclaw-workspace", + }), +})); + +vi.mock("../plugins/install-record-commit.js", () => ({ + commitConfigWithPendingPluginInstalls: vi.fn(), +})); +vi.mock("../plugins/plugin-registry.js", () => ({ resolvePluginContributionOwners: vi.fn() })); +vi.mock("./configure.channels.js", () => ({ removeChannelConfigWizard: vi.fn() })); +vi.mock("./configure.daemon.js", () => ({ maybeInstallDaemon: mocks.maybeInstallDaemon })); +vi.mock("./configure.gateway-auth.js", () => ({ promptAuthConfig: vi.fn() })); +vi.mock("./configure.gateway.js", () => ({ promptGatewayConfig: vi.fn() })); +vi.mock("./health.js", () => ({ healthCommand: mocks.healthCommand })); +vi.mock("./onboard-channels.js", () => ({ setupChannels: vi.fn() })); +vi.mock("./onboard-remote.js", () => ({ promptRemoteGatewayConfig: vi.fn() })); +vi.mock("./onboard-skills.js", () => ({ setupSkills: vi.fn() })); + +import { runConfigureWizard } from "./configure.wizard.js"; + +const runtime = { + log: vi.fn(), + error: vi.fn(), + exit: vi.fn(), +}; + +describe("configure wizard persistence before local side effects", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.readConfigFileSnapshotForWrite.mockResolvedValue({ + snapshot: { + exists: true, + valid: true, + config: { gateway: { mode: "remote" } }, + issues: [], + }, + writeOptions: { + expectedConfigPath: "/tmp/openclaw.json", + ownedConfigPathForWrite: "/tmp/openclaw.json", + }, + }); + mocks.probeGatewayReachable.mockResolvedValue({ ok: false }); + mocks.text.mockResolvedValue("18789"); + }); + + it.each([ + ["health", "succeeded"], + ["daemon", "succeeded"], + ["health", "failed"], + ["daemon", "failed"], + ] as const)("persists Local before %s reports %s", async (section, outcome) => { + const choices = ["local", section, "__continue"]; + const events: string[] = []; + const writes: OpenClawConfig[] = []; + mocks.select.mockImplementation(async () => choices.shift()); + mocks.writeWizardConfigFile.mockImplementation(async (config: OpenClawConfig) => { + events.push("commit"); + writes.push(config); + return config; + }); + if (section === "health") { + mocks.waitForGatewayReachable.mockImplementationOnce(async () => { + events.push("health"); + return { ok: outcome === "succeeded" }; + }); + } else { + mocks.maybeInstallDaemon.mockImplementationOnce(async () => { + events.push("daemon"); + return outcome; + }); + } + + await runConfigureWizard({ command: "configure" }, runtime); + + expect(writes).toHaveLength(1); + expect(writes[0]?.gateway?.mode).toBe("local"); + expect(events).toEqual(["commit", section]); + if (outcome === "failed") { + expect(mocks.outro).toHaveBeenLastCalledWith( + `Configuration updated, but ${section === "health" ? "health check" : "daemon setup"} failed.`, + ); + expect(mocks.outro).not.toHaveBeenCalledWith("Configuration updated."); + } + }); +}); diff --git a/src/commands/configure.wizard.test.ts b/src/commands/configure.wizard.test.ts index 9979cccebd5e..d31c3364cd3b 100644 --- a/src/commands/configure.wizard.test.ts +++ b/src/commands/configure.wizard.test.ts @@ -388,6 +388,7 @@ describe("runConfigureWizard", () => { }); vi.mocked(maybeInstallDaemon).mockImplementationOnce(async () => { events.push("daemon"); + return "succeeded"; }); await runConfigureWizard( diff --git a/src/commands/configure.wizard.ts b/src/commands/configure.wizard.ts index 91b17d969303..71654cab38d3 100644 --- a/src/commands/configure.wizard.ts +++ b/src/commands/configure.wizard.ts @@ -25,7 +25,7 @@ import { WizardCancelledError } from "../wizard/prompts.js"; import { resolveSetupSecretInputString } from "../wizard/setup.secret-input.js"; import { writeWizardConfigFile } from "../wizard/setup.shared.js"; import { removeChannelConfigWizard } from "./configure.channels.js"; -import { maybeInstallDaemon } from "./configure.daemon.js"; +import { maybeInstallDaemon, type DaemonSetupOutcome } from "./configure.daemon.js"; import { promptAuthConfig } from "./configure.gateway-auth.js"; import { promptGatewayConfig } from "./configure.gateway.js"; import type { @@ -600,8 +600,8 @@ export async function runConfigureWizard( let nextConfig = { ...baseConfig }; let mergeBaseConfig = structuredClone(baseConfig); - let didSetGatewayMode = false; - if (shouldPromptGatewayRunMode && nextConfig.gateway?.mode !== "local") { + let hasPendingConfig = shouldPromptGatewayRunMode && nextConfig.gateway?.mode !== "local"; + if (hasPendingConfig) { nextConfig = { ...nextConfig, gateway: { @@ -609,7 +609,6 @@ export async function runConfigureWizard( mode: "local", }, }; - didSetGatewayMode = true; } // Configure keeps legacy default-owner semantics; only explicit fleets opt into // the System Agent target used unconditionally by setup and recovery callers. @@ -619,8 +618,14 @@ export async function runConfigureWizard( : resolveOnboardingAgentTarget(inheritLegacyDefaultAgentId(baseConfig, nextConfig)); let workspaceDir = resolveSetupTarget().workspaceDir; let gatewayPort = resolveGatewayPort(baseConfig); + let didPersistConfig = false; + let daemonSetupOutcome: DaemonSetupOutcome | undefined; + let healthCheckOutcome: GatewayHealthCheckOutcome | undefined; - const persistConfig = async () => { + const persistPendingConfig = async () => { + if (!hasPendingConfig) { + return; + } nextConfig = applyWizardMetadata(nextConfig, { command: opts.command, mode: metadataMode, @@ -631,6 +636,8 @@ export async function runConfigureWizard( writeOptions: configWriteOwnership, }); mergeBaseConfig = structuredClone(nextConfig); + hasPendingConfig = false; + didPersistConfig = true; logConfigUpdated(runtime); }; @@ -781,10 +788,14 @@ export async function runConfigureWizard( if (!didConfigureGateway) { await promptDaemonPort(); } - await maybeInstallDaemon({ runtime, port: gatewayPort }); + daemonSetupOutcome = await maybeInstallDaemon({ runtime, port: gatewayPort }); }, health: async () => { - await runGatewayHealthCheck({ cfg: nextConfig, runtime, port: gatewayPort }); + healthCheckOutcome = await runGatewayHealthCheck({ + cfg: nextConfig, + runtime, + port: gatewayPort, + }); }, } satisfies Record Promise>; @@ -807,10 +818,11 @@ export async function runConfigureWizard( ] as const) { if (selectedSections.includes(section)) { await sectionActions[section](); + hasPendingConfig = true; } } - await persistConfig(); + await persistPendingConfig(); for (const section of ["daemon", "health"] as const) { if (selectedSections.includes(section)) { @@ -826,16 +838,20 @@ export async function runConfigureWizard( break; } ranSection = true; + if (choice === "daemon" || choice === "health") { + await persistPendingConfig(); + } await sectionActions[choice](); if (choice !== "daemon" && choice !== "health") { // Interactive setup commits each section before showing another prompt. - await persistConfig(); + hasPendingConfig = true; + await persistPendingConfig(); } } if (!ranSection) { - if (didSetGatewayMode) { - await persistConfig(); + if (hasPendingConfig) { + await persistPendingConfig(); outro("Gateway mode set to local."); return; } @@ -844,6 +860,21 @@ export async function runConfigureWizard( } } + const failedSideEffects = [ + ...(daemonSetupOutcome === "failed" ? ["daemon setup"] : []), + ...(healthCheckOutcome === "failed" ? ["health check"] : []), + ]; + let completionMessage = didPersistConfig + ? "Configuration updated." + : "No configuration changes selected."; + if (failedSideEffects.length > 0) { + completionMessage = `${didPersistConfig ? "Configuration updated" : "Configuration unchanged"}, but ${failedSideEffects.join(" and ")} failed.`; + } else if (!didPersistConfig && healthCheckOutcome) { + completionMessage = `Health check ${healthCheckOutcome === "succeeded" ? "completed" : "skipped"}.`; + } else if (!didPersistConfig && daemonSetupOutcome) { + completionMessage = `Daemon setup ${daemonSetupOutcome === "succeeded" ? "completed" : "skipped"}.`; + } + if (shouldSkipGatewaySummary) { const remoteUrl = normalizeOptionalString(nextConfig.gateway?.remote?.url); if (remoteUrl) { @@ -854,7 +885,7 @@ export async function runConfigureWizard( "Gateway", ); } - outro("Configuration updated."); + outro(completionMessage); return; } @@ -923,7 +954,7 @@ export async function runConfigureWizard( "Control UI", ); - outro("Configuration updated."); + outro(completionMessage); } catch (err) { if (err instanceof WizardCancelledError) { runtime.exit(1);