From 57f40a5da60870bc547fd49182f3f5f224332143 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 14:25:22 +0000 Subject: [PATCH] perf(test): speed up config tests --- src/cli/config-cli.test.ts | 258 +++++++++++----------- src/config/backup-rotation.ts | 26 +++ src/config/config.backup-rotation.test.ts | 23 +- src/config/config.env-vars.test.ts | 138 ++++-------- src/config/env-vars.ts | 13 ++ src/config/io.ts | 37 +--- 6 files changed, 225 insertions(+), 270 deletions(-) create mode 100644 src/config/backup-rotation.ts diff --git a/src/cli/config-cli.test.ts b/src/cli/config-cli.test.ts index 6f144cb5459..0b193f923d5 100644 --- a/src/cli/config-cli.test.ts +++ b/src/cli/config-cli.test.ts @@ -1,16 +1,21 @@ import { Command } from "commander"; -import fs from "node:fs/promises"; -import os from "node:os"; -import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { ConfigFileSnapshot, OpenClawConfig } from "../config/types.js"; /** * Test for issue #6070: - * `openclaw config set` should use snapshot.parsed (raw user config) instead of - * snapshot.config (runtime-merged config with defaults), to avoid overwriting - * the entire config with defaults when validation fails or config is unreadable. + * `openclaw config set/unset` must update snapshot.resolved (user config after $include/${ENV}, + * but before runtime defaults), so runtime defaults don't leak into the written config. */ +const mockReadConfigFileSnapshot = vi.fn<[], Promise>(); +const mockWriteConfigFile = vi.fn<[OpenClawConfig], Promise>(async () => {}); + +vi.mock("../config/config.js", () => ({ + readConfigFileSnapshot: () => mockReadConfigFileSnapshot(), + writeConfigFile: (cfg: OpenClawConfig) => mockWriteConfigFile(cfg), +})); + const mockLog = vi.fn(); const mockError = vi.fn(); const mockExit = vi.fn((code: number) => { @@ -26,29 +31,22 @@ vi.mock("../runtime.js", () => ({ }, })); -async function withTempHome(run: (home: string) => Promise): Promise { - const home = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-config-cli-")); - const originalEnv = { ...process.env }; - try { - // Override config path to use temp directory - process.env.OPENCLAW_CONFIG_PATH = path.join(home, ".openclaw", "openclaw.json"); - await fs.mkdir(path.join(home, ".openclaw"), { recursive: true }); - await run(home); - } finally { - process.env = originalEnv; - await fs.rm(home, { recursive: true, force: true }); - } -} - -async function readConfigFile(home: string): Promise> { - const configPath = path.join(home, ".openclaw", "openclaw.json"); - const content = await fs.readFile(configPath, "utf-8"); - return JSON.parse(content); -} - -async function writeConfigFile(home: string, config: Record): Promise { - const configPath = path.join(home, ".openclaw", "openclaw.json"); - await fs.writeFile(configPath, JSON.stringify(config, null, 2)); +function buildSnapshot(params: { + resolved: OpenClawConfig; + config: OpenClawConfig; +}): ConfigFileSnapshot { + return { + path: "/tmp/openclaw.json", + exists: true, + raw: JSON.stringify(params.resolved), + parsed: params.resolved, + resolved: params.resolved, + valid: true, + config: params.config, + issues: [], + warnings: [], + legacyIssues: [], + }; } describe("config cli", () => { @@ -62,128 +60,122 @@ describe("config cli", () => { describe("config set - issue #6070", () => { it("preserves existing config keys when setting a new value", async () => { - await withTempHome(async (home) => { - // Set up a config file with multiple existing settings (using valid schema) - const initialConfig = { - agents: { - list: [{ id: "main" }, { id: "oracle", workspace: "~/oracle-workspace" }], - }, - gateway: { - port: 18789, - }, - tools: { - allow: ["group:fs"], - }, - logging: { - level: "debug", - }, - }; - await writeConfigFile(home, initialConfig); + const resolved: OpenClawConfig = { + agents: { + list: [{ id: "main" }, { id: "oracle", workspace: "~/oracle-workspace" }], + }, + gateway: { port: 18789 }, + tools: { allow: ["group:fs"] }, + logging: { level: "debug" }, + }; + const runtimeMerged: OpenClawConfig = { + ...resolved, + agents: { + ...resolved.agents, + defaults: { + model: "gpt-5.2", + } as never, + } as never, + }; + mockReadConfigFileSnapshot.mockResolvedValueOnce( + buildSnapshot({ resolved, config: runtimeMerged }), + ); - // Run config set to add a new value - const { registerConfigCli } = await import("./config-cli.js"); - const program = new Command(); - program.exitOverride(); - registerConfigCli(program); + const { registerConfigCli } = await import("./config-cli.js"); + const program = new Command(); + program.exitOverride(); + registerConfigCli(program); - await program.parseAsync(["config", "set", "gateway.auth.mode", "token"], { from: "user" }); + await program.parseAsync(["config", "set", "gateway.auth.mode", "token"], { from: "user" }); - // Read the config file and verify ALL original keys are preserved - const finalConfig = await readConfigFile(home); - - // The new value should be set - expect((finalConfig.gateway as Record).auth).toEqual({ mode: "token" }); - - // ALL original settings must still be present (this is the key assertion for #6070) - // The key bug in #6070 was that runtime defaults (like agents.defaults) were being - // written to the file, and paths were being expanded. This test verifies the fix. - expect(finalConfig.agents).not.toHaveProperty("defaults"); // No runtime defaults injected - expect((finalConfig.agents as Record).list).toEqual( - initialConfig.agents.list, - ); - expect((finalConfig.gateway as Record).port).toBe(18789); - expect(finalConfig.tools).toEqual(initialConfig.tools); - expect(finalConfig.logging).toEqual(initialConfig.logging); - }); + expect(mockWriteConfigFile).toHaveBeenCalledTimes(1); + const written = mockWriteConfigFile.mock.calls[0]?.[0]; + expect(written.gateway?.auth).toEqual({ mode: "token" }); + expect(written.gateway?.port).toBe(18789); + expect(written.agents).toEqual(resolved.agents); + expect(written.tools).toEqual(resolved.tools); + expect(written.logging).toEqual(resolved.logging); + expect(written.agents).not.toHaveProperty("defaults"); }); it("does not inject runtime defaults into the written config", async () => { - await withTempHome(async (home) => { - // Set up a minimal config file - const initialConfig = { - gateway: { port: 18789 }, - }; - await writeConfigFile(home, initialConfig); + const resolved: OpenClawConfig = { + gateway: { port: 18789 }, + }; + const runtimeMerged: OpenClawConfig = { + ...resolved, + agents: { + defaults: { + model: "gpt-5.2", + contextWindow: 128_000, + maxTokens: 16_000, + }, + } as never, + messages: { ackReaction: "✅" } as never, + sessions: { persistence: { enabled: true } } as never, + }; + mockReadConfigFileSnapshot.mockResolvedValueOnce( + buildSnapshot({ resolved, config: runtimeMerged }), + ); - // Run config set - const { registerConfigCli } = await import("./config-cli.js"); - const program = new Command(); - program.exitOverride(); - registerConfigCli(program); + const { registerConfigCli } = await import("./config-cli.js"); + const program = new Command(); + program.exitOverride(); + registerConfigCli(program); - await program.parseAsync(["config", "set", "gateway.auth.mode", "token"], { - from: "user", - }); + await program.parseAsync(["config", "set", "gateway.auth.mode", "token"], { from: "user" }); - // Read the config file - const finalConfig = await readConfigFile(home); - - // The config should NOT contain runtime defaults that weren't originally in the file - // These are examples of defaults that get merged in by applyModelDefaults, applyAgentDefaults, etc. - expect(finalConfig).not.toHaveProperty("agents.defaults.model"); - expect(finalConfig).not.toHaveProperty("agents.defaults.contextWindow"); - expect(finalConfig).not.toHaveProperty("agents.defaults.maxTokens"); - expect(finalConfig).not.toHaveProperty("messages.ackReaction"); - expect(finalConfig).not.toHaveProperty("sessions.persistence"); - - // Original config should still be present - expect((finalConfig.gateway as Record).port).toBe(18789); - // New value should be set - expect((finalConfig.gateway as Record).auth).toEqual({ mode: "token" }); - }); + expect(mockWriteConfigFile).toHaveBeenCalledTimes(1); + const written = mockWriteConfigFile.mock.calls[0]?.[0]; + expect(written).not.toHaveProperty("agents.defaults.model"); + expect(written).not.toHaveProperty("agents.defaults.contextWindow"); + expect(written).not.toHaveProperty("agents.defaults.maxTokens"); + expect(written).not.toHaveProperty("messages.ackReaction"); + expect(written).not.toHaveProperty("sessions.persistence"); + expect(written.gateway?.port).toBe(18789); + expect(written.gateway?.auth).toEqual({ mode: "token" }); }); }); describe("config unset - issue #6070", () => { it("preserves existing config keys when unsetting a value", async () => { - await withTempHome(async (home) => { - // Set up a config file with multiple existing settings (using valid schema) - const initialConfig = { - agents: { list: [{ id: "main" }] }, - gateway: { port: 18789 }, - tools: { - profile: "coding", - alsoAllow: ["agents_list"], + const resolved: OpenClawConfig = { + agents: { list: [{ id: "main" }] }, + gateway: { port: 18789 }, + tools: { + profile: "coding", + alsoAllow: ["agents_list"], + }, + logging: { level: "debug" }, + }; + const runtimeMerged: OpenClawConfig = { + ...resolved, + agents: { + ...resolved.agents, + defaults: { + model: "gpt-5.2", }, - logging: { - level: "debug", - }, - }; - await writeConfigFile(home, initialConfig); + } as never, + }; + mockReadConfigFileSnapshot.mockResolvedValueOnce( + buildSnapshot({ resolved, config: runtimeMerged }), + ); - // Run config unset to remove a value - const { registerConfigCli } = await import("./config-cli.js"); - const program = new Command(); - program.exitOverride(); - registerConfigCli(program); + const { registerConfigCli } = await import("./config-cli.js"); + const program = new Command(); + program.exitOverride(); + registerConfigCli(program); - await program.parseAsync(["config", "unset", "tools.alsoAllow"], { from: "user" }); + await program.parseAsync(["config", "unset", "tools.alsoAllow"], { from: "user" }); - // Read the config file and verify ALL original keys (except the unset one) are preserved - const finalConfig = await readConfigFile(home); - - // The value should be removed - expect(finalConfig.tools as Record).not.toHaveProperty("alsoAllow"); - - // ALL other original settings must still be present (no runtime defaults injected) - expect(finalConfig.agents).not.toHaveProperty("defaults"); - expect((finalConfig.agents as Record).list).toEqual( - initialConfig.agents.list, - ); - expect(finalConfig.gateway).toEqual(initialConfig.gateway); - expect((finalConfig.tools as Record).profile).toBe("coding"); - expect(finalConfig.logging).toEqual(initialConfig.logging); - }); + expect(mockWriteConfigFile).toHaveBeenCalledTimes(1); + const written = mockWriteConfigFile.mock.calls[0]?.[0]; + expect(written.tools).not.toHaveProperty("alsoAllow"); + expect(written.agents).not.toHaveProperty("defaults"); + expect(written.agents?.list).toEqual(resolved.agents?.list); + expect(written.gateway).toEqual(resolved.gateway); + expect(written.tools?.profile).toBe("coding"); + expect(written.logging).toEqual(resolved.logging); }); }); }); diff --git a/src/config/backup-rotation.ts b/src/config/backup-rotation.ts new file mode 100644 index 00000000000..d6c3035ebef --- /dev/null +++ b/src/config/backup-rotation.ts @@ -0,0 +1,26 @@ +export const CONFIG_BACKUP_COUNT = 5; + +export async function rotateConfigBackups( + configPath: string, + ioFs: { + unlink: (path: string) => Promise; + rename: (from: string, to: string) => Promise; + }, +): Promise { + if (CONFIG_BACKUP_COUNT <= 1) { + return; + } + const backupBase = `${configPath}.bak`; + const maxIndex = CONFIG_BACKUP_COUNT - 1; + await ioFs.unlink(`${backupBase}.${maxIndex}`).catch(() => { + // best-effort + }); + for (let index = maxIndex - 1; index >= 1; index -= 1) { + await ioFs.rename(`${backupBase}.${index}`, `${backupBase}.${index + 1}`).catch(() => { + // best-effort + }); + } + await ioFs.rename(backupBase, `${backupBase}.1`).catch(() => { + // best-effort + }); +} diff --git a/src/config/config.backup-rotation.test.ts b/src/config/config.backup-rotation.test.ts index 166deed0dca..d569d20e711 100644 --- a/src/config/config.backup-rotation.test.ts +++ b/src/config/config.backup-rotation.test.ts @@ -1,20 +1,35 @@ import fs from "node:fs/promises"; +import path from "node:path"; import { describe, expect, it } from "vitest"; import type { OpenClawConfig } from "./types.js"; +import { rotateConfigBackups } from "./backup-rotation.js"; import { withTempHome } from "./test-helpers.js"; describe("config backup rotation", () => { it("keeps a 5-deep backup ring for config writes", async () => { await withTempHome(async () => { - const { resolveConfigPath, writeConfigFile } = await import("./config.js"); - const configPath = resolveConfigPath(); + const stateDir = process.env.OPENCLAW_STATE_DIR?.trim(); + if (!stateDir) { + throw new Error("Expected OPENCLAW_STATE_DIR to be set by withTempHome"); + } + const configPath = path.join(stateDir, "openclaw.json"); const buildConfig = (version: number): OpenClawConfig => ({ agents: { list: [{ id: `v${version}` }] }, }) as OpenClawConfig; - for (let version = 0; version <= 6; version += 1) { - await writeConfigFile(buildConfig(version)); + const writeVersion = async (version: number) => { + const json = JSON.stringify(buildConfig(version), null, 2).trimEnd().concat("\n"); + await fs.writeFile(configPath, json, "utf-8"); + }; + + await writeVersion(0); + for (let version = 1; version <= 6; version += 1) { + await rotateConfigBackups(configPath, fs); + await fs.copyFile(configPath, `${configPath}.bak`).catch(() => { + // best-effort + }); + await writeVersion(version); } const readName = async (suffix = "") => { diff --git a/src/config/config.env-vars.test.ts b/src/config/config.env-vars.test.ts index 9e9fca6f2aa..fdee7bd0532 100644 --- a/src/config/config.env-vars.test.ts +++ b/src/config/config.env-vars.test.ts @@ -1,125 +1,63 @@ import fs from "node:fs/promises"; import path from "node:path"; import { describe, expect, it } from "vitest"; -import { resolveStateDir } from "./paths.js"; +import type { OpenClawConfig } from "./types.js"; +import { loadDotEnv } from "../infra/dotenv.js"; +import { resolveConfigEnvVars } from "./env-substitution.js"; +import { applyConfigEnvVars } from "./env-vars.js"; import { withEnvOverride, withTempHome } from "./test-helpers.js"; describe("config env vars", () => { it("applies env vars from env block when missing", async () => { - await withTempHome(async (home) => { - const configDir = path.join(home, ".openclaw"); - await fs.mkdir(configDir, { recursive: true }); - await fs.writeFile( - path.join(configDir, "openclaw.json"), - JSON.stringify( - { - env: { vars: { OPENROUTER_API_KEY: "config-key" } }, - }, - null, - 2, - ), - "utf-8", - ); - - await withEnvOverride({ OPENROUTER_API_KEY: undefined }, async () => { - const { loadConfig } = await import("./config.js"); - loadConfig(); - expect(process.env.OPENROUTER_API_KEY).toBe("config-key"); - }); + await withEnvOverride({ OPENROUTER_API_KEY: undefined }, async () => { + applyConfigEnvVars({ env: { vars: { OPENROUTER_API_KEY: "config-key" } } } as OpenClawConfig); + expect(process.env.OPENROUTER_API_KEY).toBe("config-key"); }); }); it("does not override existing env vars", async () => { - await withTempHome(async (home) => { - const configDir = path.join(home, ".openclaw"); - await fs.mkdir(configDir, { recursive: true }); - await fs.writeFile( - path.join(configDir, "openclaw.json"), - JSON.stringify( - { - env: { vars: { OPENROUTER_API_KEY: "config-key" } }, - }, - null, - 2, - ), - "utf-8", - ); - - await withEnvOverride({ OPENROUTER_API_KEY: "existing-key" }, async () => { - const { loadConfig } = await import("./config.js"); - loadConfig(); - expect(process.env.OPENROUTER_API_KEY).toBe("existing-key"); - }); + await withEnvOverride({ OPENROUTER_API_KEY: "existing-key" }, async () => { + applyConfigEnvVars({ env: { vars: { OPENROUTER_API_KEY: "config-key" } } } as OpenClawConfig); + expect(process.env.OPENROUTER_API_KEY).toBe("existing-key"); }); }); it("applies env vars from env.vars when missing", async () => { - await withTempHome(async (home) => { - const configDir = path.join(home, ".openclaw"); - await fs.mkdir(configDir, { recursive: true }); - await fs.writeFile( - path.join(configDir, "openclaw.json"), - JSON.stringify( - { - env: { vars: { GROQ_API_KEY: "gsk-config" } }, - }, - null, - 2, - ), - "utf-8", - ); - - await withEnvOverride({ GROQ_API_KEY: undefined }, async () => { - const { loadConfig } = await import("./config.js"); - loadConfig(); - expect(process.env.GROQ_API_KEY).toBe("gsk-config"); - }); + await withEnvOverride({ GROQ_API_KEY: undefined }, async () => { + applyConfigEnvVars({ env: { vars: { GROQ_API_KEY: "gsk-config" } } } as OpenClawConfig); + expect(process.env.GROQ_API_KEY).toBe("gsk-config"); }); }); it("loads ${VAR} substitutions from ~/.openclaw/.env on repeated runtime loads", async () => { - await withTempHome(async (home) => { - await withEnvOverride( - { - OPENCLAW_STATE_DIR: path.join(home, ".openclaw"), - CLAWDBOT_STATE_DIR: undefined, - OPENCLAW_HOME: undefined, - CLAWDBOT_HOME: undefined, - BRAVE_API_KEY: undefined, - OPENCLAW_DISABLE_CONFIG_CACHE: "1", - }, - async () => { - const configDir = resolveStateDir(process.env, () => home); - await fs.mkdir(configDir, { recursive: true }); - await fs.writeFile( - path.join(configDir, "openclaw.json"), - JSON.stringify( - { - tools: { - web: { - search: { - apiKey: "${BRAVE_API_KEY}", - }, - }, - }, + await withTempHome(async (_home) => { + await withEnvOverride({ BRAVE_API_KEY: undefined }, async () => { + const stateDir = process.env.OPENCLAW_STATE_DIR?.trim(); + if (!stateDir) { + throw new Error("Expected OPENCLAW_STATE_DIR to be set by withTempHome"); + } + await fs.mkdir(stateDir, { recursive: true }); + await fs.writeFile(path.join(stateDir, ".env"), "BRAVE_API_KEY=from-dotenv\n", "utf-8"); + + const config: OpenClawConfig = { + tools: { + web: { + search: { + apiKey: "${BRAVE_API_KEY}", }, - null, - 2, - ), - "utf-8", - ); - await fs.writeFile(path.join(configDir, ".env"), "BRAVE_API_KEY=from-dotenv\n", "utf-8"); + }, + }, + }; - const { loadConfig } = await import("./config.js"); + loadDotEnv({ quiet: true }); + const first = resolveConfigEnvVars(config, process.env) as OpenClawConfig; + expect(first.tools?.web?.search?.apiKey).toBe("from-dotenv"); - const first = loadConfig(); - expect(first.tools?.web?.search?.apiKey).toBe("from-dotenv"); - - delete process.env.BRAVE_API_KEY; - const second = loadConfig(); - expect(second.tools?.web?.search?.apiKey).toBe("from-dotenv"); - }, - ); + delete process.env.BRAVE_API_KEY; + loadDotEnv({ quiet: true }); + const second = resolveConfigEnvVars(config, process.env) as OpenClawConfig; + expect(second.tools?.web?.search?.apiKey).toBe("from-dotenv"); + }); }); }); }); diff --git a/src/config/env-vars.ts b/src/config/env-vars.ts index 54f9fceee9c..458674b75a3 100644 --- a/src/config/env-vars.ts +++ b/src/config/env-vars.ts @@ -29,3 +29,16 @@ export function collectConfigEnvVars(cfg?: OpenClawConfig): Record(); @@ -340,25 +340,6 @@ function restoreEnvRefsFromMap( return value; } -async function rotateConfigBackups(configPath: string, ioFs: typeof fs.promises): Promise { - if (CONFIG_BACKUP_COUNT <= 1) { - return; - } - const backupBase = `${configPath}.bak`; - const maxIndex = CONFIG_BACKUP_COUNT - 1; - await ioFs.unlink(`${backupBase}.${maxIndex}`).catch(() => { - // best-effort - }); - for (let index = maxIndex - 1; index >= 1; index -= 1) { - await ioFs.rename(`${backupBase}.${index}`, `${backupBase}.${index + 1}`).catch(() => { - // best-effort - }); - } - await ioFs.rename(backupBase, `${backupBase}.1`).catch(() => { - // best-effort - }); -} - function resolveConfigAuditLogPath(env: NodeJS.ProcessEnv, homedir: () => string): string { return path.join(resolveStateDir(env, homedir), "logs", CONFIG_AUDIT_LOG_FILENAME); } @@ -460,16 +441,6 @@ function warnIfConfigFromFuture(cfg: OpenClawConfig, logger: Pick): string { if (deps.configPath) { return deps.configPath; @@ -531,7 +502,7 @@ function resolveConfigForRead( ): ConfigReadResolution { // Apply config.env to process.env BEFORE substitution so ${VAR} can reference config-defined vars. if (resolvedIncludes && typeof resolvedIncludes === "object" && "env" in resolvedIncludes) { - applyConfigEnv(resolvedIncludes as OpenClawConfig, env); + applyConfigEnvVars(resolvedIncludes as OpenClawConfig, env); } return { @@ -627,7 +598,7 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { throw new DuplicateAgentDirError(duplicates); } - applyConfigEnv(cfg, deps.env); + applyConfigEnvVars(cfg, deps.env); const enabled = shouldEnableShellEnvFallback(deps.env) || cfg.env?.shellEnv?.enabled === true; if (enabled && !shouldDeferShellEnvFallback(deps.env)) {