From a0e591c86344dc19c0ebebc96dde66c19b3a3464 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 6 Jul 2026 05:26:37 +0100 Subject: [PATCH] fix(cli): reduce plugin hook fallback noise (#100554) * fix(cli): reduce plugin hook fallback noise Co-authored-by: Vincent Koc * docs(changelog): defer plugin diagnostic note --------- Co-authored-by: Vincent Koc --- src/cli/plugins-cli-test-helpers.ts | 4 ++++ src/cli/plugins-cli.install.test.ts | 28 ++++++++++++++++++++++- src/cli/plugins-command-helpers.ts | 8 +++++-- src/cli/plugins-install-command.ts | 10 ++++----- src/hooks/install.test.ts | 31 +++++++++++++++++++++++-- src/hooks/install.ts | 35 +++++++++++++++++++++-------- 6 files changed, 97 insertions(+), 19 deletions(-) diff --git a/src/cli/plugins-cli-test-helpers.ts b/src/cli/plugins-cli-test-helpers.ts index 2167374c097e..ef0605ae021a 100644 --- a/src/cli/plugins-cli-test-helpers.ts +++ b/src/cli/plugins-cli-test-helpers.ts @@ -624,6 +624,10 @@ vi.mock("../plugins/git-install.js", () => ({ })); vi.mock("../hooks/install.js", () => ({ + HOOK_INSTALL_ERROR_CODE: { + MISSING_OPENCLAW_HOOKS: "missing_openclaw_hooks", + EMPTY_OPENCLAW_HOOKS: "empty_openclaw_hooks", + }, installHooksFromNpmSpec: (( ...args: Parameters<(typeof import("../hooks/install.js"))["installHooksFromNpmSpec"]> ) => diff --git a/src/cli/plugins-cli.install.test.ts b/src/cli/plugins-cli.install.test.ts index 5720b90087de..cc3f075c3f60 100644 --- a/src/cli/plugins-cli.install.test.ts +++ b/src/cli/plugins-cli.install.test.ts @@ -1984,6 +1984,7 @@ describe("plugins cli install", () => { installHooksFromNpmSpec.mockResolvedValue({ ok: false, error: "package.json missing openclaw.hooks", + code: "missing_openclaw_hooks", }); await expect(runPluginsCommand(["plugins", "install", "npm:demo"])).rejects.toThrow( @@ -1992,6 +1993,28 @@ describe("plugins cli install", () => { expect(installPluginFromClawHub).not.toHaveBeenCalled(); expect(runtimeErrors.at(-1)).toContain("npm install failed"); + expect(runtimeErrors.at(-1)).not.toContain("Also not a valid hook pack"); + }); + + it("keeps actionable hook-pack fallback details", async () => { + loadConfig.mockReturnValue({} as OpenClawConfig); + installPluginFromNpmSpec.mockResolvedValue({ + ok: false, + error: "npm install failed", + }); + installHooksFromNpmSpec.mockResolvedValue({ + ok: false, + error: "HOOK.md missing in /tmp/demo-hook", + }); + + await expect(runPluginsCommand(["plugins", "install", "npm:demo-hook"])).rejects.toThrow( + "__exit__:1", + ); + + expect(runtimeErrors.at(-1)).toContain("npm install failed"); + expect(runtimeErrors.at(-1)).toContain( + "Also not a valid hook pack: HOOK.md missing in /tmp/demo-hook", + ); }); it("adds a Git PATH hint when npm plugin dependency install cannot spawn git", async () => { @@ -2008,6 +2031,7 @@ describe("plugins cli install", () => { installHooksFromNpmSpec.mockResolvedValue({ ok: false, error: "package.json missing openclaw.hooks", + code: "missing_openclaw_hooks", }); await expect( @@ -2019,7 +2043,7 @@ describe("plugins cli install", () => { "one of this plugin's npm dependencies is fetched from a git URL", ); expect(runtimeErrors.at(-1)).toContain("winget install --id Git.Git -e"); - expect(runtimeErrors.at(-1)).toContain("Also not a valid hook pack"); + expect(runtimeErrors.at(-1)).not.toContain("Also not a valid hook pack"); }); it("does not resolve npm: prefixed bundled plugin ids through bundled installs", async () => { @@ -2032,6 +2056,7 @@ describe("plugins cli install", () => { installHooksFromNpmSpec.mockResolvedValue({ ok: false, error: "package.json missing openclaw.hooks", + code: "missing_openclaw_hooks", }); await expect(runPluginsCommand(["plugins", "install", "npm:memory-lancedb"])).rejects.toThrow( @@ -2042,6 +2067,7 @@ describe("plugins cli install", () => { expect(installPluginFromClawHub).not.toHaveBeenCalled(); expect(writeConfigFile).not.toHaveBeenCalled(); expect(runtimeErrors.at(-1)).toContain("Package not found on npm: memory-lancedb."); + expect(runtimeErrors.at(-1)).not.toContain("Also not a valid hook pack"); }); it("rejects empty npm: prefix installs before resolver lookup", async () => { diff --git a/src/cli/plugins-command-helpers.ts b/src/cli/plugins-command-helpers.ts index b2bac96e7717..bf11aac6ec44 100644 --- a/src/cli/plugins-command-helpers.ts +++ b/src/cli/plugins-command-helpers.ts @@ -2,6 +2,7 @@ import { normalizeLowercaseStringOrEmpty } from "@openclaw/normalization-core/string-coerce"; import { theme } from "../../packages/terminal-core/src/theme.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { HOOK_INSTALL_ERROR_CODE } from "../hooks/install.js"; import type { PluginKind } from "../plugins/plugin-kind.types.js"; import { loadPluginMetadataSnapshot } from "../plugins/plugin-metadata-snapshot.js"; import { applyExclusiveSlotSelection } from "../plugins/slots.js"; @@ -169,10 +170,10 @@ export function enableInternalHookEntries( export function formatPluginInstallWithHookFallbackError( pluginError: string, - hookError: string, + hookFallback: { error: string; code?: string }, ): string { const formattedPluginError = formatPluginInstallAttemptError(pluginError); - const formattedHookError = formatPluginInstallAttemptError(hookError); + const formattedHookError = formatPluginInstallAttemptError(hookFallback.error); if (/plugin already exists: .+ \(delete it first\)/.test(pluginError)) { return `${formattedPluginError}\nUse \`openclaw plugins update \` to upgrade the tracked plugin, or rerun install with \`--force\` to replace it.`; } @@ -182,6 +183,9 @@ export function formatPluginInstallWithHookFallbackError( ) { return formattedPluginError; } + if (hookFallback.code === HOOK_INSTALL_ERROR_CODE.MISSING_OPENCLAW_HOOKS) { + return formattedPluginError; + } return `${formattedPluginError}\nAlso not a valid hook pack: ${formattedHookError}`; } diff --git a/src/cli/plugins-install-command.ts b/src/cli/plugins-install-command.ts index 06f7afab55ee..4e47ac73ccc9 100644 --- a/src/cli/plugins-install-command.ts +++ b/src/cli/plugins-install-command.ts @@ -266,7 +266,7 @@ async function tryInstallHookPackFromLocalPath(params: { link?: boolean; expectedPackageKind?: "hook-only"; runtime?: RuntimeEnv; -}): Promise<{ ok: true } | { ok: false; error: string }> { +}): Promise<{ ok: true } | Extract> { if (params.snapshot.hookMutation.mode === "blocked") { return { ok: false, error: params.snapshot.hookMutation.reason }; } @@ -358,7 +358,7 @@ async function tryInstallHookPackFromNpmSpec(params: { expectedIntegrity?: string; expectedPackageKind?: "hook-only"; runtime?: RuntimeEnv; -}): Promise<{ ok: true } | { ok: false; error: string }> { +}): Promise<{ ok: true } | Extract> { if (params.snapshot.hookMutation.mode === "blocked") { return { ok: false, error: params.snapshot.hookMutation.reason }; } @@ -497,7 +497,7 @@ async function tryInstallPluginOrHookPackFromNpmSpec(params: { return { ok: true }; } (params.runtime ?? defaultRuntime).error( - formatPluginInstallWithHookFallbackError(result.error, hookFallback.error), + formatPluginInstallWithHookFallbackError(result.error, hookFallback), ); return { ok: false }; } @@ -1093,7 +1093,7 @@ export async function runPluginInstallCommand(params: { if (hookFallback.ok) { return; } - runtime.error(formatPluginInstallWithHookFallbackError(probe.error, hookFallback.error)); + runtime.error(formatPluginInstallWithHookFallbackError(probe.error, hookFallback)); return runtime.exit(1); } @@ -1147,7 +1147,7 @@ export async function runPluginInstallCommand(params: { if (hookFallback.ok) { return; } - runtime.error(formatPluginInstallWithHookFallbackError(result.error, hookFallback.error)); + runtime.error(formatPluginInstallWithHookFallbackError(result.error, hookFallback)); return runtime.exit(1); } diff --git a/src/hooks/install.test.ts b/src/hooks/install.test.ts index 2e93612be4ee..95a2a87c56f6 100644 --- a/src/hooks/install.test.ts +++ b/src/hooks/install.test.ts @@ -33,8 +33,12 @@ vi.mock("../plugins/install-security-scan.js", () => ({ vi.resetModules(); -const { installHooksFromArchive, installHooksFromNpmSpec, installHooksFromPath } = - await import("./install.js"); +const { + HOOK_INSTALL_ERROR_CODE, + installHooksFromArchive, + installHooksFromNpmSpec, + installHooksFromPath, +} = await import("./install.js"); const hookInstallRuntime = await import("./install.runtime.js"); const fixtureRoot = path.join(process.cwd(), ".tmp", `openclaw-hook-install-${randomUUID()}`); @@ -322,6 +326,29 @@ describe("installHooksFromArchive", () => { }); describe("installHooksFromPath", () => { + it.each([ + { + openclaw: {}, + error: "package.json missing openclaw.hooks", + code: HOOK_INSTALL_ERROR_CODE.MISSING_OPENCLAW_HOOKS, + }, + { + openclaw: { hooks: [] }, + error: "package.json openclaw.hooks is empty", + code: HOOK_INSTALL_ERROR_CODE.EMPTY_OPENCLAW_HOOKS, + }, + ])("returns a stable code for $error", async ({ openclaw, error, code }) => { + const pkgDir = makeTempDir(); + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ name: "@openclaw/test-hooks", openclaw }), + ); + + const result = await installHooksFromPath({ path: pkgDir, hooksDir: makeTempDir() }); + + expect(result).toEqual({ ok: false, error, code }); + }); + it("uses --ignore-scripts for dependency install", async () => { const workDir = makeTempDir(); const stateDir = makeTempDir(); diff --git a/src/hooks/install.ts b/src/hooks/install.ts index 380c0a216be2..b39b21130339 100644 --- a/src/hooks/install.ts +++ b/src/hooks/install.ts @@ -48,6 +48,14 @@ export type InstallHooksResult = code?: string; }; +export const HOOK_INSTALL_ERROR_CODE = { + MISSING_OPENCLAW_HOOKS: "missing_openclaw_hooks", + EMPTY_OPENCLAW_HOOKS: "empty_openclaw_hooks", +} as const; + +export type HookInstallErrorCode = + (typeof HOOK_INSTALL_ERROR_CODE)[keyof typeof HOOK_INSTALL_ERROR_CODE]; + /** Integrity drift payload surfaced when npm metadata no longer matches an install record. */ export type HookNpmIntegrityDriftParams = { spec: string; @@ -219,16 +227,26 @@ export function resolveHookInstallDir(hookId: string, hooksDir?: string): string return targetDirResult.path; } -async function ensureOpenClawHooks(manifest: HookPackageManifest) { +function resolveOpenClawHooks( + manifest: HookPackageManifest, +): { ok: true; entries: string[] } | { ok: false; error: string; code: HookInstallErrorCode } { const hooks = manifest[MANIFEST_KEY]?.hooks; if (!Array.isArray(hooks)) { - throw new Error("package.json missing openclaw.hooks"); + return { + ok: false, + error: "package.json missing openclaw.hooks", + code: HOOK_INSTALL_ERROR_CODE.MISSING_OPENCLAW_HOOKS, + }; } const list = normalizeTrimmedStringList(hooks); if (list.length === 0) { - throw new Error("package.json openclaw.hooks is empty"); + return { + ok: false, + error: "package.json openclaw.hooks is empty", + code: HOOK_INSTALL_ERROR_CODE.EMPTY_OPENCLAW_HOOKS, + }; } - return list; + return { ok: true, entries: list }; } function resolveHookPackageKind( @@ -386,12 +404,11 @@ async function installHookPackageFromDir( return { ok: false, error: `invalid package.json: ${String(err)}` }; } - let hookEntries: string[]; - try { - hookEntries = await ensureOpenClawHooks(manifest); - } catch (err) { - return { ok: false, error: String(err) }; + const hookManifest = resolveOpenClawHooks(manifest); + if (!hookManifest.ok) { + return hookManifest; } + const hookEntries = hookManifest.entries; const pkgName = typeof manifest.name === "string" ? manifest.name : ""; const hookPackId = pkgName ? unscopedPackageName(pkgName) : path.basename(params.packageDir);