From 2c5f024145d64bca7799600ac034addde8a2f201 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 12 Aug 2026 12:39:09 -0700 Subject: [PATCH] refactor(tooling): simplify plugin boundary checks (#122781) --- package.json | 1 - ...check-plugin-extension-import-boundary.mts | 208 +++++-------- .../check-web-search-provider-boundaries.mts | 282 ------------------ scripts/run-additional-boundary-checks.mts | 1 - ...n-extension-import-boundary-inventory.json | 1 - test/plugin-extension-import-boundary.test.ts | 38 ++- test/web-provider-boundary.test.ts | 10 - 7 files changed, 98 insertions(+), 443 deletions(-) delete mode 100644 scripts/check-web-search-provider-boundaries.mts delete mode 100644 test/fixtures/plugin-extension-import-boundary-inventory.json diff --git a/package.json b/package.json index 2c1a4c72f35f..cf7f61094445 100644 --- a/package.json +++ b/package.json @@ -1695,7 +1695,6 @@ "lint:ui:no-raw-window-open": "node scripts/run-oxlint.mjs --openclaw-focused-config --config config/oxlint/boundary-guards.json ui/src", "lint:ui:styles": "stylelint --config config/stylelint.config.mjs \"ui/src/**/*.css\" \"ui/src/**/*.ts\"", "lint:web-fetch-provider-boundaries": "node --import tsx scripts/check-web-fetch-provider-boundaries.mts", - "lint:web-search-provider-boundaries": "node --import tsx scripts/check-web-search-provider-boundaries.mts", "lint:webhook:no-low-level-body-read": "node --import tsx scripts/check-webhook-auth-body-order.mts", "mac:open": "open dist/OpenClaw.app", "mac:package": "bash scripts/package-mac-app.sh", diff --git a/scripts/check-plugin-extension-import-boundary.mts b/scripts/check-plugin-extension-import-boundary.mts index 277e11c6ae1a..6e6811267468 100644 --- a/scripts/check-plugin-extension-import-boundary.mts +++ b/scripts/check-plugin-extension-import-boundary.mts @@ -1,43 +1,24 @@ #!/usr/bin/env node // Inventories core plugin imports that cross into bundled extension files. -import { promises as fs } from "node:fs"; +import { existsSync } from "node:fs"; import path from "node:path"; import { createExtensionImportBoundaryChecker } from "./lib/extension-import-boundary-checker.mts"; import { - createCachedAsync, - diffInventoryEntries, formatGroupedInventoryHuman, - runBaselineInventoryCheck, resolveRepoSpecifier, + writeLine, } from "./lib/guard-inventory-utils.mjs"; import { resolveRepoRoot } from "./lib/repo-root.mjs"; import { runAsScript } from "./lib/ts-guard-utils.mts"; const repoRoot = resolveRepoRoot(import.meta.url); -const baselinePath = path.join( - repoRoot, - "test", - "fixtures", - "plugin-extension-import-boundary-inventory.json", -); - -const bundledWebSearchProviders = new Set([ - "brave", - "firecrawl", - "gemini", - "grok", - "kimi", - "perplexity", -]); -const bundledWebSearchPluginIds = new Set([ - "brave", - "firecrawl", - "google", - "moonshot", - "perplexity", - "xai", -]); +const AUTHORED_MODULE_EXTENSIONS = [".ts", ".tsx", ".mts", ".cts", ".js", ".jsx", ".mjs", ".cjs"]; +const RETIRED_WEB_SEARCH_CORE_MODULES = [ + "src/agents/tools/web-search-plugin-factory", + "src/plugins/bundled-web-search-registry", + "src/plugins/web-search-providers", +] as const; type PluginExtensionInventoryEntry = { file: string; @@ -47,6 +28,10 @@ type PluginExtensionInventoryEntry = { resolvedPath: string | null; reason: string; }; +type ScriptIo = { + stdout: { write(chunk: string): unknown }; + stderr: { write(chunk: string): unknown }; +}; function compareEntries(left: PluginExtensionInventoryEntry, right: PluginExtensionInventoryEntry) { return ( @@ -74,145 +59,98 @@ function classifyResolvedExtensionReason(kind: string, resolvedPath: string | nu return `${verb} extension-owned file from src/plugins`; } -function scanWebSearchRegistrySmells( - source: string, - relativeFile: string, -): PluginExtensionInventoryEntry[] { - if (relativeFile !== "src/plugins/web-search-providers.ts") { - return []; - } - - const entries: PluginExtensionInventoryEntry[] = []; - const lines = source.split(/\r?\n/); - for (const [index, line] of lines.entries()) { - const lineNumber = index + 1; - - if (line.includes("web-search-plugin-factory.js")) { - entries.push({ - file: relativeFile, - line: lineNumber, - kind: "registry-smell", - specifier: "../agents/tools/web-search-plugin-factory.js", - resolvedPath: "src/agents/tools/web-search-plugin-factory.js", - reason: "imports core-owned web search provider factory into plugin registry", - }); - } - - const pluginMatch = line.match(/pluginId:\s*"([^"]+)"/); - const pluginId = pluginMatch?.[1]; - if (pluginId && bundledWebSearchPluginIds.has(pluginId)) { - entries.push({ - file: relativeFile, - line: lineNumber, - kind: "registry-smell", - specifier: pluginId, - resolvedPath: relativeFile, - reason: "hardcodes bundled web search plugin ownership in core registry", - }); - } - - const providerMatch = line.match(/id:\s*"(brave|firecrawl|gemini|grok|kimi|perplexity)"/); - const providerId = providerMatch?.[1]; - if (providerId && bundledWebSearchProviders.has(providerId)) { - entries.push({ - file: relativeFile, - line: lineNumber, - kind: "registry-smell", - specifier: providerId, - resolvedPath: relativeFile, - reason: "hardcodes bundled web search provider metadata in core registry", - }); - } - } - - return entries; -} - const boundaryChecker = createExtensionImportBoundaryChecker({ roots: ["src/plugins"], shouldSkipFile(relativeFile) { return ( - relativeFile === "src/plugins/bundled-web-search-registry.ts" || relativeFile.startsWith("src/plugins/contracts/") || /^src\/plugins\/runtime\/runtime-[^/]+-contract\.[cm]?[jt]s$/u.test(relativeFile) ); }, - collectEntries({ source, filePath, relativeFile, references }) { - return [ - ...references.map(({ kind, line, specifier }) => { - const resolvedPath = resolveRepoSpecifier(repoRoot, specifier, filePath); - return { - file: relativeFile, - line, - kind, - specifier, - resolvedPath, - reason: classifyResolvedExtensionReason(kind, resolvedPath), - }; - }), - ...scanWebSearchRegistrySmells(source, relativeFile), - ]; + collectEntries({ filePath, relativeFile, references }) { + return references.map(({ kind, line, specifier }) => { + const resolvedPath = resolveRepoSpecifier(repoRoot, specifier, filePath); + return { + file: relativeFile, + line, + kind, + specifier, + resolvedPath, + reason: classifyResolvedExtensionReason(kind, resolvedPath), + }; + }); }, compareEntries, }); -/** Cached inventory of src/plugins imports that cross into bundled extensions. */ -const collectPluginExtensionImportBoundaryInventory = boundaryChecker.collectInventory; - -/** - * Cached expected plugin-extension import inventory baseline. - */ -const readExpectedInventory = createCachedAsync( - async (): Promise => - JSON.parse(await fs.readFile(baselinePath, "utf8")), -); - -/** - * Diffs expected and actual plugin-extension boundary inventory entries. - */ -function diffInventory( - expected: PluginExtensionInventoryEntry[], - actual: PluginExtensionInventoryEntry[], -) { - return diffInventoryEntries(expected, actual, compareEntries); +/** Rejects retired core registries whose ownership now comes from plugin manifests. */ +export function collectRetiredWebSearchCorePathEntries( + rootDir = repoRoot, +): PluginExtensionInventoryEntry[] { + return RETIRED_WEB_SEARCH_CORE_MODULES.flatMap((modulePath) => + AUTHORED_MODULE_EXTENSIONS.map((extension) => `${modulePath}${extension}`), + ) + .filter((relativeFile) => existsSync(path.join(rootDir, relativeFile))) + .map((relativeFile) => ({ + file: relativeFile, + line: 1, + kind: "retired-path", + specifier: relativeFile, + resolvedPath: relativeFile, + reason: "restores retired core web-search registry or factory ownership", + })); } +/** Inventory of src/plugins extension imports and retired core web-search ownership paths. */ +async function collectPluginExtensionImportBoundaryInventory() { + return [ + ...(await boundaryChecker.collectInventory()), + ...collectRetiredWebSearchCorePathEntries(), + ].toSorted(compareEntries); +} + +const ruleText = + "Rule: src/plugins/** must not import bundled plugin files or restore retired web-search registries"; const formatInventoryHuman = (inventory: PluginExtensionInventoryEntry[]) => formatGroupedInventoryHuman( { - rule: "Rule: src/plugins/** must not import bundled plugin files", + rule: ruleText, cleanMessage: "No plugin import boundary violations found.", inventoryTitle: "Plugin extension import boundary inventory:", }, inventory, ); -function formatEntry(entry: PluginExtensionInventoryEntry) { - return `${entry.file}:${entry.line} [${entry.kind}] ${entry.reason} (${entry.specifier} -> ${entry.resolvedPath})`; -} - /** - * Runs the plugin-extension import boundary baseline check. + * Runs the plugin-extension import boundary check. */ -async function runPluginExtensionImportBoundaryCheck(argv?: string[], io?: unknown) { - return await runBaselineInventoryCheck({ - argv: argv ?? process.argv.slice(2), - io, - collectActual: collectPluginExtensionImportBoundaryInventory, - readExpected: readExpectedInventory, - diffInventory, - formatInventoryHuman, - formatEntry, - }); +async function runPluginExtensionImportBoundaryCheck( + argv: string[] = process.argv.slice(2), + streams: ScriptIo = { stdout: process.stdout, stderr: process.stderr }, +): Promise<0 | 1> { + const json = argv.includes("--json"); + const actual = await collectPluginExtensionImportBoundaryInventory(); + + if (json) { + writeLine(streams.stdout, JSON.stringify(actual, null, 2)); + return actual.length > 0 ? 1 : 0; + } + + writeLine(streams.stdout, formatInventoryHuman(actual)); + if (actual.length === 0) { + return 0; + } + writeLine(streams.stderr, `${ruleText} violations found (${actual.length}).`); + return 1; } /** * Entrypoint wrapper for the plugin-extension import boundary check. */ -export async function main(argv?: string[], io?: unknown) { +export async function main(argv?: string[], io?: ScriptIo): Promise<0 | 1> { const exitCode = await runPluginExtensionImportBoundaryCheck(argv, io); - if (!io && exitCode !== 0) { - process.exit(exitCode); + if (!io) { + process.exitCode = exitCode; } return exitCode; } diff --git a/scripts/check-web-search-provider-boundaries.mts b/scripts/check-web-search-provider-boundaries.mts deleted file mode 100644 index fcf971c2488f..000000000000 --- a/scripts/check-web-search-provider-boundaries.mts +++ /dev/null @@ -1,282 +0,0 @@ -#!/usr/bin/env node - -// Inventories core web-search surfaces that still mention bundled providers. -import { promises as fs } from "node:fs"; -import path from "node:path"; -import { z } from "zod"; -import { diffInventoryEntries, runBaselineInventoryCheck } from "./lib/guard-inventory-utils.mjs"; -import { resolveRepoRoot } from "./lib/repo-root.mjs"; -import { collectSourceFileContents } from "./lib/source-file-scan-cache.mts"; -import { runAsScript } from "./lib/ts-guard-utils.mts"; -const repoRoot = resolveRepoRoot(import.meta.url); -const baselinePath = path.join( - repoRoot, - "test", - "fixtures", - "web-search-provider-boundary-inventory.json", -); - -const scanRoots = ["src"]; -const scanExtensions = new Set([".ts", ".js", ".mjs", ".cjs"]); -const ignoredDirNames = new Set([ - ".artifacts", - ".git", - ".turbo", - "build", - "coverage", - "dist", - "extensions", - "node_modules", -]); - -const bundledProviderPluginToSearchProvider = new Map([ - ["brave", "brave"], - ["firecrawl", "firecrawl"], - ["google", "gemini"], - ["moonshot", "kimi"], - ["perplexity", "perplexity"], - ["xai", "grok"], -]); - -const providerIds = new Set([ - "brave", - "firecrawl", - "gemini", - "grok", - "kimi", - "perplexity", - "shared", -]); - -const allowedGenericFiles = new Set([ - "src/agents/tools/web-search.ts", - "src/commands/onboard-search.ts", - "src/plugins/bundled-web-search-registry.ts", - "src/secrets/runtime-web-tools.ts", - "src/web-search/runtime.ts", -]); - -const ignoredFiles = new Set([ - "src/config/config.web-search-provider.test.ts", - "src/plugins/contracts/loader.contract.test.ts", - "src/plugins/contracts/registry.contract.test.ts", - "src/plugins/web-search-providers.test.ts", - "src/secrets/runtime-web-tools.test.ts", -]); - -const inventoryEntrySchema = z.looseObject({ - file: z.string(), - line: z.number(), - provider: z.string(), - reason: z.string(), -}); -type InventoryEntry = z.infer; -type ScriptIo = { - stdout: { write(chunk: string): unknown }; - stderr: { write(chunk: string): unknown }; -}; - -let webSearchProviderInventoryPromise: Promise | undefined; - -function compareInventoryEntries(left: InventoryEntry, right: InventoryEntry) { - return ( - left.provider.localeCompare(right.provider) || - left.file.localeCompare(right.file) || - left.line - right.line || - left.reason.localeCompare(right.reason) - ); -} - -function pushEntry(inventory: InventoryEntry[], entry: InventoryEntry) { - if (!providerIds.has(entry.provider)) { - throw new Error(`Unknown provider id in boundary inventory: ${entry.provider}`); - } - inventory.push(entry); -} - -function scanWebSearchProviderRegistry( - lines: string[], - relativeFile: string, - inventory: InventoryEntry[], -) { - for (const [index, line] of lines.entries()) { - const lineNumber = index + 1; - - if (line.includes("firecrawl-search-provider.js")) { - pushEntry(inventory, { - provider: "shared", - file: relativeFile, - line: lineNumber, - reason: "imports extension web search provider implementation into core registry", - }); - } - - if (line.includes("web-search-plugin-factory.js")) { - pushEntry(inventory, { - provider: "shared", - file: relativeFile, - line: lineNumber, - reason: "imports shared web search provider registration helper into core registry", - }); - } - - const pluginMatch = line.match(/pluginId:\s*"([^"]+)"/); - const pluginId = pluginMatch?.[1]; - const providerFromPlugin = pluginId - ? bundledProviderPluginToSearchProvider.get(pluginId) - : undefined; - if (providerFromPlugin) { - pushEntry(inventory, { - provider: providerFromPlugin, - file: relativeFile, - line: lineNumber, - reason: "hardcodes bundled web search plugin ownership in core registry", - }); - } - - const providerMatch = line.match(/id:\s*"(brave|firecrawl|gemini|grok|kimi|perplexity)"/); - const providerId = providerMatch?.[1]; - if (providerId) { - pushEntry(inventory, { - provider: providerId, - file: relativeFile, - line: lineNumber, - reason: "hardcodes bundled web search provider id in core registry", - }); - } - } -} - -function scanGenericCoreImports( - lines: string[], - relativeFile: string, - inventory: InventoryEntry[], -) { - if (allowedGenericFiles.has(relativeFile)) { - return; - } - for (const [index, line] of lines.entries()) { - const lineNumber = index + 1; - if (line.includes("web-search-providers.js")) { - pushEntry(inventory, { - provider: "shared", - file: relativeFile, - line: lineNumber, - reason: "imports bundled web search registry outside allowed generic plumbing", - }); - } - if (line.includes("web-search-plugin-factory.js")) { - pushEntry(inventory, { - provider: "shared", - file: relativeFile, - line: lineNumber, - reason: "imports web search provider registration helper outside extensions", - }); - } - } -} - -/** - * Collects web-search provider boundary inventory from core source files. - */ -async function collectWebSearchProviderBoundaryInventory() { - if (!webSearchProviderInventoryPromise) { - webSearchProviderInventoryPromise = (async () => { - const inventory: InventoryEntry[] = []; - const files = await collectSourceFileContents({ - repoRoot, - scanRoots, - scanExtensions, - ignoredDirNames, - }); - - for (const { relativeFile, content } of files) { - if (ignoredFiles.has(relativeFile) || relativeFile.includes(".test.")) { - continue; - } - const lines = content.split(/\r?\n/); - - if (relativeFile === "src/plugins/web-search-providers.ts") { - scanWebSearchProviderRegistry(lines, relativeFile, inventory); - continue; - } - - scanGenericCoreImports(lines, relativeFile, inventory); - } - - return inventory.toSorted(compareInventoryEntries); - })(); - } - return await webSearchProviderInventoryPromise; -} - -/** - * Reads the expected web-search provider boundary inventory baseline. - */ -async function readExpectedInventory(): Promise { - try { - const parsed: unknown = JSON.parse(await fs.readFile(baselinePath, "utf8")); - const result = z.array(inventoryEntrySchema).safeParse(parsed); - return result.success ? result.data : []; - } catch (error) { - if (error && typeof error === "object" && "code" in error && error.code === "ENOENT") { - return []; - } - throw error; - } -} - -/** - * Diffs expected and actual web-search provider boundary inventory entries. - */ -function diffInventory(expected: InventoryEntry[], actual: InventoryEntry[]) { - return diffInventoryEntries(expected, actual, compareInventoryEntries); -} - -function formatInventoryHuman(inventory: InventoryEntry[]) { - if (inventory.length === 0) { - return "No web search provider boundary inventory entries found."; - } - const lines = ["Web search provider boundary inventory:"]; - let activeProvider = ""; - for (const entry of inventory) { - if (entry.provider !== activeProvider) { - activeProvider = entry.provider; - lines.push(`${activeProvider}:`); - } - lines.push(` - ${entry.file}:${entry.line} ${entry.reason}`); - } - return lines.join("\n"); -} - -function formatEntry(entry: InventoryEntry) { - return `${entry.provider} ${entry.file}:${entry.line} ${entry.reason}`; -} - -/** - * Runs the web-search provider boundary baseline check. - */ -async function runWebSearchProviderBoundaryCheck(argv?: string[], io?: ScriptIo) { - return await runBaselineInventoryCheck({ - argv: argv ?? process.argv.slice(2), - io, - collectActual: collectWebSearchProviderBoundaryInventory, - readExpected: readExpectedInventory, - diffInventory, - formatInventoryHuman, - formatEntry, - }); -} - -/** - * Entrypoint wrapper for the web-search provider boundary check. - */ -export async function main(argv?: string[], io?: ScriptIo) { - const exitCode = await runWebSearchProviderBoundaryCheck(argv, io); - if (!io && exitCode !== 0) { - process.exit(exitCode); - } - return exitCode; -} - -runAsScript(import.meta.url, main); diff --git a/scripts/run-additional-boundary-checks.mts b/scripts/run-additional-boundary-checks.mts index 21f2a907ef75..cff745da9863 100644 --- a/scripts/run-additional-boundary-checks.mts +++ b/scripts/run-additional-boundary-checks.mts @@ -88,7 +88,6 @@ export const BOUNDARY_CHECKS = ( ["run", "lint:plugins:plugin-sdk-subpaths-exported"], ], ["deps:root-ownership:check", "pnpm", ["deps:root-ownership:check"]], - ["web-search-provider-boundary", "pnpm", ["run", "lint:web-search-provider-boundaries"]], ["web-fetch-provider-boundary", "pnpm", ["run", "lint:web-fetch-provider-boundaries"]], [ "extension-src-outside-plugin-sdk-boundary", diff --git a/test/fixtures/plugin-extension-import-boundary-inventory.json b/test/fixtures/plugin-extension-import-boundary-inventory.json deleted file mode 100644 index fe51488c7066..000000000000 --- a/test/fixtures/plugin-extension-import-boundary-inventory.json +++ /dev/null @@ -1 +0,0 @@ -[] diff --git a/test/plugin-extension-import-boundary.test.ts b/test/plugin-extension-import-boundary.test.ts index 1202081d8a9f..61d54e7dcf26 100644 --- a/test/plugin-extension-import-boundary.test.ts +++ b/test/plugin-extension-import-boundary.test.ts @@ -1,26 +1,38 @@ // Plugin extension import boundary tests enforce plugin extension import rules. -import { readFileSync } from "node:fs"; +import { mkdirSync, writeFileSync } from "node:fs"; import path from "node:path"; -import { describe, expect, it } from "vitest"; -import { main } from "../scripts/check-plugin-extension-import-boundary.mts"; +import { afterEach, describe, expect, it } from "vitest"; +import { + collectRetiredWebSearchCorePathEntries, + main, +} from "../scripts/check-plugin-extension-import-boundary.mts"; import { createCapturedIo } from "./helpers/captured-io.js"; +import { useAutoCleanupTempDirTracker } from "./helpers/temp-dir.js"; -const repoRoot = process.cwd(); -const baselinePath = path.join( - repoRoot, - "test", - "fixtures", - "plugin-extension-import-boundary-inventory.json", -); -const baseline = JSON.parse(readFileSync(baselinePath, "utf8")); +const tempDirs = useAutoCleanupTempDirTracker(afterEach); describe("plugin extension import boundary inventory", () => { - it("script json output matches the baseline exactly", async () => { + it("current tree has no plugin extension imports", async () => { const captured = createCapturedIo(); const exitCode = await main(["--json"], captured.io); expect(exitCode).toBe(0); expect(captured.readStderr()).toBe(""); - expect(JSON.parse(captured.readStdout())).toEqual(baseline); + expect(JSON.parse(captured.readStdout())).toEqual([]); + }); + + it("rejects retired core web-search ownership paths", () => { + const root = tempDirs.make("openclaw-retired-web-search-"); + const relativeFile = "src/plugins/web-search-providers.mjs"; + const filePath = path.join(root, relativeFile); + mkdirSync(path.dirname(filePath), { recursive: true }); + writeFileSync(filePath, "export {};\n", "utf8"); + + expect(collectRetiredWebSearchCorePathEntries(root)).toEqual([ + expect.objectContaining({ + file: relativeFile, + kind: "retired-path", + }), + ]); }); }); diff --git a/test/web-provider-boundary.test.ts b/test/web-provider-boundary.test.ts index 2a26e3c43543..5ac8f76ad8f4 100644 --- a/test/web-provider-boundary.test.ts +++ b/test/web-provider-boundary.test.ts @@ -1,11 +1,9 @@ // Web provider boundary tests enforce provider import boundaries. import { describe, expect, it } from "vitest"; import { main as webFetchMain } from "../scripts/check-web-fetch-provider-boundaries.mts"; -import { main as webSearchMain } from "../scripts/check-web-search-provider-boundaries.mts"; import { createCapturedIo } from "./helpers/captured-io.js"; const webFetchJsonOutputPromise = getJsonOutput(webFetchMain); -const webSearchJsonOutputPromise = getJsonOutput(webSearchMain); async function getJsonOutput( main: (argv: string[], io: ReturnType["io"]) => Promise, @@ -27,12 +25,4 @@ describe("web provider boundaries", () => { expect(jsonOutput.stderr).toBe(""); expect(jsonOutput.json).toStrictEqual([]); }); - - it("runs the web search boundary script in JSON mode", async () => { - const jsonOutput = await webSearchJsonOutputPromise; - - expect(jsonOutput.exitCode).toBe(0); - expect(jsonOutput.stderr).toBe(""); - expect(jsonOutput.json).toStrictEqual([]); - }); });