From dfce5f695871aa2230037fdfb024f4c75fd180ef Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 21 Aug 2026 01:06:23 -0700 Subject: [PATCH] perf(skills): reuse plugin metadata for watcher refresh (#127088) Amp-Thread-ID: https://ampcode.com/threads/T-01a021f5-984a-7628-a30c-491c166ff247 Co-authored-by: Amp --- src/skills/loading/plugin-skills.test.ts | 33 ++++++++++++++++++-- src/skills/loading/plugin-skills.ts | 4 +-- src/skills/runtime/refresh.test.ts | 34 ++++++++++++++++++++- src/skills/runtime/refresh.ts | 17 +++++++++-- src/skills/runtime/session-snapshot.test.ts | 5 ++- src/skills/runtime/session-snapshot.ts | 3 ++ 6 files changed, 88 insertions(+), 8 deletions(-) diff --git a/src/skills/loading/plugin-skills.test.ts b/src/skills/loading/plugin-skills.test.ts index 792f171d63ed..656b240f5558 100644 --- a/src/skills/loading/plugin-skills.test.ts +++ b/src/skills/loading/plugin-skills.test.ts @@ -13,7 +13,7 @@ import { createTrackedTempDirs } from "../../test-utils/tracked-temp-dirs.js"; const hoisted = vi.hoisted(() => { const loadManifestRegistry = vi.fn(); - const loadPluginMetadataSnapshot = vi.fn(() => { + const loadPluginMetadataSnapshot = vi.fn((_params?: unknown) => { const manifestRegistry = loadManifestRegistry(); return { manifestRegistry, @@ -24,10 +24,14 @@ const hoisted = vi.hoisted(() => { )?.id ?? pluginId, }; }); + const resolvePluginMetadataSnapshot = vi.fn((params: unknown) => + loadPluginMetadataSnapshot(params), + ); return { loadPluginManifestRegistryForInstalledIndex: loadManifestRegistry, loadPluginManifestRegistryForPluginRegistry: loadManifestRegistry, loadPluginMetadataSnapshot, + resolvePluginMetadataSnapshot, loadPluginRegistrySnapshot: vi.fn(() => ({ plugins: [] })), }; }); @@ -43,7 +47,7 @@ vi.mock("../../plugins/plugin-registry.js", () => ({ vi.mock("../../plugins/plugin-metadata-snapshot.js", () => ({ loadPluginMetadataSnapshot: hoisted.loadPluginMetadataSnapshot, - resolvePluginMetadataSnapshot: hoisted.loadPluginMetadataSnapshot, + resolvePluginMetadataSnapshot: hoisted.resolvePluginMetadataSnapshot, })); let resolvePluginSkillDirs: typeof import("./plugin-skills.js").resolvePluginSkillDirs; @@ -182,6 +186,7 @@ function registerHealthyAcpBackend() { afterEach(async () => { hoisted.loadPluginManifestRegistryForInstalledIndex.mockReset(); hoisted.loadPluginMetadataSnapshot.mockClear(); + hoisted.resolvePluginMetadataSnapshot.mockClear(); hoisted.loadPluginRegistrySnapshot.mockReset(); acpRuntimeTesting.resetAcpRuntimeBackendsForTests(); await tempDirs.cleanup(); @@ -221,6 +226,7 @@ describe("resolvePluginSkillDirs", () => { plugins: [], }); hoisted.loadPluginMetadataSnapshot.mockClear(); + hoisted.resolvePluginMetadataSnapshot.mockClear(); hoisted.loadPluginRegistrySnapshot.mockReset(); hoisted.loadPluginRegistrySnapshot.mockReturnValue({ plugins: [] }); }); @@ -273,6 +279,29 @@ describe("resolvePluginSkillDirs", () => { expect(dirs).toEqual(expectedDirs({ acpxRoot, helperRoot })); }); + it("reuses current lifecycle metadata before falling back to a cold load", async () => { + const { workspaceDir, acpxRoot, helperRoot } = await setupAcpxAndHelperRegistry(); + registerHealthyAcpBackend(); + const manifestRegistry = buildRegistry({ acpxRoot, helperRoot }); + hoisted.resolvePluginMetadataSnapshot.mockReturnValueOnce({ + manifestRegistry, + plugins: manifestRegistry.plugins, + normalizePluginId: (pluginId: string) => pluginId, + }); + + const dirs = resolvePluginSkillDirs({ + workspaceDir, + config: { + acp: { enabled: true }, + plugins: { entries: { acpx: { enabled: true }, helper: { enabled: true } } }, + } as OpenClawConfig, + }); + + expect(dirs).toEqual([path.resolve(acpxRoot, "skills"), path.resolve(helperRoot, "skills")]); + expect(hoisted.resolvePluginMetadataSnapshot).toHaveBeenCalledOnce(); + expect(hoisted.loadPluginMetadataSnapshot).not.toHaveBeenCalled(); + }); + it.each([ { name: "unavailable to available", diff --git a/src/skills/loading/plugin-skills.ts b/src/skills/loading/plugin-skills.ts index 0ec928172e42..0220d2ce36a8 100644 --- a/src/skills/loading/plugin-skills.ts +++ b/src/skills/loading/plugin-skills.ts @@ -12,7 +12,7 @@ import { } from "../../plugins/config-policy.js"; import { resolveMemorySlotDecision } from "../../plugins/config-state.js"; import { registerPluginMetadataProcessMemoLifecycleClear } from "../../plugins/plugin-metadata-lifecycle.js"; -import { loadPluginMetadataSnapshot } from "../../plugins/plugin-metadata-snapshot.js"; +import { resolvePluginMetadataSnapshot } from "../../plugins/plugin-metadata-snapshot.js"; import type { PluginMetadataSnapshot } from "../../plugins/plugin-metadata-snapshot.types.js"; import { hasKind } from "../../plugins/slots.js"; import { isPathInsideWithRealpath } from "../../security/scan-paths.js"; @@ -51,7 +51,7 @@ export function resolvePluginSkillDirs(params: { return []; } const config = params.config ?? {}; - const metadataSnapshot = loadPluginMetadataSnapshot({ + const metadataSnapshot = resolvePluginMetadataSnapshot({ workspaceDir, config, env: process.env, diff --git a/src/skills/runtime/refresh.test.ts b/src/skills/runtime/refresh.test.ts index 4a7b7df0a690..e525661071b8 100644 --- a/src/skills/runtime/refresh.test.ts +++ b/src/skills/runtime/refresh.test.ts @@ -3,6 +3,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import type { PluginMetadataSnapshot } from "../../plugins/plugin-metadata-snapshot.types.js"; import { withEnvAsync } from "../../test-utils/env.js"; import { bumpSkillsSnapshotVersion, @@ -38,6 +39,10 @@ const watchMock = vi.fn(() => { createdWatchers.push(watcher); return watcher; }); +const pluginSkillsMocks = vi.hoisted(() => ({ + resolvePluginSkillDirs: vi.fn((): string[] => []), + resolvePluginSkillDirsFromMetadata: vi.fn((): string[] => []), +})); let refreshModule: typeof import("./refresh.js"); let refreshTestSupport: typeof import("./refresh.test-support.js"); @@ -47,7 +52,8 @@ vi.mock("chokidar", () => ({ })); vi.mock("../loading/plugin-skills.js", () => ({ - resolvePluginSkillDirs: vi.fn(() => []), + resolvePluginSkillDirs: pluginSkillsMocks.resolvePluginSkillDirs, + resolvePluginSkillDirsFromMetadata: pluginSkillsMocks.resolvePluginSkillDirsFromMetadata, })); describe("ensureSkillsWatcher", () => { @@ -59,6 +65,8 @@ describe("ensureSkillsWatcher", () => { beforeEach(() => { watchMock.mockClear(); createdWatchers.length = 0; + pluginSkillsMocks.resolvePluginSkillDirs.mockClear(); + pluginSkillsMocks.resolvePluginSkillDirsFromMetadata.mockClear(); }); afterEach(async () => { @@ -534,6 +542,30 @@ describe("ensureSkillsWatcher", () => { } }); + it("reuses prepared plugin metadata when reconciling watch targets", () => { + const config = { skills: { load: {} } }; + const pluginMetadataSnapshot = { policyHash: "prepared" } as PluginMetadataSnapshot; + + refreshModule.ensureSkillsWatcher({ + workspaceDir: "/tmp/workspace", + config, + pluginMetadataSnapshot, + }); + refreshModule.ensureSkillsWatcher({ + workspaceDir: "/tmp/workspace", + config, + pluginMetadataSnapshot, + }); + + expect(pluginSkillsMocks.resolvePluginSkillDirs).not.toHaveBeenCalled(); + expect(pluginSkillsMocks.resolvePluginSkillDirsFromMetadata).toHaveBeenCalledTimes(2); + expect(pluginSkillsMocks.resolvePluginSkillDirsFromMetadata).toHaveBeenLastCalledWith({ + workspaceDir: "/tmp/workspace", + config, + metadataSnapshot: pluginMetadataSnapshot, + }); + }); + it("watches extra-dir roots and companion skills folders without resolving them", async () => { const repoDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-watch-pair-")); try { diff --git a/src/skills/runtime/refresh.ts b/src/skills/runtime/refresh.ts index a50154b5dc88..7c33d5b6a774 100644 --- a/src/skills/runtime/refresh.ts +++ b/src/skills/runtime/refresh.ts @@ -8,8 +8,12 @@ import { isDefaultStateDir } from "../../config/paths.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { isPathInside } from "../../infra/path-guards.js"; import { createSubsystemLogger } from "../../logging/subsystem.js"; +import type { PluginMetadataSnapshot } from "../../plugins/plugin-metadata-snapshot.types.js"; import { CONFIG_DIR, resolveUserPath } from "../../utils.js"; -import { resolvePluginSkillDirs } from "../loading/plugin-skills.js"; +import { + resolvePluginSkillDirs, + resolvePluginSkillDirsFromMetadata, +} from "../loading/plugin-skills.js"; import { resolveAllowedSkillSymlinkTargetRealPaths, tryRealpath, @@ -99,6 +103,7 @@ function resolveWatchTargets( config: OpenClawConfig | undefined, executionSkillsDir: string | undefined, watcherKey: string, + pluginMetadataSnapshot: PluginMetadataSnapshot | undefined, ): WatchTarget[] { const baseRoots: Array<{ path: string; source: string }> = []; if (workspaceDir.trim()) { @@ -123,7 +128,13 @@ function resolveWatchTargets( .map((d) => normalizeOptionalString(d) ?? "") .filter(Boolean) .map((dir) => resolveUserPath(dir)); - const pluginSkillDirs = resolvePluginSkillDirs({ workspaceDir, config }); + const pluginSkillDirs = pluginMetadataSnapshot + ? resolvePluginSkillDirsFromMetadata({ + workspaceDir, + config, + metadataSnapshot: pluginMetadataSnapshot, + }) + : resolvePluginSkillDirs({ workspaceDir, config }); const allowedSymlinkTargetRealPaths = resolveAllowedSkillSymlinkTargetRealPaths(config); const signature = JSON.stringify({ basePaths: baseRoots.map((root) => toWatchRoot(root.path)), @@ -668,6 +679,7 @@ export function ensureSkillsWatcher(params: { workspaceDir: string; executionSkillsDir?: string; config?: OpenClawConfig; + pluginMetadataSnapshot?: PluginMetadataSnapshot; }) { const workspaceDir = params.workspaceDir.trim(); if (!workspaceDir) { @@ -694,6 +706,7 @@ export function ensureSkillsWatcher(params: { params.config, params.executionSkillsDir, watcherKey, + params.pluginMetadataSnapshot, ); const targetsUnchanged = sameWatchTargets(previousTargets, watchTargets); const watcherDepthsCoverTargets = watchTargets.every( diff --git a/src/skills/runtime/session-snapshot.test.ts b/src/skills/runtime/session-snapshot.test.ts index 0714afdd646a..ad61b85f2d14 100644 --- a/src/skills/runtime/session-snapshot.test.ts +++ b/src/skills/runtime/session-snapshot.test.ts @@ -75,7 +75,7 @@ describe("resolveReusableWorkspaceSkillSnapshot", () => { ); }); - it("reuses prepared plugin metadata when loading execution-workspace skills", () => { + it("reuses prepared plugin metadata for watcher reconciliation and skill loading", () => { const pluginMetadataSnapshot = { policyHash: "prepared" } as PluginMetadataSnapshot; resolveReusableWorkspaceSkillSnapshot({ @@ -89,6 +89,9 @@ describe("resolveReusableWorkspaceSkillSnapshot", () => { expect(loadMergedWorkspaceSkillsMock.mock.calls[0]?.[0].pluginMetadataSnapshot).toBe( pluginMetadataSnapshot, ); + expect(ensureSkillsWatcherMock).toHaveBeenCalledWith( + expect.objectContaining({ pluginMetadataSnapshot }), + ); }); it("reuses cached resolvedSkills across calls with the same workspace, version, and filter", () => { diff --git a/src/skills/runtime/session-snapshot.ts b/src/skills/runtime/session-snapshot.ts index f133b5b29119..e3d00e7a2ee5 100644 --- a/src/skills/runtime/session-snapshot.ts +++ b/src/skills/runtime/session-snapshot.ts @@ -68,6 +68,9 @@ export function resolveReusableWorkspaceSkillSnapshot( workspaceDir: watcherWorkspaceDir, ...(skillRoots ? { executionSkillsDir: skillRoots.executionSkillsDir } : {}), config: params.config, + ...(params.pluginMetadataSnapshot + ? { pluginMetadataSnapshot: params.pluginMetadataSnapshot } + : {}), }); } const snapshotVersion = params.snapshotVersion ?? getSkillsSnapshotVersion(watcherWorkspaceDir);