From e3861e0bede70fb405be2b9b6ec6f35ce0391cfd Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 3 Aug 2026 08:00:59 -0700 Subject: [PATCH] fix(cli): publish completion caches atomically (#118715) Co-authored-by: Peter Steinberger --- src/cli/completion-cli.ts | 14 +- src/cli/completion-cli.write-state.test.ts | 183 ++++++++++++++++++++- 2 files changed, 190 insertions(+), 7 deletions(-) diff --git a/src/cli/completion-cli.ts b/src/cli/completion-cli.ts index 66ad490dd627..91b8e898b90c 100644 --- a/src/cli/completion-cli.ts +++ b/src/cli/completion-cli.ts @@ -1,6 +1,5 @@ // Shell completion generation, cache writing, and install command registration. import fs from "node:fs/promises"; -import path from "node:path"; import { Command, Option } from "commander"; import { formatDocsLink } from "../../packages/terminal-core/src/links.js"; import { theme } from "../../packages/terminal-core/src/theme.js"; @@ -23,6 +22,7 @@ import { resolveShellFromEnv, type CompletionShell, } from "./completion-runtime.js"; +import { publishOutputFileAtomically } from "./output-file.runtime.js"; import { getCoreCliCommandNames, registerCoreCliByName } from "./program/command-registry-core.js"; import { getProgramContext } from "./program/program-context.js"; import { getSubCliEntries, registerSubCliByName } from "./program/register.subclis-core.js"; @@ -140,13 +140,15 @@ async function writeCompletionCache(params: { shells: CompletionShell[]; binName: string; }): Promise { - const firstShell = params.shells[0] ?? "zsh"; - const cacheDir = path.dirname(resolveCompletionCachePath(firstShell, params.binName)); - await fs.mkdir(cacheDir, { recursive: true }); for (const shell of params.shells) { const script = getCompletionScript(shell, params.program); - const targetPath = resolveCompletionCachePath(shell, params.binName); - await fs.writeFile(targetPath, script, "utf-8"); + await publishOutputFileAtomically({ + filePath: resolveCompletionCachePath(shell, params.binName), + tempPrefix: ".openclaw-completion-cache", + writeTemp: async (tempPath) => { + await fs.writeFile(tempPath, script, { encoding: "utf-8", flag: "wx" }); + }, + }); } } diff --git a/src/cli/completion-cli.write-state.test.ts b/src/cli/completion-cli.write-state.test.ts index 853a683974c2..d3c4230463d4 100644 --- a/src/cli/completion-cli.write-state.test.ts +++ b/src/cli/completion-cli.write-state.test.ts @@ -9,8 +9,15 @@ import { COMPLETION_SHELLS, resolveCompletionCachePath, resolveCompletionProfilePath, + type CompletionShell, } from "./completion-runtime.js"; +type PublishOutputFileAtomically = + typeof import("./output-file.runtime.js").publishOutputFileAtomically; + +const outputFileMocks = vi.hoisted(() => ({ + publishOutputFileAtomically: vi.fn(), +})); const stderrWrites = vi.hoisted(() => vi.fn()); const getCoreCliCommandNamesMock = vi.hoisted(() => vi.fn(() => [])); const registerCoreCliByNameMock = vi.hoisted(() => vi.fn()); @@ -32,6 +39,19 @@ const registerSubCliByNameMock = vi.hoisted(() => ); const registerPluginCliCommandsFromValidatedConfigMock = vi.hoisted(() => vi.fn(async () => null)); +vi.mock("./output-file.runtime.js", async () => { + const actual = await vi.importActual( + "./output-file.runtime.js", + ); + outputFileMocks.publishOutputFileAtomically.mockImplementation( + actual.publishOutputFileAtomically, + ); + return { + ...actual, + publishOutputFileAtomically: outputFileMocks.publishOutputFileAtomically, + }; +}); + vi.mock("./program/command-registry-core.js", () => ({ getCoreCliCommandNames: getCoreCliCommandNamesMock, registerCoreCliByName: registerCoreCliByNameMock, @@ -75,6 +95,16 @@ async function withIsolatedCompletionState( } } +async function writeCompletionCacheForShell(shell: CompletionShell): Promise { + const { getCompletionScript, registerCompletionCli } = await import("./completion-cli.js"); + const program = new Command().name("openclaw"); + registerCompletionCli(program); + await program.parseAsync(["completion", "--shell", shell, "--write-state"], { + from: "user", + }); + return getCompletionScript(shell, program); +} + function expectCompletionInstallationToSkipRegistration(): void { expect(getProgramContextMock).not.toHaveBeenCalled(); expect(getCoreCliCommandNamesMock).not.toHaveBeenCalled(); @@ -88,7 +118,14 @@ function expectCompletionInstallationToSkipRegistration(): void { describe("completion-cli write-state", () => { let restoreStderrWriteSpy: (() => void) | null = null; - beforeEach(() => { + beforeEach(async () => { + const actual = await vi.importActual( + "./output-file.runtime.js", + ); + outputFileMocks.publishOutputFileAtomically.mockReset(); + outputFileMocks.publishOutputFileAtomically.mockImplementation( + actual.publishOutputFileAtomically, + ); stderrWrites.mockReset(); getCoreCliCommandNamesMock.mockClear(); registerCoreCliByNameMock.mockClear(); @@ -109,6 +146,150 @@ describe("completion-cli write-state", () => { restoreStderrWriteSpy?.(); }); + it.each(COMPLETION_SHELLS)( + "publishes %s completion atomically without changing existing file or directory modes", + async (shell) => { + await withIsolatedCompletionState(async () => { + const cachePath = resolveCompletionCachePath(shell, "openclaw"); + const cacheDir = path.dirname(cachePath); + await fs.mkdir(cacheDir, { recursive: true }); + await fs.writeFile(cachePath, "# previous completion\n", "utf8"); + if (process.platform !== "win32") { + await fs.chmod(cacheDir, 0o750); + await fs.chmod(cachePath, 0o640); + } + + const expectedScript = await writeCompletionCacheForShell(shell); + + await expect(fs.readFile(cachePath, "utf8")).resolves.toBe(expectedScript); + expect(outputFileMocks.publishOutputFileAtomically).toHaveBeenCalledWith( + expect.objectContaining({ + filePath: cachePath, + tempPrefix: ".openclaw-completion-cache", + }), + ); + expect(await fs.readdir(cacheDir)).toEqual([path.basename(cachePath)]); + if (process.platform !== "win32") { + expect((await fs.stat(cachePath)).mode & 0o777).toBe(0o640); + expect((await fs.stat(cacheDir)).mode & 0o777).toBe(0o750); + } + }); + }, + ); + + it.each(COMPLETION_SHELLS)( + "preserves the existing %s completion when staged publication fails", + async (shell) => { + const actual = await vi.importActual( + "./output-file.runtime.js", + ); + await withIsolatedCompletionState(async () => { + const cachePath = resolveCompletionCachePath(shell, "openclaw"); + const cacheDir = path.dirname(cachePath); + await fs.mkdir(cacheDir, { recursive: true }); + await fs.writeFile(cachePath, "# previous completion\n", "utf8"); + if (process.platform !== "win32") { + await fs.chmod(cacheDir, 0o750); + await fs.chmod(cachePath, 0o640); + } + outputFileMocks.publishOutputFileAtomically.mockImplementationOnce(async (params) => { + return await actual.publishOutputFileAtomically({ + ...params, + writeTemp: async (tempPath) => { + await params.writeTemp(tempPath); + await fs.truncate(tempPath, 1); + throw new Error("injected completion cache write failure"); + }, + }); + }); + + await expect(writeCompletionCacheForShell(shell)).rejects.toThrow( + "injected completion cache write failure", + ); + + await expect(fs.readFile(cachePath, "utf8")).resolves.toBe("# previous completion\n"); + expect(await fs.readdir(cacheDir)).toEqual([path.basename(cachePath)]); + if (process.platform !== "win32") { + expect((await fs.stat(cachePath)).mode & 0o777).toBe(0o640); + expect((await fs.stat(cacheDir)).mode & 0o777).toBe(0o750); + } + }); + }, + ); + + it.skipIf(process.platform === "win32")( + "replaces a completion cache symlink without overwriting its target", + async () => { + await withIsolatedCompletionState(async () => { + const cachePath = resolveCompletionCachePath("zsh", "openclaw"); + const cacheDir = path.dirname(cachePath); + const protectedPath = path.join(path.dirname(cacheDir), "protected-script"); + await fs.mkdir(cacheDir, { recursive: true }); + await fs.writeFile(protectedPath, "# protected script\n", "utf8"); + await fs.symlink(protectedPath, cachePath); + + const expectedScript = await writeCompletionCacheForShell("zsh"); + + expect((await fs.lstat(cachePath)).isSymbolicLink()).toBe(false); + await expect(fs.readFile(cachePath, "utf8")).resolves.toBe(expectedScript); + await expect(fs.readFile(protectedPath, "utf8")).resolves.toBe("# protected script\n"); + expect(await fs.readdir(cacheDir)).toEqual([path.basename(cachePath)]); + }); + }, + ); + + it.skipIf(process.platform === "win32")( + "rejects a planted temporary symlink without overwriting its target", + async () => { + const actual = await vi.importActual( + "./output-file.runtime.js", + ); + await withIsolatedCompletionState(async () => { + const cachePath = resolveCompletionCachePath("zsh", "openclaw"); + const cacheDir = path.dirname(cachePath); + const protectedPath = path.join(path.dirname(cacheDir), "protected-script"); + await fs.mkdir(cacheDir, { recursive: true }); + await fs.writeFile(cachePath, "# previous completion\n", "utf8"); + await fs.writeFile(protectedPath, "# protected script\n", "utf8"); + outputFileMocks.publishOutputFileAtomically.mockImplementationOnce(async (params) => { + return await actual.publishOutputFileAtomically({ + ...params, + writeTemp: async (tempPath) => { + await fs.symlink(protectedPath, tempPath); + return await params.writeTemp(tempPath); + }, + }); + }); + + await expect(writeCompletionCacheForShell("zsh")).rejects.toThrow(/EEXIST/u); + + await expect(fs.readFile(cachePath, "utf8")).resolves.toBe("# previous completion\n"); + await expect(fs.readFile(protectedPath, "utf8")).resolves.toBe("# protected script\n"); + expect(await fs.readdir(cacheDir)).toEqual([path.basename(cachePath)]); + }); + }, + ); + + it.skipIf(process.platform === "win32")( + "rejects a symlinked completion cache directory without writing into its target", + async () => { + await withIsolatedCompletionState(async () => { + const cachePath = resolveCompletionCachePath("zsh", "openclaw"); + const cacheDir = path.dirname(cachePath); + const protectedDir = path.join(path.dirname(cacheDir), "protected-directory"); + await fs.mkdir(protectedDir, { recursive: true }); + await fs.symlink(protectedDir, cacheDir, "dir"); + + await expect(writeCompletionCacheForShell("zsh")).rejects.toThrow( + "directory component must be a directory", + ); + + expect((await fs.lstat(cacheDir)).isSymbolicLink()).toBe(true); + expect(await fs.readdir(protectedDir)).toEqual([]); + }); + }, + ); + it.each(COMPLETION_SHELLS)( "installs cached %s completion without registering commands or plugins", async (shell) => {