From 69e4fbbeb5f800779e9b3f3c1a59527f77bfcf1f Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 21 Aug 2026 14:33:10 -0700 Subject: [PATCH] fix(cli): tell operators a racing config set changed nothing (#127554) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When two config writes raced, the loser printed a bare "config changed since last load" and exited 1 — no statement that its write was abandoned and no hint that re-running works. The gateway sibling already says "re-run config.get and retry". Format ConfigMutationConflictError at handleConfigMutationError, the single CLI boundary, so the low-level throw sites keep serving gateway and programmatic callers unchanged. Dry-run JSON now reports kind "conflict" instead of mislabelling a concurrency conflict as a schema error. Concurrency control itself is unchanged: baseHash pinning and the retry loop stay exactly as they were. --- docs/cli/config.md | 2 +- src/cli/config-cli-runner.ts | 9 +++++-- src/cli/config-cli.test.ts | 48 ++++++++++++++++++++++++++++++++++++ src/cli/config-set-dryrun.ts | 2 +- 4 files changed, 57 insertions(+), 4 deletions(-) diff --git a/docs/cli/config.md b/docs/cli/config.md index 8f4d0a9bf114..f4925ca76f04 100644 --- a/docs/cli/config.md +++ b/docs/cli/config.md @@ -365,7 +365,7 @@ openclaw config set channels.discord.token \ - `checks.resolvabilityComplete`: whether resolvability checks ran to completion (false when exec refs are skipped) - `refsChecked`: number of refs actually resolved during dry-run - `skippedExecRefs`: number of exec refs skipped because `--allow-exec` was not set - - `errors`: structured missing-path, schema, or resolvability failures when `ok=false` + - `errors`: structured failures when `ok=false`; each carries a `kind` of `missing-path`, `schema`, `resolvability`, `model`, or `conflict` (`conflict` means the config file changed while the command was writing, so nothing was changed — re-run to pick up the new file) diff --git a/src/cli/config-cli-runner.ts b/src/cli/config-cli-runner.ts index 651c7e94e2b9..f3a0e9d8a86b 100644 --- a/src/cli/config-cli-runner.ts +++ b/src/cli/config-cli-runner.ts @@ -3,6 +3,7 @@ import { uniqueValues } from "@openclaw/normalization-core/string-normalization" import { replaceConfigFile } from "../config/config.js"; import { AUTO_MANAGED_CONFIG_META_PATHS } from "../config/io.meta.js"; import { formatConfigIssueLines } from "../config/issue-format.js"; +import { ConfigMutationConflictError } from "../config/mutation-conflict.js"; import { resolveConfigPath } from "../config/paths.js"; import { readBestEffortRuntimeConfigSchema } from "../config/runtime-schema.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; @@ -513,7 +514,11 @@ export function handleConfigMutationError(params: { runtime: RuntimeEnv; options: ConfigMutationOptions; }) { - const message = formatErrorMessage(params.err); + const isConflict = params.err instanceof ConfigMutationConflictError; + const detail = formatErrorMessage(params.err); + const message = isConflict + ? `The config file changed while this command was writing (${detail}), so nothing was changed. Re-run the same command to pick up the new file and try again.` + : detail; if (params.options.dryRun && params.options.json) { if (params.err instanceof ConfigSetDryRunValidationError) { writeRuntimeJson(params.runtime, params.err.result); @@ -528,7 +533,7 @@ export function handleConfigMutationError(params: { checks: { schema: false, resolvability: false, resolvabilityComplete: false }, refsChecked: 0, skippedExecRefs: 0, - errors: [{ kind: "schema", message }], + errors: [{ kind: isConflict ? "conflict" : "schema", message }], }; writeRuntimeJson(params.runtime, result); params.runtime.error(danger(message)); diff --git a/src/cli/config-cli.test.ts b/src/cli/config-cli.test.ts index 6b27d541ba06..b2d908fcde4d 100644 --- a/src/cli/config-cli.test.ts +++ b/src/cli/config-cli.test.ts @@ -5,6 +5,7 @@ import { Command } from "commander"; // Config CLI tests cover config command registration, reads, writes, and output modes. import { createRequireRecord } from "openclaw/plugin-sdk/test-fixtures"; import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { ConfigMutationConflictError } from "../config/mutation-conflict.js"; import type { ConfigFileSnapshot, OpenClawConfig } from "../config/types.js"; import type { PluginManifestRecord, PluginManifestRegistry } from "../plugins/manifest-registry.js"; import type { PluginMetadataSnapshot } from "../plugins/plugin-metadata-snapshot.js"; @@ -3373,6 +3374,53 @@ describe("config cli", () => { expectErrorIncludes("Dry run failed: 1 SecretRef assignment(s) could not be resolved."); }); + it("explains config mutation conflicts without changing the exit code", async () => { + mockWriteConfigFile.mockRejectedValueOnce( + new ConfigMutationConflictError("included config changed since last load"), + ); + + await expect(runConfigSet("gateway.port", "19000")).rejects.toThrow(ExitError); + + expect(mockExit).toHaveBeenCalledWith(1); + expectErrorIncludes( + "The config file changed while this command was writing (included config changed since last load), so nothing was changed. Re-run the same command to pick up the new file and try again.", + ); + }); + + it("reports config mutation conflicts accurately in dry-run JSON", async () => { + mockReadConfigFileSnapshot.mockRejectedValueOnce( + new ConfigMutationConflictError("config changed since last load"), + ); + + await expect( + runConfigCommand(["config", "set", "gateway.port", "19000", "--dry-run", "--json"]), + ).rejects.toThrow(ExitError); + + expect(mockExit).toHaveBeenCalledWith(1); + expect(parseLastLogPayload()).toMatchObject({ + ok: false, + errors: [ + { + kind: "conflict", + message: + "The config file changed while this command was writing (config changed since last load), so nothing was changed. Re-run the same command to pick up the new file and try again.", + }, + ], + }); + }); + + it("preserves non-conflict config mutation errors", async () => { + mockWriteConfigFile.mockRejectedValueOnce(new Error("permission denied")); + + await expect(runConfigSet("gateway.port", "19000")).rejects.toThrow(ExitError); + + expect(mockExit).toHaveBeenCalledWith(1); + expectErrorIncludes("permission denied"); + expect(mockError.mock.calls.flat().join("\n")).not.toContain( + "The config file changed while this command was writing", + ); + }); + it("emits structured JSON for --dry-run --json success", async () => { setGatewaySnapshot({ providers: { default: { source: "env" } } }); diff --git a/src/cli/config-set-dryrun.ts b/src/cli/config-set-dryrun.ts index c9f3cf62c13e..cffac1d783ee 100644 --- a/src/cli/config-set-dryrun.ts +++ b/src/cli/config-set-dryrun.ts @@ -4,7 +4,7 @@ export type ConfigSetDryRunInputMode = "value" | "json" | "builder" | "unset"; /** One validation error found during config-set dry-run processing. */ export type ConfigSetDryRunError = { - kind: "missing-path" | "schema" | "resolvability" | "model"; + kind: "missing-path" | "schema" | "resolvability" | "model" | "conflict"; message: string; ref?: string; };