From 06b3e4ef8aa0fa202e406a34adb96a2eaf91e6fb Mon Sep 17 00:00:00 2001 From: Alex Knight Date: Mon, 27 Apr 2026 15:46:50 +1000 Subject: [PATCH] Fail invalid plugin registration gates loudly (#72577) * fix plugin registration gate failures --- CHANGELOG.md | 1 + docs/plugins/sdk-testing.md | 13 ++++++ src/plugins/loader.test.ts | 88 +++++++++++++++++++++++++++++++++++++ src/plugins/registry.ts | 75 ++++++++++++------------------- 4 files changed, 131 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96e168f54e1..d838a2ae495 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ Docs: https://docs.openclaw.ai - Cron: classify isolated runs as errors from structured embedded-run execution-denial metadata, with final-output marker fallback for `SYSTEM_RUN_DENIED`, `INVALID_REQUEST`, and approval-binding refusals, so blocked commands no longer appear green in cron history. Fixes #67172; carries forward #67186. Thanks @oc-gh-dr, @hclsys, and @1yihui. - Onboarding/GitHub Copilot: add manifest-owned `--github-copilot-token` support for non-interactive setup, including env fallback, tokenRef storage in ref mode, saved-profile reuse, and current Copilot default-model wiring. Refs #50002 and supersedes #50003. Thanks @scottgl9. - Gateway/install: add a validated `--wrapper`/`OPENCLAW_WRAPPER` service install path that persists executable LaunchAgent/systemd wrappers across forced reinstalls, updates, and doctor repairs instead of falling back to raw node/bun `ProgramArguments`. Fixes #69400. (#72445) Thanks @willtmc. +- Plugins: fail plugin registration when loader-owned acceptance gates reject missing hook names or memory-only capability registration from non-memory plugins, surfacing the issue through plugin status and doctor instead of silently dropping the registration. Fixes #72459. Thanks @1fanwang and @amknight. - macOS Gateway: write launchd services with a state-dir `WorkingDirectory`, use a durable state-dir temp path instead of freezing macOS session `TMPDIR`, create that temp directory before bootstrap, and label abort-shaped launchd exits as `SIGABRT/abort` in status output. Fixes #53679 and #70223; refs #71848. Thanks @dlturock, @stammi922, and @palladius. - Exec approvals: accept runtime-owned `source: "allow-always"` and `commandText` allowlist metadata in gateway and node approval-set payloads so Control UI round-trips no longer fail with `unexpected property 'source'`. Fixes #60000; carries forward #60064. Thanks @sd1471123, @sharkqwy, and @luoyanglang. - Exec/node: skip approval-plan preparation for full-trust `host=node` runs so interpreter and script commands no longer fail with `SYSTEM_RUN_DENIED: approval cannot safely bind` when effective policy is `security=full` and `ask=off`. Fixes #48457 and duplicate #69251. Thanks @ajtran303, @jaserNo1, @Blakeshannon, @lesliefag, and @AvIsBeastMC. diff --git a/docs/plugins/sdk-testing.md b/docs/plugins/sdk-testing.md index dfd874f6a93..aeff3bcf898 100644 --- a/docs/plugins/sdk-testing.md +++ b/docs/plugins/sdk-testing.md @@ -81,6 +81,19 @@ describe("my-channel target resolution", () => { ## Testing patterns +### Testing registration contracts + +Unit tests that pass a hand-written `api` mock to `register(api)` do not exercise +OpenClaw's loader acceptance gates. Add at least one loader-backed smoke test +for each registration surface your plugin depends on, especially hooks and +exclusive capabilities such as memory. + +The real loader fails plugin registration when required metadata is missing or a +plugin calls a capability API it does not own. For example, +`api.registerHook(...)` requires a hook name, and +`api.registerMemoryCapability(...)` requires the plugin manifest or exported +entry to declare `kind: "memory"`. + ### Unit testing a channel plugin ```typescript diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index 71272bdebc0..2d89d9b37b3 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -66,6 +66,7 @@ import { import { buildMemoryPromptSection, clearMemoryPluginState, + getMemoryCapabilityRegistration, getMemoryRuntime, listActiveMemoryPublicArtifacts, listMemoryCorpusSupplements, @@ -3121,6 +3122,93 @@ module.exports = { id: "throws-after-import", register() {} };`, clearPluginInteractiveHandlers(); }); + it("fails plugin registration when a hook is missing its required name", () => { + useNoBundledPlugins(); + const plugin = writePlugin({ + id: "nameless-hook", + filename: "nameless-hook.cjs", + body: `module.exports = { + id: "nameless-hook", + register(api) { + api.registerHook("gateway:startup", () => {}); + }, + };`, + }); + + clearInternalHooks(); + + const registry = loadOpenClawPlugins({ + cache: false, + workspaceDir: plugin.dir, + config: { + plugins: { + load: { paths: [plugin.file] }, + allow: ["nameless-hook"], + }, + }, + onlyPluginIds: ["nameless-hook"], + }); + + const record = registry.plugins.find((entry) => entry.id === "nameless-hook"); + expect(record?.status).toBe("error"); + expect(record?.failurePhase).toBe("register"); + expect(record?.error).toContain("hook registration missing name"); + expect(registry.hooks).toEqual([]); + expect(getRegisteredEventKeys()).toEqual([]); + expect( + registry.diagnostics.some( + (diag) => + diag.pluginId === "nameless-hook" && + diag.level === "error" && + diag.message.includes("hook registration missing name"), + ), + ).toBe(true); + + clearInternalHooks(); + }); + + it("fails plugin registration when a non-memory plugin registers a memory capability", () => { + useNoBundledPlugins(); + const plugin = writePlugin({ + id: "invalid-memory-capability", + filename: "invalid-memory-capability.cjs", + body: `module.exports = { + id: "invalid-memory-capability", + register(api) { + api.registerMemoryCapability({ + promptBuilder: () => ["should not register"], + }); + }, + };`, + }); + + const registry = loadOpenClawPlugins({ + cache: false, + workspaceDir: plugin.dir, + config: { + plugins: { + load: { paths: [plugin.file] }, + allow: ["invalid-memory-capability"], + }, + }, + onlyPluginIds: ["invalid-memory-capability"], + }); + + const record = registry.plugins.find((entry) => entry.id === "invalid-memory-capability"); + expect(record?.status).toBe("error"); + expect(record?.failurePhase).toBe("register"); + expect(record?.error).toContain("only memory plugins can register a memory capability"); + expect(getMemoryCapabilityRegistration()).toBeUndefined(); + expect( + registry.diagnostics.some( + (diag) => + diag.pluginId === "invalid-memory-capability" && + diag.level === "error" && + diag.message.includes("only memory plugins can register a memory capability"), + ), + ).toBe(true); + }); + it("can scope bundled provider loads to deepseek without hanging", () => { const scoped = loadOpenClawPlugins({ cache: false, diff --git a/src/plugins/registry.ts b/src/plugins/registry.ts index 86e43cb9d85..2cc1fea101c 100644 --- a/src/plugins/registry.ts +++ b/src/plugins/registry.ts @@ -235,6 +235,17 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { registry.diagnostics.push(diag); }; + const throwRegistrationError = (message: string): never => { + throw new Error(message); + }; + + const requireRegistrationValue = (value: string | undefined, message: string): string => { + if (!value) { + throw new Error(message); + } + return value; + }; + const registerCodexAppServerExtensionFactory = ( record: PluginRecord, factory: Parameters[0], @@ -414,23 +425,17 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { const eventList = Array.isArray(events) ? events : [events]; const normalizedEvents = eventList.map((event) => event.trim()).filter(Boolean); const entry = opts?.entry ?? null; - const name = entry?.hook.name ?? opts?.name?.trim(); - if (!name) { - pushDiagnostic({ - level: "warn", - pluginId: record.id, - source: record.source, - message: "hook registration missing name", - }); - return; - } - const existingHook = registry.hooks.find((entry) => entry.entry.hook.name === name); + const hookName = requireRegistrationValue( + entry?.hook.name ?? opts?.name?.trim(), + "hook registration missing name", + ); + const existingHook = registry.hooks.find((entry) => entry.entry.hook.name === hookName); if (existingHook) { pushDiagnostic({ level: "error", pluginId: record.id, source: record.source, - message: `hook already registered: ${name} (${existingHook.pluginId})`, + message: `hook already registered: ${hookName} (${existingHook.pluginId})`, }); return; } @@ -441,7 +446,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { ...entry, hook: { ...entry.hook, - name, + name: hookName, description, source: "openclaw-plugin", pluginId: record.id, @@ -453,7 +458,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { } : { hook: { - name, + name: hookName, description, source: "openclaw-plugin", pluginId: record.id, @@ -466,7 +471,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { invocation: { enabled: true }, }; - record.hookNames.push(name); + record.hookNames.push(hookName); registry.hooks.push({ pluginId: record.id, entry: hookEntry, @@ -483,7 +488,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { return; } - const previousRegistrations = activePluginHookRegistrations.get(name) ?? []; + const previousRegistrations = activePluginHookRegistrations.get(hookName) ?? []; for (const registration of previousRegistrations) { unregisterInternalHook(registration.event, registration.handler); } @@ -496,10 +501,10 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { registerInternalHook(event, handler); nextRegistrations.push({ event, handler }); } - activePluginHookRegistrations.set(name, nextRegistrations); + activePluginHookRegistrations.set(hookName, nextRegistrations); const rollbackEntries = pluginHookRollback.get(record.id) ?? []; rollbackEntries.push({ - name, + name: hookName, previousRegistrations: [...previousRegistrations], }); pluginHookRollback.set(record.id, rollbackEntries); @@ -1562,13 +1567,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { }, registerMemoryCapability: (capability) => { if (!hasKind(record.kind, "memory")) { - pushDiagnostic({ - level: "error", - pluginId: record.id, - source: record.source, - message: "only memory plugins can register a memory capability", - }); - return; + throwRegistrationError("only memory plugins can register a memory capability"); } if ( Array.isArray(record.kind) && @@ -1588,13 +1587,9 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { }, registerMemoryPromptSection: (builder) => { if (!hasKind(record.kind, "memory")) { - pushDiagnostic({ - level: "error", - pluginId: record.id, - source: record.source, - message: "only memory plugins can register a memory prompt section", - }); - return; + throwRegistrationError( + "only memory plugins can register a memory prompt section", + ); } if ( Array.isArray(record.kind) && @@ -1620,13 +1615,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { }, registerMemoryFlushPlan: (resolver) => { if (!hasKind(record.kind, "memory")) { - pushDiagnostic({ - level: "error", - pluginId: record.id, - source: record.source, - message: "only memory plugins can register a memory flush plan", - }); - return; + throwRegistrationError("only memory plugins can register a memory flush plan"); } if ( Array.isArray(record.kind) && @@ -1646,13 +1635,7 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { }, registerMemoryRuntime: (runtime) => { if (!hasKind(record.kind, "memory")) { - pushDiagnostic({ - level: "error", - pluginId: record.id, - source: record.source, - message: "only memory plugins can register a memory runtime", - }); - return; + throwRegistrationError("only memory plugins can register a memory runtime"); } if ( Array.isArray(record.kind) &&