From 2a283e87a74129fce8f5daea1eaece508e1b1df3 Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Fri, 17 Apr 2026 08:34:48 +0530 Subject: [PATCH] fix(plugins): enforce synchronous registration --- extensions/voice-call/index.test.ts | 4 +- extensions/webhooks/index.test.ts | 66 +++++++ extensions/webhooks/index.ts | 82 ++++---- extensions/webhooks/runtime-api.ts | 1 + extensions/webhooks/src/config.test.ts | 51 ++--- extensions/webhooks/src/config.ts | 44 ++--- extensions/webhooks/src/http.test.ts | 38 ++-- extensions/webhooks/src/http.ts | 25 ++- src/plugins/cli-registry-loader.ts | 72 +------ src/plugins/cli.test.ts | 52 ----- src/plugins/loader.cli-metadata.test.ts | 11 +- src/plugins/loader.test.ts | 33 ++++ src/plugins/loader.ts | 179 ++++++++++++++++-- src/plugins/types.ts | 8 +- src/test-utils/plugin-registration.ts | 8 +- .../bundled-provider-builders.ts | 2 +- test/helpers/plugins/provider-registration.ts | 4 +- 17 files changed, 411 insertions(+), 269 deletions(-) create mode 100644 extensions/webhooks/index.test.ts diff --git a/extensions/voice-call/index.test.ts b/extensions/voice-call/index.test.ts index 3fc37167dbb..b22098f6b2a 100644 --- a/extensions/voice-call/index.test.ts +++ b/extensions/voice-call/index.test.ts @@ -37,7 +37,7 @@ type Registered = { methods: Map; tools: unknown[]; }; -type RegisterVoiceCall = (api: Record) => void | Promise; +type RegisterVoiceCall = (api: Record) => void; type RegisterCliContext = { program: Command; config: Record; @@ -83,7 +83,7 @@ async function registerVoiceCallCli(program: Command) { const { register } = plugin as unknown as { register: RegisterVoiceCall; }; - await register({ + register({ id: "voice-call", name: "Voice Call", description: "test", diff --git a/extensions/webhooks/index.test.ts b/extensions/webhooks/index.test.ts new file mode 100644 index 00000000000..14e60b0d099 --- /dev/null +++ b/extensions/webhooks/index.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it, vi } from "vitest"; +import { createTestPluginApi } from "../../test/helpers/plugins/plugin-api.js"; +import type { OpenClawPluginApi } from "./api.js"; +import plugin from "./index.js"; + +function createApi(params?: { + pluginConfig?: OpenClawPluginApi["pluginConfig"]; + registerHttpRoute?: OpenClawPluginApi["registerHttpRoute"]; + logger?: OpenClawPluginApi["logger"]; +}): OpenClawPluginApi { + return createTestPluginApi({ + id: "webhooks", + name: "Webhooks", + source: "test", + pluginConfig: params?.pluginConfig ?? {}, + runtime: { + taskFlow: { + bindSession: vi.fn(({ sessionKey }: { sessionKey: string }) => ({ sessionKey })), + }, + } as unknown as OpenClawPluginApi["runtime"], + registerHttpRoute: params?.registerHttpRoute ?? vi.fn(), + logger: + params?.logger ?? + ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + } as OpenClawPluginApi["logger"]), + }); +} + +describe("webhooks plugin registration", () => { + it("registers SecretRef-backed routes synchronously", () => { + const registerHttpRoute = vi.fn(); + + const result = plugin.register( + createApi({ + pluginConfig: { + routes: { + zapier: { + sessionKey: "agent:main:main", + secret: { + source: "env", + provider: "default", + id: "OPENCLAW_WEBHOOK_SECRET", + }, + }, + }, + }, + registerHttpRoute, + }), + ); + + expect(result).toBeUndefined(); + expect(registerHttpRoute).toHaveBeenCalledTimes(1); + expect(registerHttpRoute).toHaveBeenCalledWith( + expect.objectContaining({ + path: "/plugins/webhooks/zapier", + auth: "plugin", + match: "exact", + replaceExisting: true, + }), + ); + }); +}); diff --git a/extensions/webhooks/index.ts b/extensions/webhooks/index.ts index 9d838dc219b..abcc6c43112 100644 --- a/extensions/webhooks/index.ts +++ b/extensions/webhooks/index.ts @@ -2,50 +2,52 @@ import { definePluginEntry, type OpenClawPluginApi } from "./api.js"; import { resolveWebhooksPluginConfig } from "./src/config.js"; import { createTaskFlowWebhookRequestHandler, type TaskFlowWebhookTarget } from "./src/http.js"; +function registerWebhookRoutes(api: OpenClawPluginApi): void { + const routes = resolveWebhooksPluginConfig({ + pluginConfig: api.pluginConfig, + }); + if (routes.length === 0) { + return; + } + + const targetsByPath = new Map(); + const handler = createTaskFlowWebhookRequestHandler({ + cfg: api.config, + targetsByPath, + }); + + for (const route of routes) { + const taskFlow = api.runtime.taskFlow.bindSession({ + sessionKey: route.sessionKey, + }); + const target: TaskFlowWebhookTarget = { + routeId: route.routeId, + path: route.path, + secretInput: route.secret, + secretConfigPath: `plugins.entries.webhooks.routes.${route.routeId}.secret`, + defaultControllerId: route.controllerId, + taskFlow, + }; + targetsByPath.set(target.path, [...(targetsByPath.get(target.path) ?? []), target]); + api.registerHttpRoute({ + path: target.path, + auth: "plugin", + match: "exact", + replaceExisting: true, + handler, + }); + api.logger.info?.( + `[webhooks] registered route ${route.routeId} on ${route.path} for session ${route.sessionKey}`, + ); + } +} + export default definePluginEntry({ id: "webhooks", name: "Webhooks", description: "Authenticated inbound webhooks that bind external automation to OpenClaw TaskFlows.", - async register(api: OpenClawPluginApi) { - const routes = await resolveWebhooksPluginConfig({ - pluginConfig: api.pluginConfig, - cfg: api.config, - env: process.env, - logger: api.logger, - }); - if (routes.length === 0) { - return; - } - - const targetsByPath = new Map(); - const handler = createTaskFlowWebhookRequestHandler({ - cfg: api.config, - targetsByPath, - }); - - for (const route of routes) { - const taskFlow = api.runtime.taskFlow.bindSession({ - sessionKey: route.sessionKey, - }); - const target: TaskFlowWebhookTarget = { - routeId: route.routeId, - path: route.path, - secret: route.secret, - defaultControllerId: route.controllerId, - taskFlow, - }; - targetsByPath.set(target.path, [...(targetsByPath.get(target.path) ?? []), target]); - api.registerHttpRoute({ - path: target.path, - auth: "plugin", - match: "exact", - replaceExisting: true, - handler, - }); - api.logger.info?.( - `[webhooks] registered route ${route.routeId} on ${route.path} for session ${route.sessionKey}`, - ); - } + register(api: OpenClawPluginApi) { + registerWebhookRoutes(api); }, }); diff --git a/extensions/webhooks/runtime-api.ts b/extensions/webhooks/runtime-api.ts index 3eec785cb32..37713b698d8 100644 --- a/extensions/webhooks/runtime-api.ts +++ b/extensions/webhooks/runtime-api.ts @@ -4,6 +4,7 @@ export { normalizeWebhookPath, readJsonWebhookBodyOrReject, resolveRequestClientIp, + resolveWebhookTargetWithAuthOrReject, resolveWebhookTargetWithAuthOrRejectSync, withResolvedWebhookRequestPipeline, WEBHOOK_IN_FLIGHT_DEFAULTS, diff --git a/extensions/webhooks/src/config.test.ts b/extensions/webhooks/src/config.test.ts index 946583208db..229eb5fcbe2 100644 --- a/extensions/webhooks/src/config.test.ts +++ b/extensions/webhooks/src/config.test.ts @@ -1,10 +1,9 @@ -import { describe, expect, it, vi } from "vitest"; -import type { OpenClawConfig } from "../runtime-api.js"; +import { describe, expect, it } from "vitest"; import { resolveWebhooksPluginConfig } from "./config.js"; describe("resolveWebhooksPluginConfig", () => { - it("resolves default paths and SecretRef-backed secrets", async () => { - const routes = await resolveWebhooksPluginConfig({ + it("keeps SecretRef-backed secrets on the route config", () => { + const routes = resolveWebhooksPluginConfig({ pluginConfig: { routes: { zapier: { @@ -17,10 +16,6 @@ describe("resolveWebhooksPluginConfig", () => { }, }, }, - cfg: {} as OpenClawConfig, - env: { - OPENCLAW_WEBHOOK_SECRET: "shared-secret", - }, }); expect(routes).toEqual([ @@ -28,16 +23,18 @@ describe("resolveWebhooksPluginConfig", () => { routeId: "zapier", path: "/plugins/webhooks/zapier", sessionKey: "agent:main:main", - secret: "shared-secret", + secret: { + source: "env", + provider: "default", + id: "OPENCLAW_WEBHOOK_SECRET", + }, controllerId: "webhooks/zapier", }, ]); }); - it("skips routes whose secret cannot be resolved", async () => { - const warn = vi.fn(); - - const routes = await resolveWebhooksPluginConfig({ + it("keeps routes whose secret needs runtime resolution", () => { + const routes = resolveWebhooksPluginConfig({ pluginConfig: { routes: { missing: { @@ -50,19 +47,25 @@ describe("resolveWebhooksPluginConfig", () => { }, }, }, - cfg: {} as OpenClawConfig, - env: {}, - logger: { warn } as never, }); - expect(routes).toEqual([]); - expect(warn).toHaveBeenCalledWith( - expect.stringContaining("[webhooks] skipping route missing:"), - ); + expect(routes).toEqual([ + { + routeId: "missing", + path: "/plugins/webhooks/missing", + sessionKey: "agent:main:main", + secret: { + source: "env", + provider: "default", + id: "MISSING_SECRET", + }, + controllerId: "webhooks/missing", + }, + ]); }); - it("rejects duplicate normalized paths", async () => { - await expect( + it("rejects duplicate normalized paths", () => { + expect(() => resolveWebhooksPluginConfig({ pluginConfig: { routes: { @@ -78,9 +81,7 @@ describe("resolveWebhooksPluginConfig", () => { }, }, }, - cfg: {} as OpenClawConfig, - env: {}, }), - ).rejects.toThrow(/conflicts with routes\.first\.path/i); + ).toThrow(/conflicts with routes\.first\.path/i); }); }); diff --git a/extensions/webhooks/src/config.ts b/extensions/webhooks/src/config.ts index 0a6c084f34e..60171d407a9 100644 --- a/extensions/webhooks/src/config.ts +++ b/extensions/webhooks/src/config.ts @@ -1,10 +1,5 @@ import { z } from "zod"; -import type { PluginLogger } from "../api.js"; -import { - normalizeWebhookPath, - resolveConfiguredSecretInputString, - type OpenClawConfig, -} from "../runtime-api.js"; +import { normalizeWebhookPath } from "../runtime-api.js"; const secretRefSchema = z .object({ @@ -33,23 +28,22 @@ const webhooksPluginConfigSchema = z }) .strict(); -export type ResolvedWebhookRouteConfig = { +export type WebhookSecretInput = z.infer; + +export type ConfiguredWebhookRouteConfig = { routeId: string; path: string; sessionKey: string; - secret: string; + secret: WebhookSecretInput; controllerId: string; description?: string; }; -export async function resolveWebhooksPluginConfig(params: { +export function resolveWebhooksPluginConfig(params: { pluginConfig: unknown; - cfg: OpenClawConfig; - env: NodeJS.ProcessEnv; - logger?: PluginLogger; -}): Promise { +}): ConfiguredWebhookRouteConfig[] { const parsed = webhooksPluginConfigSchema.parse(params.pluginConfig ?? {}); - const resolvedRoutes: ResolvedWebhookRouteConfig[] = []; + const configuredRoutes: ConfiguredWebhookRouteConfig[] = []; const seenPaths = new Map(); for (const [routeId, route] of Object.entries(parsed.routes)) { @@ -64,32 +58,16 @@ export async function resolveWebhooksPluginConfig(params: { ); } - const secretResolution = await resolveConfiguredSecretInputString({ - config: params.cfg, - env: params.env, - value: route.secret, - path: `plugins.entries.webhooks.routes.${routeId}.secret`, - }); - const secret = secretResolution.value?.trim(); - if (!secret) { - params.logger?.warn?.( - `[webhooks] skipping route ${routeId}: ${ - secretResolution.unresolvedRefReason ?? "secret is empty or unresolved" - }`, - ); - continue; - } - seenPaths.set(path, routeId); - resolvedRoutes.push({ + configuredRoutes.push({ routeId, path, sessionKey: route.sessionKey, - secret, + secret: route.secret, controllerId: route.controllerId ?? `webhooks/${routeId}`, ...(route.description ? { description: route.description } : {}), }); } - return resolvedRoutes; + return configuredRoutes; } diff --git a/extensions/webhooks/src/http.test.ts b/extensions/webhooks/src/http.test.ts index 0b6e55f397c..c7e1c6d8f94 100644 --- a/extensions/webhooks/src/http.test.ts +++ b/extensions/webhooks/src/http.test.ts @@ -69,13 +69,16 @@ function createJsonRequest(params: { function createHandler(): { handler: ReturnType; target: TaskFlowWebhookTarget; + secret: string; } { const runtime = createRuntimeTaskFlow(); nextSessionId += 1; + const secret = "shared-secret"; const target: TaskFlowWebhookTarget = { routeId: "zapier", path: "/plugins/webhooks/zapier", - secret: "shared-secret", + secretInput: secret, + secretConfigPath: "plugins.entries.webhooks.routes.zapier.secret", defaultControllerId: "webhooks/zapier", taskFlow: runtime.bindSession({ sessionKey: `agent:main:webhook-test-${String(nextSessionId)}`, @@ -88,6 +91,7 @@ function createHandler(): { targetsByPath, }), target, + secret, }; } @@ -133,11 +137,11 @@ describe("createTaskFlowWebhookRequestHandler", () => { }); it("creates flows through the bound session and scrubs owner metadata from responses", async () => { - const { handler, target } = createHandler(); + const { handler, target, secret } = createHandler(); const res = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "create_flow", goal: "Review inbound queue", @@ -158,7 +162,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { }); it("runs child tasks and scrubs task ownership fields from responses", async () => { - const { handler, target } = createHandler(); + const { handler, target, secret } = createHandler(); const flow = target.taskFlow.createManaged({ controllerId: "webhooks/zapier", goal: "Triage inbox", @@ -166,7 +170,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { const res = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "run_task", flowId: flow.flowId, @@ -193,11 +197,11 @@ describe("createTaskFlowWebhookRequestHandler", () => { }); it("returns 404 for missing flow mutations", async () => { - const { handler, target } = createHandler(); + const { handler, target, secret } = createHandler(); const res = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "set_waiting", flowId: "flow-missing", @@ -219,7 +223,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { }); it("returns 409 for revision conflicts", async () => { - const { handler, target } = createHandler(); + const { handler, target, secret } = createHandler(); const flow = target.taskFlow.createManaged({ controllerId: "webhooks/zapier", goal: "Review inbox", @@ -227,7 +231,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { const res = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "set_waiting", flowId: flow.flowId, @@ -252,7 +256,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { }); it("rejects internal runtimes and running-only metadata from external callers", async () => { - const { handler, target } = createHandler(); + const { handler, target, secret } = createHandler(); const flow = target.taskFlow.createManaged({ controllerId: "webhooks/zapier", goal: "Review inbox", @@ -261,7 +265,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { const runtimeRes = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "run_task", flowId: flow.flowId, @@ -278,7 +282,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { const queuedMetadataRes = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "run_task", flowId: flow.flowId, @@ -297,7 +301,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { }); it("reuses the same task record when retried with the same runId", async () => { - const { handler, target } = createHandler(); + const { handler, target, secret } = createHandler(); const flow = target.taskFlow.createManaged({ controllerId: "webhooks/zapier", goal: "Triage inbox", @@ -306,7 +310,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { const first = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "run_task", flowId: flow.flowId, @@ -319,7 +323,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { const second = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "run_task", flowId: flow.flowId, @@ -339,7 +343,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { }); it("returns 409 when cancellation targets a terminal flow", async () => { - const { handler, target } = createHandler(); + const { handler, target, secret } = createHandler(); const flow = target.taskFlow.createManaged({ controllerId: "webhooks/zapier", goal: "Review inbox", @@ -353,7 +357,7 @@ describe("createTaskFlowWebhookRequestHandler", () => { const res = await dispatchJsonRequest({ handler, path: target.path, - secret: target.secret, + secret, body: { action: "cancel_flow", flowId: flow.flowId, diff --git a/extensions/webhooks/src/http.ts b/extensions/webhooks/src/http.ts index 2fae8fe1882..3448c698ceb 100644 --- a/extensions/webhooks/src/http.ts +++ b/extensions/webhooks/src/http.ts @@ -8,13 +8,15 @@ import { createWebhookInFlightLimiter, readJsonWebhookBodyOrReject, resolveRequestClientIp, - resolveWebhookTargetWithAuthOrRejectSync, + resolveConfiguredSecretInputString, + resolveWebhookTargetWithAuthOrReject, withResolvedWebhookRequestPipeline, WEBHOOK_IN_FLIGHT_DEFAULTS, WEBHOOK_RATE_LIMIT_DEFAULTS, type OpenClawConfig, type WebhookInFlightLimiter, } from "../runtime-api.js"; +import type { WebhookSecretInput } from "./config.js"; type BoundTaskFlowRuntime = ReturnType; @@ -174,7 +176,8 @@ type WebhookAction = z.infer; export type TaskFlowWebhookTarget = { routeId: string; path: string; - secret: string; + secretInput: WebhookSecretInput; + secretConfigPath: string; defaultControllerId: string; taskFlow: BoundTaskFlowRuntime; }; @@ -698,11 +701,23 @@ export function createTaskFlowWebhookRequestHandler(params: { inFlightLimiter, handle: async ({ targets }) => { const presentedSecret = extractSharedSecret(req); - const target = resolveWebhookTargetWithAuthOrRejectSync({ + const target = await resolveWebhookTargetWithAuthOrReject({ targets, res, - isMatch: (candidate) => - presentedSecret.length > 0 && timingSafeEquals(candidate.secret, presentedSecret), + isMatch: async (candidate) => { + if (presentedSecret.length === 0) { + return false; + } + const resolvedSecret = await resolveConfiguredSecretInputString({ + config: params.cfg, + env: process.env, + value: candidate.secretInput, + path: candidate.secretConfigPath, + }); + return Boolean( + resolvedSecret.value && timingSafeEquals(resolvedSecret.value, presentedSecret), + ); + }, }); if (!target) { return true; diff --git a/src/plugins/cli-registry-loader.ts b/src/plugins/cli-registry-loader.ts index 4c6deb0ca40..be61358298e 100644 --- a/src/plugins/cli-registry-loader.ts +++ b/src/plugins/cli-registry-loader.ts @@ -47,28 +47,6 @@ function resolvePluginCliLogger(logger?: PluginLogger): PluginLogger { return logger ?? createPluginCliLogger(); } -function hasIgnoredAsyncPluginRegistration(registry: PluginRegistry): boolean { - return (registry.diagnostics ?? []).some( - (entry) => - entry.message === "plugin register returned a promise; async registration is ignored", - ); -} - -function mergeCliRegistrars(params: { - runtimeRegistry: PluginRegistry; - metadataRegistry: PluginRegistry; -}): PluginRegistry["cliRegistrars"] { - const runtimeCommands = new Set( - params.runtimeRegistry.cliRegistrars.flatMap((entry) => entry.commands), - ); - return [ - ...params.runtimeRegistry.cliRegistrars, - ...params.metadataRegistry.cliRegistrars.filter( - (entry) => !entry.commands.some((command) => runtimeCommands.has(command)), - ), - ]; -} - function buildPluginCliLoaderParams( context: PluginCliLoadContext, params?: { primaryCommand?: string }, @@ -129,48 +107,17 @@ export async function loadPluginCliCommandRegistryWithContext(params: { context: PluginCliLoadContext; primaryCommand?: string; loaderOptions?: PluginCliLoaderOptions; - onMetadataFallbackError: (error: unknown) => void; }): Promise { - const runtimeRegistry = loadOpenClawPlugins( - buildPluginCliLoaderParams( - params.context, - { primaryCommand: params.primaryCommand }, - params.loaderOptions, - ), - ); - - if (!hasIgnoredAsyncPluginRegistration(runtimeRegistry)) { - return { - ...params.context, - registry: runtimeRegistry, - }; - } - - try { - const metadataRegistry = await loadOpenClawPluginCliRegistry( + return { + ...params.context, + registry: loadOpenClawPlugins( buildPluginCliLoaderParams( params.context, { primaryCommand: params.primaryCommand }, params.loaderOptions, ), - ); - return { - ...params.context, - registry: { - ...runtimeRegistry, - cliRegistrars: mergeCliRegistrars({ - runtimeRegistry, - metadataRegistry, - }), - }, - }; - } catch (error) { - params.onMetadataFallbackError(error); - return { - ...params.context, - registry: runtimeRegistry, - }; - } + ), + }; } function buildPluginCliCommandGroupEntries(params: { @@ -194,10 +141,6 @@ function buildPluginCliCommandGroupEntries(params: { })); } -function logPluginCliMetadataFallbackError(logger: PluginLogger, error: unknown) { - logger.warn(`plugin CLI metadata fallback failed: ${String(error)}`); -} - export async function loadPluginCliDescriptors( params: PluginCliPublicLoadParams, ): Promise { @@ -227,7 +170,6 @@ export async function loadPluginCliRegistrationEntries(params: { loaderOptions?: PluginCliLoaderOptions; logger?: PluginLogger; primaryCommand?: string; - onMetadataFallbackError: (error: unknown) => void; }): Promise { const resolvedLogger = resolvePluginCliLogger(params.logger); const context = resolvePluginCliLoadContext({ @@ -239,7 +181,6 @@ export async function loadPluginCliRegistrationEntries(params: { context, primaryCommand: params.primaryCommand, loaderOptions: params.loaderOptions, - onMetadataFallbackError: params.onMetadataFallbackError, }); return buildPluginCliCommandGroupEntries({ registry, @@ -256,8 +197,5 @@ export async function loadPluginCliRegistrationEntriesWithDefaults( return loadPluginCliRegistrationEntries({ ...params, logger, - onMetadataFallbackError: (error) => { - logPluginCliMetadataFallbackError(logger, error); - }, }); } diff --git a/src/plugins/cli.test.ts b/src/plugins/cli.test.ts index d99efcd7d90..c9a7d3372dd 100644 --- a/src/plugins/cli.test.ts +++ b/src/plugins/cli.test.ts @@ -81,13 +81,6 @@ function createCliRegistry(params?: { }; } -function createEmptyCliRegistry(params?: { diagnostics?: Array<{ message: string }> }) { - return { - cliRegistrars: [], - diagnostics: params?.diagnostics ?? [], - }; -} - function createAutoEnabledCliFixture() { const rawConfig = { plugins: {}, @@ -310,51 +303,6 @@ describe("registerPluginCliCommands", () => { expect(mocks.loadOpenClawPluginCliRegistry).not.toHaveBeenCalled(); }); - it("falls back to awaited CLI metadata collection when runtime loading ignored async registration", async () => { - const asyncRegistrar = vi.fn(async ({ program }: { program: Command }) => { - const asyncCommand = program.command("async-cli").description("Async CLI"); - asyncCommand.command("run").action(mocks.memoryListAction); - }); - mocks.loadOpenClawPlugins.mockReturnValue( - createEmptyCliRegistry({ - diagnostics: [ - { - message: "plugin register returned a promise; async registration is ignored", - }, - ], - }), - ); - mocks.loadOpenClawPluginCliRegistry.mockResolvedValue({ - cliRegistrars: [ - { - pluginId: "async-plugin", - register: asyncRegistrar, - commands: ["async-cli"], - descriptors: [ - { - name: "async-cli", - description: "Async CLI", - hasSubcommands: true, - }, - ], - source: "bundled", - }, - ], - diagnostics: [], - }); - const program = createProgram(); - program.exitOverride(); - - await registerPluginCliCommands(program, {} as OpenClawConfig, undefined, undefined, { - mode: "lazy", - }); - - expect(mocks.loadOpenClawPluginCliRegistry).toHaveBeenCalledTimes(1); - await program.parseAsync(["async-cli", "run"], { from: "user" }); - expect(asyncRegistrar).toHaveBeenCalledTimes(1); - expect(mocks.memoryListAction).toHaveBeenCalledTimes(1); - }); - it("lazy-registers descriptor-backed plugin commands on first invocation", async () => { const program = createProgram(); program.exitOverride(); diff --git a/src/plugins/loader.cli-metadata.test.ts b/src/plugins/loader.cli-metadata.test.ts index d0250afafae..c13a705a730 100644 --- a/src/plugins/loader.cli-metadata.test.ts +++ b/src/plugins/loader.cli-metadata.test.ts @@ -549,7 +549,7 @@ module.exports = { ); }); - it("awaits async plugin registration when collecting CLI metadata", async () => { + it("rejects async plugin registration when collecting CLI metadata", async () => { useNoBundledPlugins(); const plugin = writePlugin({ id: "async-cli", @@ -580,10 +580,11 @@ module.exports = { }, }); - expect(registry.cliRegistrars.flatMap((entry) => entry.commands)).toContain("async-cli"); - expect( - registry.diagnostics.some((entry) => entry.message.includes("async registration is ignored")), - ).toBe(false); + expect(registry.cliRegistrars.flatMap((entry) => entry.commands)).not.toContain("async-cli"); + const loaded = registry.plugins.find((entry) => entry.id === "async-cli"); + expect(loaded?.status).toBe("error"); + expect(loaded?.failurePhase).toBe("register"); + expect(loaded?.error).toContain("plugin register must be synchronous"); }); it("applies memory slot gating to non-bundled CLI metadata loads", async () => { diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index 2ff7080638a..88951002ffe 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -1159,6 +1159,39 @@ describe("loadOpenClawPlugins", () => { ).toBe(true); }, }, + { + label: "rejects async register functions instead of silently loading them", + run: () => { + useNoBundledPlugins(); + const plugin = writePlugin({ + id: "async-register", + filename: "async-register.cjs", + body: `module.exports = { + id: "async-register", + async register(api) { + await Promise.resolve(); + api.registerGatewayMethod("async-register.ping", ({ respond }) => respond(true, { ok: true })); + }, +};`, + }); + + const registry = loadOpenClawPlugins({ + cache: false, + config: { + plugins: { + load: { paths: [plugin.file] }, + allow: ["async-register"], + }, + }, + }); + + const loaded = registry.plugins.find((entry) => entry.id === "async-register"); + expect(loaded?.status).toBe("error"); + expect(loaded?.failurePhase).toBe("register"); + expect(loaded?.error).toContain("plugin register must be synchronous"); + expect(Object.keys(registry.gatewayHandlers)).not.toContain("async-register.ping"); + }, + }, { label: "limits imports to the requested plugin ids", run: () => { diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index 85563ed8798..5f668a280cd 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -98,7 +98,12 @@ import { shouldPreferNativeJiti, } from "./sdk-alias.js"; import { hasKind, kindsEqual } from "./slots.js"; -import type { OpenClawPluginDefinition, OpenClawPluginModule, PluginLogger } from "./types.js"; +import type { + OpenClawPluginApi, + OpenClawPluginDefinition, + OpenClawPluginModule, + PluginLogger, +} from "./types.js"; export type PluginLoadResult = PluginRegistry; @@ -250,6 +255,162 @@ function profilePluginLoaderSync(params: { } } +function isPromiseLike(value: unknown): value is PromiseLike { + return ( + (typeof value === "object" || typeof value === "function") && + value !== null && + typeof (value as { then?: unknown }).then === "function" + ); +} + +type PluginRegistrySnapshot = { + arrays: { + tools: PluginRegistry["tools"]; + hooks: PluginRegistry["hooks"]; + typedHooks: PluginRegistry["typedHooks"]; + channels: PluginRegistry["channels"]; + channelSetups: PluginRegistry["channelSetups"]; + providers: PluginRegistry["providers"]; + cliBackends: NonNullable; + textTransforms: PluginRegistry["textTransforms"]; + speechProviders: PluginRegistry["speechProviders"]; + realtimeTranscriptionProviders: PluginRegistry["realtimeTranscriptionProviders"]; + realtimeVoiceProviders: PluginRegistry["realtimeVoiceProviders"]; + mediaUnderstandingProviders: PluginRegistry["mediaUnderstandingProviders"]; + imageGenerationProviders: PluginRegistry["imageGenerationProviders"]; + videoGenerationProviders: PluginRegistry["videoGenerationProviders"]; + musicGenerationProviders: PluginRegistry["musicGenerationProviders"]; + webFetchProviders: PluginRegistry["webFetchProviders"]; + webSearchProviders: PluginRegistry["webSearchProviders"]; + memoryEmbeddingProviders: PluginRegistry["memoryEmbeddingProviders"]; + agentHarnesses: PluginRegistry["agentHarnesses"]; + httpRoutes: PluginRegistry["httpRoutes"]; + cliRegistrars: PluginRegistry["cliRegistrars"]; + reloads: NonNullable; + nodeHostCommands: NonNullable; + securityAuditCollectors: NonNullable; + services: PluginRegistry["services"]; + commands: PluginRegistry["commands"]; + conversationBindingResolvedHandlers: PluginRegistry["conversationBindingResolvedHandlers"]; + diagnostics: PluginRegistry["diagnostics"]; + }; + gatewayHandlers: PluginRegistry["gatewayHandlers"]; + gatewayMethodScopes: NonNullable; +}; + +function snapshotPluginRegistry(registry: PluginRegistry): PluginRegistrySnapshot { + return { + arrays: { + tools: [...registry.tools], + hooks: [...registry.hooks], + typedHooks: [...registry.typedHooks], + channels: [...registry.channels], + channelSetups: [...registry.channelSetups], + providers: [...registry.providers], + cliBackends: [...(registry.cliBackends ?? [])], + textTransforms: [...registry.textTransforms], + speechProviders: [...registry.speechProviders], + realtimeTranscriptionProviders: [...registry.realtimeTranscriptionProviders], + realtimeVoiceProviders: [...registry.realtimeVoiceProviders], + mediaUnderstandingProviders: [...registry.mediaUnderstandingProviders], + imageGenerationProviders: [...registry.imageGenerationProviders], + videoGenerationProviders: [...registry.videoGenerationProviders], + musicGenerationProviders: [...registry.musicGenerationProviders], + webFetchProviders: [...registry.webFetchProviders], + webSearchProviders: [...registry.webSearchProviders], + memoryEmbeddingProviders: [...registry.memoryEmbeddingProviders], + agentHarnesses: [...registry.agentHarnesses], + httpRoutes: [...registry.httpRoutes], + cliRegistrars: [...registry.cliRegistrars], + reloads: [...(registry.reloads ?? [])], + nodeHostCommands: [...(registry.nodeHostCommands ?? [])], + securityAuditCollectors: [...(registry.securityAuditCollectors ?? [])], + services: [...registry.services], + commands: [...registry.commands], + conversationBindingResolvedHandlers: [...registry.conversationBindingResolvedHandlers], + diagnostics: [...registry.diagnostics], + }, + gatewayHandlers: { ...registry.gatewayHandlers }, + gatewayMethodScopes: { ...registry.gatewayMethodScopes }, + }; +} + +function restorePluginRegistry(registry: PluginRegistry, snapshot: PluginRegistrySnapshot): void { + registry.tools = snapshot.arrays.tools; + registry.hooks = snapshot.arrays.hooks; + registry.typedHooks = snapshot.arrays.typedHooks; + registry.channels = snapshot.arrays.channels; + registry.channelSetups = snapshot.arrays.channelSetups; + registry.providers = snapshot.arrays.providers; + registry.cliBackends = snapshot.arrays.cliBackends; + registry.textTransforms = snapshot.arrays.textTransforms; + registry.speechProviders = snapshot.arrays.speechProviders; + registry.realtimeTranscriptionProviders = snapshot.arrays.realtimeTranscriptionProviders; + registry.realtimeVoiceProviders = snapshot.arrays.realtimeVoiceProviders; + registry.mediaUnderstandingProviders = snapshot.arrays.mediaUnderstandingProviders; + registry.imageGenerationProviders = snapshot.arrays.imageGenerationProviders; + registry.videoGenerationProviders = snapshot.arrays.videoGenerationProviders; + registry.musicGenerationProviders = snapshot.arrays.musicGenerationProviders; + registry.webFetchProviders = snapshot.arrays.webFetchProviders; + registry.webSearchProviders = snapshot.arrays.webSearchProviders; + registry.memoryEmbeddingProviders = snapshot.arrays.memoryEmbeddingProviders; + registry.agentHarnesses = snapshot.arrays.agentHarnesses; + registry.httpRoutes = snapshot.arrays.httpRoutes; + registry.cliRegistrars = snapshot.arrays.cliRegistrars; + registry.reloads = snapshot.arrays.reloads; + registry.nodeHostCommands = snapshot.arrays.nodeHostCommands; + registry.securityAuditCollectors = snapshot.arrays.securityAuditCollectors; + registry.services = snapshot.arrays.services; + registry.commands = snapshot.arrays.commands; + registry.conversationBindingResolvedHandlers = + snapshot.arrays.conversationBindingResolvedHandlers; + registry.diagnostics = snapshot.arrays.diagnostics; + registry.gatewayHandlers = snapshot.gatewayHandlers; + registry.gatewayMethodScopes = snapshot.gatewayMethodScopes; +} + +function createGuardedPluginRegistrationApi(api: OpenClawPluginApi): { + api: OpenClawPluginApi; + close: () => void; +} { + let closed = false; + return { + api: new Proxy(api, { + get(target, prop, receiver) { + const value = Reflect.get(target, prop, receiver); + if (typeof value !== "function") { + return value; + } + return (...args: unknown[]) => { + if (closed) { + return undefined; + } + return Reflect.apply(value, target, args); + }; + }, + }), + close: () => { + closed = true; + }, + }; +} + +function runPluginRegisterSync( + register: NonNullable, + api: Parameters>[0], +): void { + const guarded = createGuardedPluginRegistrationApi(api); + try { + const result = register(guarded.api); + if (isPromiseLike(result)) { + void Promise.resolve(result).catch(() => {}); + throw new Error("plugin register must be synchronous"); + } + } finally { + guarded.close(); + } +} + /** * On Windows, the Node.js ESM loader requires absolute paths to be expressed * as file:// URLs (e.g. file:///C:/Users/...). Raw drive-letter paths like @@ -2055,17 +2216,10 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi const previousMemoryCorpusSupplements = listMemoryCorpusSupplements(); const previousMemoryPromptSupplements = listMemoryPromptSupplements(); const previousMemoryRuntime = getMemoryRuntime(); + const registrySnapshot = snapshotPluginRegistry(registry); try { - const result = register(api); - if (result && typeof result.then === "function") { - registry.diagnostics.push({ - level: "warn", - pluginId: record.id, - source: record.source, - message: "plugin register returned a promise; async registration is ignored", - }); - } + runPluginRegisterSync(register, api); // Snapshot loads should not replace process-global runtime prompt state. if (!shouldActivate) { restoreRegisteredAgentHarnesses(previousAgentHarnesses); @@ -2082,6 +2236,7 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi registry.plugins.push(record); seenIds.set(pluginId, candidate.origin); } catch (err) { + restorePluginRegistry(registry, registrySnapshot); restoreRegisteredAgentHarnesses(previousAgentHarnesses); restoreRegisteredCompactionProviders(previousCompactionProviders); restoreRegisteredMemoryEmbeddingProviders(previousMemoryEmbeddingProviders); @@ -2477,11 +2632,13 @@ export async function loadOpenClawPluginCliRegistry( }, }); + const registrySnapshot = snapshotPluginRegistry(registry); try { - await register(api); + runPluginRegisterSync(register, api); registry.plugins.push(record); seenIds.set(pluginId, candidate.origin); } catch (err) { + restorePluginRegistry(registry, registrySnapshot); recordPluginError({ logger, registry, diff --git a/src/plugins/types.ts b/src/plugins/types.ts index 6f1413b3f1b..b47aff0aa8d 100644 --- a/src/plugins/types.ts +++ b/src/plugins/types.ts @@ -1838,13 +1838,11 @@ export type OpenClawPluginDefinition = { reload?: OpenClawPluginReloadRegistration; nodeHostCommands?: OpenClawPluginNodeHostCommand[]; securityAuditCollectors?: OpenClawPluginSecurityAuditCollector[]; - register?: (api: OpenClawPluginApi) => void | Promise; - activate?: (api: OpenClawPluginApi) => void | Promise; + register?: (api: OpenClawPluginApi) => void; + activate?: (api: OpenClawPluginApi) => void; }; -export type OpenClawPluginModule = - | OpenClawPluginDefinition - | ((api: OpenClawPluginApi) => void | Promise); +export type OpenClawPluginModule = OpenClawPluginDefinition | ((api: OpenClawPluginApi) => void); export type PluginRegistrationMode = "full" | "setup-only" | "setup-runtime" | "cli-metadata"; diff --git a/src/test-utils/plugin-registration.ts b/src/test-utils/plugin-registration.ts index 8742c28402e..7f9f62ebbe8 100644 --- a/src/test-utils/plugin-registration.ts +++ b/src/test-utils/plugin-registration.ts @@ -4,14 +4,14 @@ import type { OpenClawPluginApi, ProviderPlugin } from "../plugins/types.js"; export { createCapturedPluginRegistration }; type RegistrablePlugin = { - register(api: OpenClawPluginApi): void | Promise; + register(api: OpenClawPluginApi): void; }; export async function registerSingleProviderPlugin(params: { - register(api: OpenClawPluginApi): void | Promise; + register(api: OpenClawPluginApi): void; }): Promise { const captured = createCapturedPluginRegistration(); - await params.register(captured.api); + params.register(captured.api); const provider = captured.providers[0]; if (!provider) { throw new Error("provider registration missing"); @@ -24,7 +24,7 @@ export async function registerProviderPlugins( ): Promise { const captured = createCapturedPluginRegistration(); for (const plugin of plugins) { - await plugin.register(captured.api); + plugin.register(captured.api); } return captured.providers; } diff --git a/test/helpers/media-generation/bundled-provider-builders.ts b/test/helpers/media-generation/bundled-provider-builders.ts index bd4bcb14fc8..f893870db22 100644 --- a/test/helpers/media-generation/bundled-provider-builders.ts +++ b/test/helpers/media-generation/bundled-provider-builders.ts @@ -3,7 +3,7 @@ import { loadBundledPluginPublicSurfaceSync } from "../../../src/test-utils/bund type BundledPluginEntryModule = { default: { - register(api: OpenClawPluginApi): void | Promise; + register(api: OpenClawPluginApi): void; }; }; diff --git a/test/helpers/plugins/provider-registration.ts b/test/helpers/plugins/provider-registration.ts index cd5d77c657f..cc646312842 100644 --- a/test/helpers/plugins/provider-registration.ts +++ b/test/helpers/plugins/provider-registration.ts @@ -18,7 +18,7 @@ type RegisteredProviderCollections = { }; type ProviderPluginModule = { - register(api: ReturnType): void | Promise; + register(api: ReturnType): void; }; export async function registerProviderPlugin(params: { @@ -33,7 +33,7 @@ export async function registerProviderPlugin(params: { const musicProviders: MusicGenerationProviderPlugin[] = []; const videoProviders: VideoGenerationProviderPlugin[] = []; - await params.plugin.register( + params.plugin.register( createTestPluginApi({ id: params.id, name: params.name,