From 0ec29289c68634c2e5a259024232ee04c4abacd8 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 26 May 2026 18:08:44 +0100 Subject: [PATCH] fix: tighten CLI utility failure handling (#86918) * fix: tighten cli utility failure handling * fix: preserve completion install error cause * fix: keep update completion refresh best effort --- scripts/ensure-cli-startup-build.mjs | 3 ++ src/cli/completion-runtime.test.ts | 17 +++++++ src/cli/completion-runtime.ts | 9 ++-- src/cli/nodes-cli/register.status.ts | 8 ++-- src/cli/program.nodes-basic.e2e.test.ts | 3 +- src/cli/update-cli.test.ts | 48 ++++++++++++++++++- src/cli/update-cli/update-command.ts | 13 ++++- test/scripts/ensure-cli-startup-build.test.ts | 12 +++++ 8 files changed, 99 insertions(+), 14 deletions(-) diff --git a/scripts/ensure-cli-startup-build.mjs b/scripts/ensure-cli-startup-build.mjs index 9ee2083b671..8f90788abfc 100644 --- a/scripts/ensure-cli-startup-build.mjs +++ b/scripts/ensure-cli-startup-build.mjs @@ -30,6 +30,9 @@ export function ensureCliStartupBuild(params = {}) { env: params.env ?? process.env, stdio: params.stdio ?? "inherit", }); + if (result.error) { + throw result.error; + } const status = result.status ?? (result.signal ? 1 : 0); if (status !== 0) { throw new Error(`cliStartup build profile failed with exit code ${status}`); diff --git a/src/cli/completion-runtime.test.ts b/src/cli/completion-runtime.test.ts index 2bab3734b0d..c46ef1b34a6 100644 --- a/src/cli/completion-runtime.test.ts +++ b/src/cli/completion-runtime.test.ts @@ -107,4 +107,21 @@ describe("completion-runtime", () => { await fs.rm(stateDir, { recursive: true, force: true }); } }); + + it("rejects install when the completion cache is missing", async () => { + const homeDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-completion-home-")); + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-completion-state-")); + + process.env.HOME = homeDir; + process.env.OPENCLAW_STATE_DIR = stateDir; + + try { + await expect(installCompletion("zsh", true, "openclaw")).rejects.toThrow( + "Completion cache not found", + ); + } finally { + await fs.rm(homeDir, { recursive: true, force: true }); + await fs.rm(stateDir, { recursive: true, force: true }); + } + }); }); diff --git a/src/cli/completion-runtime.ts b/src/cli/completion-runtime.ts index 44d67745d66..a64042a7dc8 100644 --- a/src/cli/completion-runtime.ts +++ b/src/cli/completion-runtime.ts @@ -233,17 +233,15 @@ export async function usesSlowDynamicCompletion( export async function installCompletion(shell: string, yes: boolean, binName = "openclaw") { const isShellSupported = isCompletionShell(shell); if (!isShellSupported) { - console.error(`Automated installation not supported for ${shell} yet.`); - return; + throw new Error(`Automated installation not supported for ${shell} yet.`); } const cachePath = resolveCompletionCachePath(shell, binName); const cacheExists = await pathExists(cachePath); if (!cacheExists) { - console.error( + throw new Error( `Completion cache not found at ${cachePath}. Run \`${binName} completion --write-state\` first.`, ); - return; } let profilePath: string; @@ -305,6 +303,7 @@ export async function installCompletion(shell: string, yes: boolean, binName = " ); } } catch (err) { - console.error(`Failed to install completion: ${err as string}`); + const message = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to install completion: ${message}`, { cause: err }); } } diff --git a/src/cli/nodes-cli/register.status.ts b/src/cli/nodes-cli/register.status.ts index e5c6fbe714b..8f2ec6ada99 100644 --- a/src/cli/nodes-cli/register.status.ts +++ b/src/cli/nodes-cli/register.status.ts @@ -427,10 +427,6 @@ export function registerNodesStatusCommands(nodes: Command) { }); const filteredLabel = hasFilters && filteredPaired.length !== paired.length ? ` (of ${paired.length})` : ""; - defaultRuntime.log( - `Pending: ${pendingRows.length} · Paired: ${filteredPaired.length}${filteredLabel}`, - ); - if (opts.json) { defaultRuntime.writeJson({ pending: pendingRows, @@ -439,6 +435,10 @@ export function registerNodesStatusCommands(nodes: Command) { return; } + defaultRuntime.log( + `Pending: ${pendingRows.length} · Paired: ${filteredPaired.length}${filteredLabel}`, + ); + if (pendingRows.length > 0) { const rendered = renderPendingPairingRequestsTable({ pending: pendingRows, diff --git a/src/cli/program.nodes-basic.e2e.test.ts b/src/cli/program.nodes-basic.e2e.test.ts index 4c2c5de6f60..ebfa7e39653 100644 --- a/src/cli/program.nodes-basic.e2e.test.ts +++ b/src/cli/program.nodes-basic.e2e.test.ts @@ -195,7 +195,8 @@ describe("cli program (nodes basics)", () => { expect(JSON.stringify(json)).not.toContain("paired-token"); expect(JSON.stringify(json)).not.toContain("pair-only-token"); const output = getRuntimeOutput(); - expect(output).toContain("Pending: 1 · Paired: 3"); + expect(output).toMatch(/^\{/); + expect(output).not.toContain("Pending: 1 · Paired: 3"); expect(output).not.toContain("Effective Only Unknown"); expect(output).not.toContain("unpaired-live"); }); diff --git a/src/cli/update-cli.test.ts b/src/cli/update-cli.test.ts index 7c15a05bba5..c1f9b11b5a1 100644 --- a/src/cli/update-cli.test.ts +++ b/src/cli/update-cli.test.ts @@ -44,6 +44,9 @@ const updateNpmInstalledPlugins = vi.fn(); const loadInstalledPluginIndexInstallRecords = vi.fn( async (params: { config?: OpenClawConfig } = {}) => params.config?.plugins?.installs ?? {}, ); +const checkShellCompletionStatus = vi.fn(); +const ensureCompletionCacheExists = vi.fn(); +const installCompletion = vi.fn(); const legacyConfigRepairMocks = vi.hoisted(() => ({ repairLegacyConfigForUpdateChannel: vi.fn(), })); @@ -277,9 +280,20 @@ vi.mock("./update-cli/restart-helper.js", () => ({ vi.mock("../commands/doctor.js", () => ({ doctorCommand: vi.fn(), })); +vi.mock("../commands/doctor-completion.js", () => ({ + checkShellCompletionStatus: (...args: unknown[]) => checkShellCompletionStatus(...args), + ensureCompletionCacheExists: (...args: unknown[]) => ensureCompletionCacheExists(...args), +})); vi.mock("../commands/doctor/legacy-config-repair.js", () => ({ repairLegacyConfigForUpdateChannel: legacyConfigRepairMocks.repairLegacyConfigForUpdateChannel, })); +vi.mock("./completion-runtime.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + installCompletion: (...args: unknown[]) => installCompletion(...args), + }; +}); // Mock the daemon-cli module vi.mock("./daemon-cli.js", () => ({ runDaemonInstall: mockedRunDaemonInstall, @@ -793,6 +807,15 @@ describe("update-cli", () => { config: baseConfig, outcomes: [], }); + checkShellCompletionStatus.mockResolvedValue({ + shell: "zsh", + profileInstalled: false, + cacheExists: false, + cachePath: "/tmp/openclaw-completion.zsh", + usesSlowPattern: false, + }); + ensureCompletionCacheExists.mockResolvedValue(true); + installCompletion.mockResolvedValue(undefined); vi.mocked(runDaemonInstall).mockResolvedValue(undefined); vi.mocked(runDaemonRestart).mockResolvedValue(true); vi.mocked(doctorCommand).mockResolvedValue(undefined); @@ -883,6 +906,28 @@ describe("update-cli", () => { expect(logOutput).not.toContain("Error: spawnSync"); }); + it("keeps update completion refresh best-effort when profile install fails", async () => { + setTty(true); + checkShellCompletionStatus.mockResolvedValue({ + shell: "zsh", + profileInstalled: true, + cacheExists: true, + cachePath: "/tmp/openclaw-completion.zsh", + usesSlowPattern: true, + }); + installCompletion.mockRejectedValueOnce(new Error("EACCES: permission denied")); + + await updateCommand({ yes: true, restart: false }); + + expect(installCompletion).toHaveBeenCalledWith("zsh", true, "openclaw"); + const logOutput = vi + .mocked(runtimeCapture.log) + .mock.calls.map((call) => String(call[0])) + .join("\n"); + expect(logOutput).toContain("Shell completion refresh failed: EACCES: permission denied"); + expect(defaultRuntime.exit).not.toHaveBeenCalledWith(1); + }); + it("respawns into the updated package root before running post-update tasks", async () => { const { entrypoints } = setupUpdatedRootRefresh(); @@ -1186,8 +1231,7 @@ describe("update-cli", () => { .mocked(readConfigFileSnapshot) .mock.calls.some( ([options]) => - options?.skipPluginValidation === true && - options.suppressFutureVersionWarning === true, + options?.skipPluginValidation === true && options.suppressFutureVersionWarning === true, ), ).toBe(true); expect(defaultRuntime.exit).toHaveBeenCalledWith(0); diff --git a/src/cli/update-cli/update-command.ts b/src/cli/update-cli/update-command.ts index f78f977c3db..7108b4efe7d 100644 --- a/src/cli/update-cli/update-command.ts +++ b/src/cli/update-cli/update-command.ts @@ -1268,7 +1268,7 @@ async function tryInstallShellCompletion(opts: { defaultRuntime.log(theme.muted("Upgrading shell completion to cached version...")); const cacheGenerated = await ensureCompletionCacheExists(CLI_NAME); if (cacheGenerated) { - await installCompletion(status.shell, true, CLI_NAME); + await installShellCompletionForUpdate(status.shell, true); } return; } @@ -1305,7 +1305,16 @@ async function tryInstallShellCompletion(opts: { return; } - await installCompletion(status.shell, opts.skipPrompt, CLI_NAME); + await installShellCompletionForUpdate(status.shell, opts.skipPrompt); + } +} + +async function installShellCompletionForUpdate(shell: string, yes: boolean): Promise { + try { + await installCompletion(shell, yes, CLI_NAME); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + defaultRuntime.log(theme.warn(`Shell completion refresh failed: ${message}`)); } } diff --git a/test/scripts/ensure-cli-startup-build.test.ts b/test/scripts/ensure-cli-startup-build.test.ts index 9e81027e9a8..e879c357efc 100644 --- a/test/scripts/ensure-cli-startup-build.test.ts +++ b/test/scripts/ensure-cli-startup-build.test.ts @@ -85,4 +85,16 @@ describe("ensure-cli-startup-build", () => { }), ).toThrow("cliStartup build profile failed with exit code 1"); }); + + it("fails when spawning the cliStartup build profile fails", () => { + const root = makeTempRoot(); + + expect(() => + ensureCliStartupBuild({ + rootDir: root, + spawnSync: () => ({ error: new Error("spawn denied") }), + stdio: "pipe", + }), + ).toThrow("spawn denied"); + }); });