From 61986571bb7e048dd5fb55a9f837ff03042e33c8 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 13 Jul 2026 21:50:46 -0700 Subject: [PATCH] fix(plugins): forward-port bundled SDK fallback hardening (#107131) * fix(plugins): avoid bundled SDK fallback races * fix(ci): satisfy plugin SDK LOC ratchet * fix(ci): align fallback forward-port guards --------- Co-authored-by: Vincent Koc --- AGENTS.md | 1 + src/plugin-sdk/channel-entry-contract.test.ts | 38 +++++++++++++++++++ src/plugin-sdk/channel-entry-contract.ts | 26 ++++++------- .../plugin-module-loader-cache.test.ts | 30 +++++++++++++++ src/plugins/plugin-module-loader-cache.ts | 10 ++++- 5 files changed, 90 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8cb4180e4921..15d7bb1222c4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -174,6 +174,7 @@ Skills own workflows; root owns hard policy and routing. - zsh: quote `gh api` endpoints containing `?` or brackets; otherwise glob expansion corrupts the invocation. - Blacksmith Testbox status/stop: `--id `; no status JSON flag. - Crabbox final timing JSON = proof complete; if portal sync hangs after it, interrupt wrapper only. +- Sparse-sync temp checkout may claim kept Testbox; repo-path reuse needs `--reclaim`. - GitHub Actions: resolve workflow files from `.github/workflows` or API; never infer filenames from display names. - zsh: quote command globs; unmatched patterns abort before the tool runs. - zsh: don't use `path` as a variable; it rewrites `$PATH`. diff --git a/src/plugin-sdk/channel-entry-contract.test.ts b/src/plugin-sdk/channel-entry-contract.test.ts index d634d07c1e61..781eef444e44 100644 --- a/src/plugin-sdk/channel-entry-contract.test.ts +++ b/src/plugin-sdk/channel-entry-contract.test.ts @@ -27,6 +27,7 @@ afterEach(() => { } vi.resetModules(); vi.doUnmock("jiti"); + vi.doUnmock("../plugins/native-module-require.js"); vi.unstubAllEnvs(); delete ( globalThis as typeof globalThis & { @@ -464,6 +465,43 @@ describe("loadBundledEntryExportSync", () => { }); }); + it("transforms OpenClaw SDK dependencies after a native built sidecar load declines", async () => { + const sourceLoad = vi.fn(() => ({ sentinel: 42 })); + const createJiti = vi.fn((_filename: string, _options?: Record) => sourceLoad); + vi.doMock("../plugins/native-module-require.js", () => ({ + tryNativeRequireJavaScriptModule: vi.fn(() => ({ ok: false })), + })); + + const channelEntryContract = await importFreshModule< + typeof import("./channel-entry-contract.js") + >(import.meta.url, "./channel-entry-contract.js?scope=native-esm-race-fallback"); + const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-channel-entry-contract-")); + tempDirs.push(tempRoot); + const pluginRoot = path.join(tempRoot, "dist", "extensions", "whatsapp"); + fs.mkdirSync(pluginRoot, { recursive: true }); + const importerPath = path.join(pluginRoot, "setup-entry.js"); + const sidecarPath = path.join(pluginRoot, "setup-plugin-api.js"); + fs.writeFileSync(importerPath, "export default {};\n", "utf8"); + fs.writeFileSync(sidecarPath, "export const sentinel = 42;\n", "utf8"); + + expect( + channelEntryContract.loadBundledEntryExportSync( + pathToFileURL(importerPath).href, + { + specifier: "./setup-plugin-api.js", + exportName: "sentinel", + }, + { createLoaderForTest: createJiti as never }, + ), + ).toBe(42); + const jitiOptions = createJiti.mock.calls[0]?.[1] as + | { nativeModules?: string[]; tryNative?: boolean } + | undefined; + expect(jitiOptions?.tryNative).toBe(false); + expect(jitiOptions?.nativeModules).toEqual([]); + expect(sourceLoad).toHaveBeenCalledWith(sidecarPath); + }); + it("loads packaged telegram setup sidecars from dist-facing api modules", () => { const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-channel-entry-contract-")); tempDirs.push(tempRoot); diff --git a/src/plugin-sdk/channel-entry-contract.ts b/src/plugin-sdk/channel-entry-contract.ts index ac7c42da166b..8fd57b87b07a 100644 --- a/src/plugin-sdk/channel-entry-contract.ts +++ b/src/plugin-sdk/channel-entry-contract.ts @@ -1,4 +1,3 @@ -// Channel entry contracts validate plugin channel entrypoints and runtime API facades. import fs from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -362,13 +361,18 @@ function resolveBundledEntryModulePath(importMetaUrl: string, specifier: string) ); } -function getSourceModuleLoader(modulePath: string, options: BundledEntryModuleLoadOptions) { +function getSourceModuleLoader( + modulePath: string, + options: BundledEntryModuleLoadOptions, + transformOpenClawDependencies = false, +) { return getCachedPluginSourceModuleLoader({ cache: moduleLoaders, modulePath, importerUrl: import.meta.url, preferBuiltDist: true, loaderFilename: import.meta.url, + transformOpenClawDependencies, ...(options.createLoaderForTest ? { createLoader: options.createLoaderForTest } : {}), }); } @@ -407,7 +411,9 @@ function loadBundledEntryModuleSync( if (native.ok) { loaded = native.moduleExport; } else { - const moduleLoader = getSourceModuleLoader(modulePath, options); + // Native require can leave an SDK module inside an active dynamic-import graph. + // Transform the fallback graph end-to-end so it cannot require that module again. + const moduleLoader = getSourceModuleLoader(modulePath, options, true); sourceLoaderReadyMs = profile ? performance.now() : 0; loaded = moduleLoader(toSafeImportPath(modulePath)); } @@ -418,19 +424,14 @@ function loadBundledEntryModuleSync( } if (profile) { const endMs = performance.now(); - // Use shared formatter — but split timing fields ourselves so we can - // attribute time spent in source-loader creation vs the actual graph load. - // Both are emitted as extras - // alongside the canonical `elapsedMs=` field. + // Split source-loader creation from graph loading while preserving canonical elapsedMs. console.error( formatPluginLoadProfileLine({ phase: "bundled-entry-module-load", pluginId: "(bundled-entry)", source: modulePath, elapsedMs: endMs - loadStartMs, - // When the built-artifact fast path resolves natively, the - // source-loader timestamp stays `0`; keep its breakdown at zero so - // `elapsedMs=` owns the native load time. + // Native loads leave the source timestamp at zero, so elapsedMs owns the full load. extras: [ ["sourceLoaderCreateMs", sourceLoaderReadyMs ? sourceLoaderReadyMs - loadStartMs : 0], ["sourceLoaderCallMs", sourceLoaderReadyMs ? endMs - sourceLoaderReadyMs : 0], @@ -576,9 +577,8 @@ export function defineBundledChannelSetupEntry({ registerSetupRuntime, features, }: DefineBundledChannelSetupEntryOptions): BundledChannelSetupEntryContract { - // Bundled setup entries stay on a light path during setup-only/setup-runtime loads. - // When runtime wiring is needed, expose only the setter so the loader can hand - // the setup surface the active runtime without importing the full channel entry. + // Setup loads stay light; expose only the setter needed to inject the active runtime + // without importing the full channel entry. const setChannelRuntime = runtime ? (pluginRuntime: BundledChannelRuntime) => { const setter = loadBundledEntryExportSync<(runtime: BundledChannelRuntime) => void>( diff --git a/src/plugins/plugin-module-loader-cache.test.ts b/src/plugins/plugin-module-loader-cache.test.ts index c9de1bfdfba7..00b4dea795da 100644 --- a/src/plugins/plugin-module-loader-cache.test.ts +++ b/src/plugins/plugin-module-loader-cache.test.ts @@ -598,6 +598,36 @@ describe("getCachedPluginModuleLoader", () => { ]); }); + it("can transform OpenClaw dependencies on a forced source fallback", async () => { + const fromSourceTransformer = vi.fn(() => ({ fromSourceTransform: true })); + const createJiti = vi.fn(() => fromSourceTransformer); + const nativeStub = vi.fn(() => ({ ok: true, moduleExport: { fromNative: true } })); + vi.doMock("./native-module-require.js", () => ({ + isJavaScriptModulePath: () => true, + tryNativeRequireJavaScriptModule: nativeStub, + })); + const { getCachedPluginSourceModuleLoader } = await importFreshModule< + typeof import("./plugin-module-loader-cache.js") + >(import.meta.url, "./plugin-module-loader-cache.js?scope=forced-source-native-fallback"); + + const loader = getCachedPluginSourceModuleLoader({ + cache: new Map(), + modulePath: "/repo/dist/extensions/demo/api.js", + importerUrl: "file:///repo/src/plugin-sdk/channel-entry-contract.ts", + loaderFilename: "file:///repo/src/plugin-sdk/channel-entry-contract.ts", + transformOpenClawDependencies: true, + createLoader: asPluginModuleLoaderFactory(createJiti), + }); + + expect(loader("/repo/dist/extensions/demo/api.js")).toEqual({ + fromSourceTransform: true, + }); + const options = requireRecord(callArg(createJiti, 0, 1, "jiti options"), "jiti options"); + expect(options.tryNative).toBe(false); + expect(options.nativeModules).toEqual([]); + expect(nativeStub).not.toHaveBeenCalled(); + }); + it("normalizes Windows absolute paths before creating and calling the source transformer", async () => { vi.spyOn(process, "platform", "get").mockReturnValue("win32"); const fromSourceTransformer = vi.fn(() => ({ fromSourceTransform: true })); diff --git a/src/plugins/plugin-module-loader-cache.ts b/src/plugins/plugin-module-loader-cache.ts index 5a983a369920..35fe214e9142 100644 --- a/src/plugins/plugin-module-loader-cache.ts +++ b/src/plugins/plugin-module-loader-cache.ts @@ -33,11 +33,13 @@ type ResolvePluginModuleLoaderCacheEntryParams = { pluginSdkResolution?: PluginSdkResolutionPreference; cacheScopeKey?: string; sharedCacheScopeKey?: string; + transformOpenClawDependencies?: boolean; }; type PluginModuleLoaderCacheEntry = { loaderFilename: string; aliasMap: Record; tryNative: boolean; + transformOpenClawDependencies: boolean; cacheKey: string; scopedCacheKey: string; }; @@ -157,12 +159,14 @@ function resolvePluginModuleLoaderCacheEntry( } : resolveDefaultPluginModuleLoaderConfig(params); const { tryNative, aliasMap } = resolved; - const cacheKey = + const moduleConfigCacheKey = resolved.cacheKey ?? createPluginLoaderModuleCacheKey({ tryNative, aliasMap, }); + const transformOpenClawDependencies = params.transformOpenClawDependencies ?? tryNative; + const cacheKey = `${moduleConfigCacheKey}\0transform-openclaw=${transformOpenClawDependencies ? "1" : "0"}`; const scopedCacheKey = `${loaderFilename}::${ params.sharedCacheScopeKey ?? (params.cacheScopeKey ? `${params.cacheScopeKey}::${cacheKey}` : cacheKey) @@ -171,6 +175,7 @@ function resolvePluginModuleLoaderCacheEntry( loaderFilename, aliasMap, tryNative, + transformOpenClawDependencies, cacheKey, scopedCacheKey, }; @@ -220,13 +225,13 @@ function createPluginModuleLoader(params: { loaderFilename: string; aliasMap: Record; tryNative: boolean; + transformOpenClawDependencies: boolean; createLoader?: PluginModuleLoaderFactory; }): PluginModuleLoader { // A declined native require can leave an ESM dependency in flight. The // fallback must transform both the entry and OpenClaw SDK dependencies. const getLoadWithSourceTransform = createLazySourceTransformLoader({ ...params, - transformOpenClawDependencies: params.tryNative, }); const loadedTargetExports = new Map(); const loadCachedTarget = (target: string, rest: unknown[], load: () => unknown): unknown => { @@ -304,6 +309,7 @@ export function getCachedPluginModuleLoader( loaderFilename: cacheEntry.loaderFilename, aliasMap: cacheEntry.aliasMap, tryNative: cacheEntry.tryNative, + transformOpenClawDependencies: cacheEntry.transformOpenClawDependencies, ...(params.createLoader ? { createLoader: params.createLoader } : {}), }); params.cache.set(cacheEntry.scopedCacheKey, loader);