mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-27 21:07:01 -06:00
fix(cli): tell operators a racing config set changed nothing (#127554)
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.
This commit is contained in:
committed by
GitHub
parent
c18654bf01
commit
69e4fbbeb5
+1
-1
@@ -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)
|
||||
|
||||
</Accordion>
|
||||
</AccordionGroup>
|
||||
|
||||
@@ -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));
|
||||
|
||||
@@ -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" } } });
|
||||
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user