fix(plugins): keep test helpers out of contract barrels (#63311)

Merged via squash.

Prepared head SHA: 769e90c6af
Co-authored-by: altaywtf <9790196+altaywtf@users.noreply.github.com>
Co-authored-by: altaywtf <9790196+altaywtf@users.noreply.github.com>
Reviewed-by: @altaywtf
This commit is contained in:
Altay
2026-04-08 22:59:05 +01:00
committed by GitHub
parent 9ffe216a52
commit 554bc0a9fd
21 changed files with 529 additions and 62 deletions

View File

@@ -16,6 +16,7 @@ import {
loadPluginManifestRegistry,
type PluginManifestRecord,
} from "../plugins/manifest-registry.js";
import { normalizeBundledPluginArtifactSubpath } from "../plugins/public-surface-runtime.js";
const ALWAYS_ALLOWED_RUNTIME_DIR_NAMES = new Set([
"image-generation-core",
@@ -166,7 +167,7 @@ export function resolveRegistryPluginModuleLocation(params: {
(plugin) => path.basename(plugin.rootDir) === params.dirName,
(plugin) => plugin.channels.includes(params.dirName),
];
const artifactBasename = params.artifactBasename.replace(/^\.\//u, "");
const artifactBasename = normalizeBundledPluginArtifactSubpath(params.artifactBasename);
const sourceBaseName = artifactBasename.replace(/\.js$/u, "");
for (const matchFn of tiers) {
for (const record of registry.filter(matchFn)) {

View File

@@ -4,7 +4,10 @@ import path from "node:path";
import { fileURLToPath } from "node:url";
import { openBoundaryFileSync } from "../infra/boundary-file-read.js";
import { resolveBundledPluginsDir } from "../plugins/bundled-dir.js";
import { resolveBundledPluginPublicSurfacePath } from "../plugins/public-surface-runtime.js";
import {
normalizeBundledPluginArtifactSubpath,
resolveBundledPluginPublicSurfacePath,
} from "../plugins/public-surface-runtime.js";
import {
buildPluginLoaderJitiOptions,
resolvePluginLoaderJitiConfig,
@@ -62,7 +65,8 @@ function resolveSourceFirstPublicSurfacePath(params: {
dirName: string;
artifactBasename: string;
}): string | null {
const sourceBaseName = params.artifactBasename.replace(/\.js$/u, "");
const artifactBasename = normalizeBundledPluginArtifactSubpath(params.artifactBasename);
const sourceBaseName = artifactBasename.replace(/\.js$/u, "");
const sourceRoot =
params.bundledPluginsDir ?? path.resolve(getOpenClawPackageRoot(), "extensions");
for (const ext of PUBLIC_SURFACE_SOURCE_EXTENSIONS) {

View File

@@ -4,7 +4,10 @@ import path from "node:path";
import { fileURLToPath } from "node:url";
import { resolveBundledPluginsDir } from "../plugins/bundled-dir.js";
import type { PluginManifestRecord } from "../plugins/manifest-registry.js";
import { resolveBundledPluginPublicSurfacePath } from "../plugins/public-surface-runtime.js";
import {
normalizeBundledPluginArtifactSubpath,
resolveBundledPluginPublicSurfacePath,
} from "../plugins/public-surface-runtime.js";
import { resolveLoaderPackageRoot } from "../plugins/sdk-alias.js";
import {
loadBundledPluginPublicSurfaceModuleSync as loadBundledPluginPublicSurfaceModuleSyncLight,
@@ -44,7 +47,8 @@ function resolveSourceFirstPublicSurfacePath(params: {
dirName: string;
artifactBasename: string;
}): string | null {
const sourceBaseName = params.artifactBasename.replace(/\.js$/u, "");
const artifactBasename = normalizeBundledPluginArtifactSubpath(params.artifactBasename);
const sourceBaseName = artifactBasename.replace(/\.js$/u, "");
const sourceRoot = params.bundledPluginsDir ?? path.resolve(OPENCLAW_PACKAGE_ROOT, "extensions");
for (const ext of PUBLIC_SURFACE_SOURCE_EXTENSIONS) {
const candidate = path.resolve(sourceRoot, params.dirName, `${sourceBaseName}${ext}`);
@@ -66,7 +70,7 @@ function resolveRegistryPluginModuleLocationFromRegistry(params: {
(plugin) => path.basename(plugin.rootDir) === params.dirName,
(plugin) => plugin.channels.includes(params.dirName),
];
const artifactBasename = params.artifactBasename.replace(/^\.\//u, "");
const artifactBasename = normalizeBundledPluginArtifactSubpath(params.artifactBasename);
const sourceBaseName = artifactBasename.replace(/\.js$/u, "");
for (const matchFn of tiers) {
for (const record of params.registry.filter(matchFn)) {

View File

@@ -1,11 +1,30 @@
import { readFileSync } from "node:fs";
import { resolve } from "node:path";
import { existsSync, readFileSync } from "node:fs";
import path, { dirname, relative, resolve } from "node:path";
import { fileURLToPath } from "node:url";
import ts from "typescript";
import { describe, expect, it } from "vitest";
import { listBundledPluginMetadata } from "../bundled-plugin-metadata.js";
import { loadPluginManifestRegistry } from "../manifest-registry.js";
const CORE_PLUGIN_ENTRY_IMPORT_RE =
/import\s*\{[^}]*\bdefinePluginEntry\b[^}]*\}\s*from\s*"openclaw\/plugin-sdk\/core"/;
const RUNTIME_ENTRY_HELPER_RE = /(^|\/)plugin-entry\.runtime\.[cm]?[jt]s$/;
const REPO_ROOT = resolve(fileURLToPath(new URL("../../..", import.meta.url)));
const RUNTIME_ENTRY_HELPER_RE = /(^|\/)plugin-entry\.runtime\.[cm]?[jt]s$/;
const SOURCE_MODULE_EXTENSIONS = [".ts", ".mts", ".cts", ".js", ".mjs", ".cjs"] as const;
const FORBIDDEN_CONTRACT_MODULE_SPECIFIER_PATTERNS = [
/^vitest$/u,
/^openclaw\/plugin-sdk\/testing$/u,
/(^|\/)test-api(?:\.[cm]?[jt]s)?$/u,
/(^|\/)__tests__(\/|$)/u,
/(^|\/)test-support(\/|$)/u,
/(^|\/)[^/]*\.test(?:[-.][^/]*)?(?:\.[cm]?[jt]s)?$/u,
/(^|\/)[^/]*(?:test-harness|test-plugin|test-helper|test-support|harness)[^/]*(?:\.[cm]?[jt]s)?$/u,
] as const;
const FORBIDDEN_CONTRACT_MODULE_PATH_PATTERNS = [
/(^|\/)__tests__(\/|$)/u,
/(^|\/)test-support(\/|$)/u,
/(^|\/)test-api\.[cm]?[jt]s$/u,
/(^|\/)[^/]*\.test(?:[-.][^/]*)?\.[cm]?[jt]s$/u,
/(^|\/)[^/]*(?:test-harness|test-plugin|test-helper|test-support|harness)[^/]*\.[cm]?[jt]s$/u,
] as const;
function listBundledPluginRoots() {
return loadPluginManifestRegistry({})
.plugins.filter((plugin) => plugin.origin === "bundled")
@@ -16,6 +35,189 @@ function listBundledPluginRoots() {
.toSorted((left, right) => left.pluginId.localeCompare(right.pluginId));
}
function resolvePublicSurfaceSourcePath(
pluginDir: string,
artifactBasename: string,
): string | null {
const stem = artifactBasename.replace(/\.[^.]+$/u, "");
for (const extension of SOURCE_MODULE_EXTENSIONS) {
const candidate = resolve(pluginDir, `${stem}${extension}`);
if (existsSync(candidate)) {
return candidate;
}
}
return null;
}
function isGuardedContractArtifactBasename(artifactBasename: string): boolean {
return (
artifactBasename === "channel-config-api.js" || artifactBasename.endsWith("contract-api.js")
);
}
function collectProductionContractEntryPaths(): Array<{
pluginId: string;
entryPath: string;
pluginRoot: string;
}> {
return listBundledPluginMetadata({ rootDir: REPO_ROOT }).flatMap((plugin) => {
const pluginRoot = resolve(REPO_ROOT, "extensions", plugin.dirName);
const entryPaths = new Set<string>();
for (const artifact of plugin.publicSurfaceArtifacts ?? []) {
if (!isGuardedContractArtifactBasename(artifact)) {
continue;
}
const sourcePath = resolvePublicSurfaceSourcePath(pluginRoot, artifact);
if (sourcePath) {
entryPaths.add(sourcePath);
}
}
return [...entryPaths].map((entryPath) => ({
pluginId: plugin.manifest.id,
entryPath,
pluginRoot,
}));
});
}
function formatRepoRelativePath(filePath: string): string {
return relative(REPO_ROOT, filePath).replaceAll(path.sep, "/");
}
function analyzeSourceModule(params: { filePath: string; source: string }): {
specifiers: string[];
relativeSpecifiers: string[];
importsDefinePluginEntryFromCore: boolean;
} {
const sourceFile = ts.createSourceFile(
params.filePath,
params.source,
ts.ScriptTarget.Latest,
true,
);
const specifiers = new Set<string>();
let importsDefinePluginEntryFromCore = false;
for (const statement of sourceFile.statements) {
if (ts.isImportDeclaration(statement)) {
const specifier = ts.isStringLiteral(statement.moduleSpecifier)
? statement.moduleSpecifier.text
: undefined;
if (specifier) {
specifiers.add(specifier);
}
if (
specifier === "openclaw/plugin-sdk/core" &&
statement.importClause?.namedBindings &&
ts.isNamedImports(statement.importClause.namedBindings) &&
statement.importClause.namedBindings.elements.some(
(element) => (element.propertyName?.text ?? element.name.text) === "definePluginEntry",
)
) {
importsDefinePluginEntryFromCore = true;
}
continue;
}
if (!ts.isExportDeclaration(statement)) {
continue;
}
if (statement.moduleSpecifier && ts.isStringLiteral(statement.moduleSpecifier)) {
specifiers.add(statement.moduleSpecifier.text);
}
}
const nextSpecifiers = [...specifiers];
return {
specifiers: nextSpecifiers,
relativeSpecifiers: nextSpecifiers.filter((specifier) => specifier.startsWith(".")),
importsDefinePluginEntryFromCore,
};
}
function matchesForbiddenContractSpecifier(specifier: string): boolean {
return FORBIDDEN_CONTRACT_MODULE_SPECIFIER_PATTERNS.some((pattern) => pattern.test(specifier));
}
function collectForbiddenContractSpecifiers(specifiers: readonly string[]): string[] {
return specifiers.filter((specifier) => matchesForbiddenContractSpecifier(specifier));
}
function resolveRelativeSourceModulePath(fromPath: string, specifier: string): string | null {
const rawTargetPath = resolve(dirname(fromPath), specifier);
const candidates = new Set<string>();
const rawExtension = path.extname(rawTargetPath);
if (rawExtension) {
candidates.add(rawTargetPath);
const stem = rawTargetPath.slice(0, -rawExtension.length);
for (const extension of SOURCE_MODULE_EXTENSIONS) {
candidates.add(`${stem}${extension}`);
}
} else {
for (const extension of SOURCE_MODULE_EXTENSIONS) {
candidates.add(`${rawTargetPath}${extension}`);
candidates.add(resolve(rawTargetPath, `index${extension}`));
}
}
for (const candidate of candidates) {
if (existsSync(candidate)) {
return candidate;
}
}
return null;
}
function findForbiddenContractModuleGraphPaths(params: {
entryPath: string;
pluginRoot: string;
}): string[] {
const failures: string[] = [];
const visited = new Set<string>();
const pending = [params.entryPath];
while (pending.length > 0) {
const currentPath = pending.pop();
if (!currentPath || visited.has(currentPath)) {
continue;
}
visited.add(currentPath);
const repoRelativePath = formatRepoRelativePath(currentPath);
for (const pattern of FORBIDDEN_CONTRACT_MODULE_PATH_PATTERNS) {
if (pattern.test(repoRelativePath)) {
failures.push(`${repoRelativePath} matched ${pattern}`);
}
}
const source = readFileSync(currentPath, "utf8");
const analysis = analyzeSourceModule({ filePath: currentPath, source });
for (const specifier of collectForbiddenContractSpecifiers(analysis.specifiers)) {
failures.push(`${repoRelativePath} imported ${specifier}`);
}
for (const specifier of analysis.relativeSpecifiers) {
const resolvedModulePath = resolveRelativeSourceModulePath(currentPath, specifier);
if (!resolvedModulePath) {
continue;
}
if (resolvedModulePath === currentPath) {
continue;
}
if (!resolvedModulePath.startsWith(params.pluginRoot + path.sep)) {
continue;
}
pending.push(resolvedModulePath);
}
}
return failures;
}
describe("plugin entry guardrails", () => {
it("keeps bundled extension entry modules off direct definePluginEntry imports from core", () => {
const failures: string[] = [];
@@ -24,7 +226,7 @@ describe("plugin entry guardrails", () => {
const indexPath = resolve(plugin.rootDir, "index.ts");
try {
const source = readFileSync(indexPath, "utf8");
if (CORE_PLUGIN_ENTRY_IMPORT_RE.test(source)) {
if (analyzeSourceModule({ filePath: indexPath, source }).importsDefinePluginEntryFromCore) {
failures.push(`extensions/${plugin.pluginId}/index.ts`);
}
} catch {
@@ -59,4 +261,62 @@ describe("plugin entry guardrails", () => {
expect(failures).toEqual([]);
});
it("keeps bundled production contract barrels off test-only imports and re-exports", () => {
const failures = collectProductionContractEntryPaths().flatMap(
({ pluginId, entryPath, pluginRoot }) =>
findForbiddenContractModuleGraphPaths({
entryPath,
pluginRoot,
}).map((failure) => `${pluginId}: ${failure}`),
);
expect(failures).toEqual([]);
});
it("follows relative import edges while scanning guarded contract graphs", () => {
expect(
analyzeSourceModule({
filePath: "guardrail-fixture.ts",
source: `
import { x } from "./safe.js";
import "./setup.js";
export { x };
export * from "./barrel.js";
import { y } from "openclaw/plugin-sdk/testing";
`,
}).relativeSpecifiers.toSorted(),
).toEqual(["./barrel.js", "./safe.js", "./setup.js"]);
});
it("guards contract-style production artifacts beyond the legacy allowlist", () => {
expect(isGuardedContractArtifactBasename("channel-config-api.js")).toBe(true);
expect(isGuardedContractArtifactBasename("contract-api.js")).toBe(true);
expect(isGuardedContractArtifactBasename("doctor-contract-api.js")).toBe(true);
expect(isGuardedContractArtifactBasename("web-search-contract-api.js")).toBe(true);
expect(isGuardedContractArtifactBasename("test-api.js")).toBe(false);
});
it("flags test-support directory hops in guarded contract graphs", () => {
expect(collectForbiddenContractSpecifiers(["./test-support/index.js"])).toEqual([
"./test-support/index.js",
]);
expect(
FORBIDDEN_CONTRACT_MODULE_PATH_PATTERNS.some((pattern) =>
pattern.test("extensions/demo/src/test-support/index.ts"),
),
).toBe(true);
});
it("detects aliased definePluginEntry imports from core", () => {
expect(
analyzeSourceModule({
filePath: "aliased-plugin-entry.ts",
source: `
import { definePluginEntry as dpe } from "openclaw/plugin-sdk/core";
import { somethingElse } from "openclaw/plugin-sdk/core";
`,
}).importsDefinePluginEntryFromCore,
).toBe(true);
});
});

View File

@@ -0,0 +1,32 @@
import { describe, expect, it } from "vitest";
import { normalizeBundledPluginArtifactSubpath } from "./public-surface-runtime.js";
describe("bundled plugin public surface runtime", () => {
it("allows plugin-local nested artifact paths", () => {
expect(normalizeBundledPluginArtifactSubpath("src/outbound-adapter.js")).toBe(
"src/outbound-adapter.js",
);
expect(normalizeBundledPluginArtifactSubpath("./test-api.js")).toBe("test-api.js");
});
it("rejects artifact paths that escape the plugin root", () => {
expect(() => normalizeBundledPluginArtifactSubpath("../outside.js")).toThrow(
/must stay plugin-local/,
);
expect(() => normalizeBundledPluginArtifactSubpath("src/../outside.js")).toThrow(
/must stay plugin-local/,
);
expect(() => normalizeBundledPluginArtifactSubpath("/tmp/outside.js")).toThrow(
/must stay plugin-local/,
);
expect(() => normalizeBundledPluginArtifactSubpath("..\\outside.js")).toThrow(
/must stay plugin-local/,
);
expect(() => normalizeBundledPluginArtifactSubpath("C:outside.js")).toThrow(
/must stay plugin-local/,
);
expect(() => normalizeBundledPluginArtifactSubpath("src/C:outside.js")).toThrow(
/must stay plugin-local/,
);
});
});

View File

@@ -4,6 +4,33 @@ import { resolveBundledPluginsDir } from "./bundled-dir.js";
const PUBLIC_SURFACE_SOURCE_EXTENSIONS = [".ts", ".mts", ".js", ".mjs", ".cts", ".cjs"] as const;
export function normalizeBundledPluginArtifactSubpath(artifactBasename: string): string {
if (
path.posix.isAbsolute(artifactBasename) ||
path.win32.isAbsolute(artifactBasename) ||
artifactBasename.includes("\\")
) {
throw new Error(`Bundled plugin artifact path must stay plugin-local: ${artifactBasename}`);
}
const normalized = artifactBasename.replace(/^\.\//u, "");
if (!normalized) {
throw new Error("Bundled plugin artifact path must not be empty");
}
const segments = normalized.split("/");
if (
segments.some(
(segment) =>
segment.length === 0 || segment === "." || segment === ".." || segment.includes(":"),
)
) {
throw new Error(`Bundled plugin artifact path must stay plugin-local: ${artifactBasename}`);
}
return normalized;
}
export function resolveBundledPluginPublicSurfacePath(params: {
rootDir: string;
dirName: string;
@@ -11,10 +38,7 @@ export function resolveBundledPluginPublicSurfacePath(params: {
env?: NodeJS.ProcessEnv;
bundledPluginsDir?: string;
}): string | null {
const artifactBasename = params.artifactBasename.replace(/^\.\//u, "");
if (!artifactBasename) {
return null;
}
const artifactBasename = normalizeBundledPluginArtifactSubpath(params.artifactBasename);
const explicitBundledPluginsDir =
params.bundledPluginsDir ?? resolveBundledPluginsDir(params.env ?? process.env);

View File

@@ -5,6 +5,7 @@ import {
findBundledPluginMetadataById,
type BundledPluginMetadata,
} from "../plugins/bundled-plugin-metadata.js";
import { normalizeBundledPluginArtifactSubpath } from "../plugins/public-surface-runtime.js";
import { resolveLoaderPackageRoot } from "../plugins/sdk-alias.js";
const OPENCLAW_PACKAGE_ROOT =
@@ -28,7 +29,7 @@ export function loadBundledPluginPublicSurfaceSync<T extends object>(params: {
const metadata = findBundledPluginMetadata(params.pluginId);
return loadBundledPluginPublicSurfaceModuleSync<T>({
dirName: metadata.dirName,
artifactBasename: params.artifactBasename,
artifactBasename: normalizeBundledPluginArtifactSubpath(params.artifactBasename),
});
}
@@ -46,11 +47,12 @@ export function resolveRelativeBundledPluginPublicModuleId(params: {
}): string {
const metadata = findBundledPluginMetadata(params.pluginId);
const fromFilePath = fileURLToPath(params.fromModuleUrl);
const artifactBasename = normalizeBundledPluginArtifactSubpath(params.artifactBasename);
const targetPath = path.resolve(
OPENCLAW_PACKAGE_ROOT,
"extensions",
metadata.dirName,
params.artifactBasename,
artifactBasename,
);
const relativePath = path
.relative(path.dirname(fromFilePath), targetPath)