From d8f705c0a2031495af6ca010147bd1b4e33bb1c2 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 14 Jul 2026 02:58:34 -0700 Subject: [PATCH] refactor(skills): trim internal exports (#107353) * refactor(skills): trim internal exports * test(skills): derive mismatch fixture type --- scripts/deadcode-exports.baseline.mjs | 18 -- src/gateway/server-maintenance.test.ts | 3 +- src/skills/discovery/chat-commands.test.ts | 44 +--- src/skills/discovery/chat-commands.ts | 4 - src/skills/discovery/filter.test.ts | 13 +- src/skills/discovery/filter.ts | 2 +- src/skills/discovery/skill-index.test.ts | 6 - src/skills/discovery/skill-index.ts | 6 +- .../lifecycle/gh-config-discovery.test.ts | 6 +- src/skills/lifecycle/gh-config-discovery.ts | 2 +- src/skills/loading/plugin-skills.test.ts | 57 +++-- src/skills/loading/plugin-skills.ts | 6 - src/skills/runtime/refresh.test.ts | 55 +++-- src/skills/runtime/refresh.ts | 4 +- src/skills/runtime/session-snapshot.test.ts | 8 +- src/skills/runtime/session-snapshot.ts | 4 - src/skills/workshop/curator.test.ts | 216 ++++++++++++------ src/skills/workshop/curator.ts | 16 +- 18 files changed, 238 insertions(+), 232 deletions(-) diff --git a/scripts/deadcode-exports.baseline.mjs b/scripts/deadcode-exports.baseline.mjs index 07ce7f06736c..70b9a3d58b34 100644 --- a/scripts/deadcode-exports.baseline.mjs +++ b/scripts/deadcode-exports.baseline.mjs @@ -734,29 +734,11 @@ export const KNIP_UNUSED_EXPORT_BASELINE = [ "src/sessions/session-upstream-monitor.ts: runSessionUpstreamMonitorTick", "src/sessions/session-upstream-monitor.ts: SessionUpstreamMonitor", "src/sessions/user-turn-transcript.ts: persistUserTurnTranscript", - "src/skills/discovery/chat-commands.ts: testing", - "src/skills/discovery/filter.ts: normalizeSkillFilterForComparison", - "src/skills/discovery/skill-index.ts: isSkillPromptVisible", - "src/skills/discovery/skill-index.ts: isSkillRuntimeVisible", - "src/skills/discovery/skill-index.ts: isSkillUserInvocable", - "src/skills/lifecycle/gh-config-discovery.ts: GhConfigDirMismatch", "src/skills/lifecycle/install.ts: testing", "src/skills/lifecycle/upload-store.ts: createSkillUploadStore", "src/skills/lifecycle/upload-store.ts: MAX_ACTIVE_SKILL_UPLOADS", - "src/skills/loading/plugin-skills.ts: testing", - "src/skills/runtime/refresh.ts: bumpSkillsSnapshotVersion", "src/skills/runtime/refresh.ts: resetSkillsRefreshForTest", - "src/skills/runtime/refresh.ts: shouldIgnoreSkillsWatchPath", "src/skills/runtime/remote-skills.ts: resetRemoteNodeSkillsForTests", - "src/skills/runtime/session-snapshot.ts: resetResolvedSkillsCacheForTests", - "src/skills/workshop/curator.ts: ARCHIVE_AFTER_MS", - "src/skills/workshop/curator.ts: CURATOR_INITIAL_DELAY_MS", - "src/skills/workshop/curator.ts: CURATOR_SWEEP_INTERVAL_MS", - "src/skills/workshop/curator.ts: DOCTOR_WEDGED_AFTER_MS", - "src/skills/workshop/curator.ts: recordSkillUsage", - "src/skills/workshop/curator.ts: registerSkillUsageTracking", - "src/skills/workshop/curator.ts: runSkillCuratorSweep", - "src/skills/workshop/curator.ts: STALE_AFTER_MS", "src/status/status-text.ts: resolveStatusChannelFeatureLine", "src/talk/agent-consult-runtime.ts: setRealtimeVoiceAgentConsultDepsForTest", "src/tasks/detached-task-runtime-state.ts: DetachedTaskLifecycleRuntime", diff --git a/src/gateway/server-maintenance.test.ts b/src/gateway/server-maintenance.test.ts index a2c11fdb7d22..abcd889be151 100644 --- a/src/gateway/server-maintenance.test.ts +++ b/src/gateway/server-maintenance.test.ts @@ -3,7 +3,8 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { managedWorktrees } from "../agents/worktrees/service.js"; import type { HealthSummary } from "../commands/health.js"; -import { CURATOR_INITIAL_DELAY_MS, CURATOR_SWEEP_INTERVAL_MS } from "../skills/workshop/curator.js"; +const CURATOR_INITIAL_DELAY_MS = 5 * 60_000; +const CURATOR_SWEEP_INTERVAL_MS = 24 * 60 * 60_000; import type { ChatAbortControllerEntry } from "./chat-abort.js"; import { DEDUPE_MAX, DEDUPE_TTL_MS } from "./server-constants.js"; import { pendingChatSendDedupeKey } from "./server-shared.js"; diff --git a/src/skills/discovery/chat-commands.test.ts b/src/skills/discovery/chat-commands.test.ts index 358298a8bf94..5264908f6ab5 100644 --- a/src/skills/discovery/chat-commands.test.ts +++ b/src/skills/discovery/chat-commands.test.ts @@ -7,7 +7,6 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vites let listSkillCommandsForAgents: typeof import("./chat-commands.js").listSkillCommandsForAgents; let listSkillCommandsForWorkspace: typeof import("./chat-commands.js").listSkillCommandsForWorkspace; let resolveSkillCommandInvocation: typeof import("./chat-commands.js").resolveSkillCommandInvocation; -let skillCommandsTesting: typeof import("./chat-commands.js").testing; const tempDirs: string[] = []; const resolveNodeExecEligibilityMock = vi.hoisted(() => @@ -162,12 +161,8 @@ vi.mock("./agent-filter.js", () => ({ })); beforeAll(async () => { - ({ - listSkillCommandsForAgents, - listSkillCommandsForWorkspace, - resolveSkillCommandInvocation, - testing: skillCommandsTesting, - } = await import("./chat-commands.js")); + ({ listSkillCommandsForAgents, listSkillCommandsForWorkspace, resolveSkillCommandInvocation } = + await import("./chat-commands.js")); }); afterAll(async () => { @@ -457,38 +452,3 @@ describe("listSkillCommandsForWorkspace", () => { ); }); }); - -describe("dedupeBySkillName", () => { - it("keeps the first entry when multiple commands share a skillName", () => { - const input = [ - { name: "github", skillName: "github", description: "GitHub" }, - { name: "github_2", skillName: "github", description: "GitHub" }, - { name: "weather", skillName: "weather", description: "Weather" }, - { name: "weather_2", skillName: "weather", description: "Weather" }, - ]; - const output = skillCommandsTesting.dedupeBySkillName(input); - expect(output.map((e) => e.name)).toEqual(["github", "weather"]); - }); - - it("matches skillName case-insensitively", () => { - const input = [ - { name: "ClawHub", skillName: "ClawHub", description: "ClawHub" }, - { name: "clawhub_2", skillName: "clawhub", description: "ClawHub" }, - ]; - const output = skillCommandsTesting.dedupeBySkillName(input); - expect(output).toHaveLength(1); - expect(output[0]?.name).toBe("ClawHub"); - }); - - it("passes through commands with an empty skillName", () => { - const input = [ - { name: "a", skillName: "", description: "A" }, - { name: "b", skillName: "", description: "B" }, - ]; - expect(skillCommandsTesting.dedupeBySkillName(input)).toHaveLength(2); - }); - - it("returns an empty array for empty input", () => { - expect(skillCommandsTesting.dedupeBySkillName([])).toStrictEqual([]); - }); -}); diff --git a/src/skills/discovery/chat-commands.ts b/src/skills/discovery/chat-commands.ts index c6a97c79227c..ccf93c8285a5 100644 --- a/src/skills/discovery/chat-commands.ts +++ b/src/skills/discovery/chat-commands.ts @@ -136,7 +136,3 @@ export function listSkillCommandsForAgents(params: { left.skillName.localeCompare(right.skillName, "en"), ); } - -export const testing = { - dedupeBySkillName, -}; diff --git a/src/skills/discovery/filter.test.ts b/src/skills/discovery/filter.test.ts index 6265f75e9881..cd66c627c77c 100644 --- a/src/skills/discovery/filter.test.ts +++ b/src/skills/discovery/filter.test.ts @@ -1,10 +1,6 @@ // Skill filter tests cover allowlist and agent-scoped skill selection behavior. import { describe, expect, it } from "vitest"; -import { - matchesSkillFilter, - normalizeSkillFilter, - normalizeSkillFilterForComparison, -} from "./filter.js"; +import { matchesSkillFilter, normalizeSkillFilter } from "./filter.js"; describe("skills/filter", () => { it("normalizes configured filters with trimming", () => { @@ -19,13 +15,6 @@ describe("skills/filter", () => { expect(normalizeSkillFilter(undefined)).toBeUndefined(); }); - it("normalizes for comparison with dedupe + ordering", () => { - expect(normalizeSkillFilterForComparison(["weather", "meme-factory", "weather"])).toEqual([ - "meme-factory", - "weather", - ]); - }); - it("matches equivalent filters after normalization", () => { expect(matchesSkillFilter(["weather", "meme-factory"], [" meme-factory ", "weather"])).toBe( true, diff --git a/src/skills/discovery/filter.ts b/src/skills/discovery/filter.ts index 7561ec5e13e5..d6e1ac60d47e 100644 --- a/src/skills/discovery/filter.ts +++ b/src/skills/discovery/filter.ts @@ -12,7 +12,7 @@ export function normalizeSkillFilter(skillFilter?: ReadonlyArray): stri return normalizeStringEntries(skillFilter); } -export function normalizeSkillFilterForComparison( +function normalizeSkillFilterForComparison( skillFilter?: ReadonlyArray, ): string[] | undefined { const normalized = normalizeSkillFilter(skillFilter); diff --git a/src/skills/discovery/skill-index.test.ts b/src/skills/discovery/skill-index.test.ts index c5c4ac13dea5..4542fc9e7b00 100644 --- a/src/skills/discovery/skill-index.test.ts +++ b/src/skills/discovery/skill-index.test.ts @@ -5,9 +5,6 @@ import { buildSkillIndexEntries, filterPromptVisibleSkillEntries, filterUserInvocableSkillEntries, - isSkillPromptVisible, - isSkillRuntimeVisible, - isSkillUserInvocable, normalizeSkillIndexName, } from "./skill-index.js"; @@ -77,9 +74,6 @@ describe("skill index", () => { promptHidden, legacyPromptHidden, ]); - expect(isSkillRuntimeVisible(runtimeHidden)).toBe(false); - expect(isSkillPromptVisible(legacyPromptHidden)).toBe(false); - expect(isSkillUserInvocable(commandHidden)).toBe(false); }); it("records source, bundled state, skill key, and agent filter state", () => { diff --git a/src/skills/discovery/skill-index.ts b/src/skills/discovery/skill-index.ts index 940553ef767c..8fbf081d54f0 100644 --- a/src/skills/discovery/skill-index.ts +++ b/src/skills/discovery/skill-index.ts @@ -34,11 +34,11 @@ export function normalizeSkillIndexName(value: string): string { .replace(/^-+|-+$/g, ""); } -export function isSkillRuntimeVisible(entry: SkillEntry): boolean { +function isSkillRuntimeVisible(entry: SkillEntry): boolean { return entry.exposure?.includeInRuntimeRegistry ?? true; } -export function isSkillPromptVisible(entry: SkillEntry): boolean { +function isSkillPromptVisible(entry: SkillEntry): boolean { if (entry.exposure) { return entry.exposure.includeInAvailableSkillsPrompt ?? true; } @@ -48,7 +48,7 @@ export function isSkillPromptVisible(entry: SkillEntry): boolean { return !entry.skill.disableModelInvocation; } -export function isSkillUserInvocable(entry: SkillEntry): boolean { +function isSkillUserInvocable(entry: SkillEntry): boolean { if (entry.exposure) { return entry.exposure.userInvocable ?? true; } diff --git a/src/skills/lifecycle/gh-config-discovery.test.ts b/src/skills/lifecycle/gh-config-discovery.test.ts index b4602647d971..79c016a52948 100644 --- a/src/skills/lifecycle/gh-config-discovery.test.ts +++ b/src/skills/lifecycle/gh-config-discovery.test.ts @@ -3,10 +3,14 @@ import { describe, expect, it } from "vitest"; import { detectGhConfigDirMismatch, formatGhConfigDirMismatchHint, - type GhConfigDirMismatch, type GhConfigDiscoveryInput, } from "./gh-config-discovery.js"; +type GhConfigDirMismatch = Omit< + Extract, { kind: "mismatch" }>, + "kind" +>; + function makeInput(overrides: Partial): GhConfigDiscoveryInput { return { platform: "linux", diff --git a/src/skills/lifecycle/gh-config-discovery.ts b/src/skills/lifecycle/gh-config-discovery.ts index 063d43cc690c..e6a45aa0d108 100644 --- a/src/skills/lifecycle/gh-config-discovery.ts +++ b/src/skills/lifecycle/gh-config-discovery.ts @@ -32,7 +32,7 @@ export type GhConfigDiscoveryInput = { candidateOperatorHomes?: readonly string[]; }; -export type GhConfigDirMismatch = { +type GhConfigDirMismatch = { // The directory `gh` would actually consult given the current process env. effectiveConfigDir: string; // The directory that contains the operator's real `hosts.yml`. diff --git a/src/skills/loading/plugin-skills.test.ts b/src/skills/loading/plugin-skills.test.ts index 4ce0de7d83dc..f71aa9065137 100644 --- a/src/skills/loading/plugin-skills.test.ts +++ b/src/skills/loading/plugin-skills.test.ts @@ -1,5 +1,5 @@ // Plugin skill loading tests cover skill discovery from plugin-provided skill bundles. -import fsSync, { type Dirent } from "node:fs"; +import fsSync from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; @@ -10,7 +10,6 @@ import { import type { OpenClawConfig } from "../../config/config.js"; import type { PluginManifestRegistry } from "../../plugins/manifest-registry.js"; import { createTrackedTempDirs } from "../../test-utils/tracked-temp-dirs.js"; -import { testing } from "./plugin-skills.js"; const hoisted = vi.hoisted(() => { const loadManifestRegistry = vi.fn(); @@ -393,7 +392,38 @@ describe("resolvePluginSkillDirs", () => { }); describe("publishPluginSkills", () => { - const { isGeneratedPluginSkillEntry, publishPluginSkills, resolvePluginSkillLinkType } = testing; + beforeAll(async () => { + ({ resolvePluginSkillDirs } = await import("./plugin-skills.js")); + }); + + function publishPluginSkills(skillDirs: string[], opts: { pluginSkillsDir: string }): void { + const plugins = skillDirs.map((rootDir, index) => ({ + id: `publish-test-${index}`, + name: `Publish Test ${index}`, + channels: [], + providers: [], + cliBackends: [], + skills: ["."], + hooks: [], + origin: "workspace" as const, + rootDir, + source: rootDir, + manifestPath: path.join(rootDir, "openclaw.plugin.json"), + })); + hoisted.loadPluginManifestRegistryForInstalledIndex.mockReturnValue({ + diagnostics: [], + plugins, + }); + resolvePluginSkillDirs({ + workspaceDir: opts.pluginSkillsDir, + pluginSkillsDir: opts.pluginSkillsDir, + config: { + plugins: { + entries: Object.fromEntries(plugins.map((plugin) => [plugin.id, { enabled: true }])), + }, + }, + }); + } function withPlatform(platform: NodeJS.Platform, fn: () => T): T { const originalPlatform = process.platform; @@ -436,12 +466,6 @@ describe("publishPluginSkills", () => { expect(fsSync.readlinkSync(linkB)).toBe(dirB); }); - it("uses junction links for plugin skill directories on Windows", () => { - expect(resolvePluginSkillLinkType("win32")).toBe("junction"); - expect(resolvePluginSkillLinkType("linux")).toBe("dir"); - expect(resolvePluginSkillLinkType("darwin")).toBe("dir"); - }); - it("is idempotent: skips symlinks that already point to the same target", async () => { const skillParent = await tempDirs.make("plugin-skills-"); const managedDir = await tempDirs.make("managed-skills-"); @@ -538,21 +562,6 @@ describe("publishPluginSkills", () => { expect(fsSync.existsSync(staleDir)).toBe(false); }); - it("treats Windows directory entries as generated plugin skill entries", () => { - const directoryEntry = { - isDirectory: () => true, - isSymbolicLink: () => false, - } as Dirent; - const regularEntry = { - isDirectory: () => false, - isSymbolicLink: () => false, - } as Dirent; - - expect(withPlatform("win32", () => isGeneratedPluginSkillEntry(directoryEntry))).toBe(true); - expect(withPlatform("linux", () => isGeneratedPluginSkillEntry(directoryEntry))).toBe(false); - expect(withPlatform("win32", () => isGeneratedPluginSkillEntry(regularEntry))).toBe(false); - }); - it("cleans up broken symlinks (dangling)", async () => { const skillParent = await tempDirs.make("plugin-skills-"); const managedDir = await tempDirs.make("managed-skills-"); diff --git a/src/skills/loading/plugin-skills.ts b/src/skills/loading/plugin-skills.ts index 74f4d258bb92..a42b5425f333 100644 --- a/src/skills/loading/plugin-skills.ts +++ b/src/skills/loading/plugin-skills.ts @@ -297,9 +297,3 @@ function isNotFoundError(err: unknown): boolean { const code = (err as Record).code; return code === "ENOENT" || code === "ENOTDIR"; } - -export const testing = { - isGeneratedPluginSkillEntry, - publishPluginSkills, - resolvePluginSkillLinkType, -}; diff --git a/src/skills/runtime/refresh.test.ts b/src/skills/runtime/refresh.test.ts index 360230f997b0..d037640966b0 100644 --- a/src/skills/runtime/refresh.test.ts +++ b/src/skills/runtime/refresh.test.ts @@ -4,6 +4,7 @@ import os from "node:os"; import path from "node:path"; import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { + bumpSkillsSnapshotVersion, getSkillsSnapshotVersion, shouldRefreshSnapshotForVersion, type SkillsChangeEvent, @@ -68,7 +69,17 @@ describe("ensureSkillsWatcher", () => { // Each unique directory gets its own watcher (one path argument per call). const calls = watchMock.mock.calls as unknown as Array< - [string, { depth?: number; followSymlinks?: boolean; ignored?: unknown }] + [ + string, + { + depth?: number; + followSymlinks?: boolean; + ignored?: ( + watchPath: string, + stats?: { isDirectory?: () => boolean; isSymbolicLink?: () => boolean }, + ) => boolean; + }, + ] >; expect(calls.length).toBeGreaterThan(0); const targets = calls.map((call) => call[0]); @@ -86,33 +97,33 @@ describe("ensureSkillsWatcher", () => { expect(targets).toContain(posix(path.join(os.homedir(), ".agents", "skills"))); const wildcardTargets = targets.filter((target) => target.includes("*")); expect(wildcardTargets).toStrictEqual([]); - const ignored = refreshModule.shouldIgnoreSkillsWatchPath; + const ignored = opts.ignored; + expect(ignored).toBeDefined(); // Node/JS paths - expect(ignored("/tmp/workspace/skills/node_modules/pkg/index.js")).toBe(true); - expect(ignored("/tmp/workspace/skills/dist/index.js")).toBe(true); - expect(ignored("/tmp/workspace/skills/.git/config")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/node_modules/pkg/index.js")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/dist/index.js")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/.git/config")).toBe(true); // Python virtual environments and caches - expect(ignored("/tmp/workspace/skills/scripts/.venv/bin/python")).toBe(true); - expect(ignored("/tmp/workspace/skills/venv/lib/python3.10/site.py")).toBe(true); - expect(ignored("/tmp/workspace/skills/__pycache__/module.pyc")).toBe(true); - expect(ignored("/tmp/workspace/skills/.mypy_cache/3.10/foo.json")).toBe(true); - expect(ignored("/tmp/workspace/skills/.pytest_cache/v/cache")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/scripts/.venv/bin/python")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/venv/lib/python3.10/site.py")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/__pycache__/module.pyc")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/.mypy_cache/3.10/foo.json")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/.pytest_cache/v/cache")).toBe(true); // Build artifacts and caches - expect(ignored("/tmp/workspace/skills/build/output.js")).toBe(true); - expect(ignored("/tmp/workspace/skills/.cache/data.json")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/build/output.js")).toBe(true); + expect(ignored?.("/tmp/workspace/skills/.cache/data.json")).toBe(true); // Paths without stats stay visible so chokidar can stat and classify them. - expect(ignored("/tmp/.hidden/skills/index.md")).toBe(false); - expect(ignored("/tmp/workspace/skills/my-skill", { isDirectory: () => true })).toBe(false); - expect(ignored("/tmp/workspace/skills/my-skill", { isSymbolicLink: () => true })).toBe(false); - expect(ignored("/tmp/workspace/skills/my-skill/README.md", {})).toBe(true); - expect(ignored("/tmp/workspace/skills/my-skill/SKILL.md", {})).toBe(true); - expect(ignored("/tmp/workspace/skills/my-skill/SKILL.md", {}, { usePolling: true })).toBe( + expect(ignored?.("/tmp/.hidden/skills/index.md")).toBe(false); + expect(ignored?.("/tmp/workspace/skills/my-skill", { isDirectory: () => true })).toBe(false); + expect(ignored?.("/tmp/workspace/skills/my-skill", { isSymbolicLink: () => true })).toBe( false, ); + expect(ignored?.("/tmp/workspace/skills/my-skill/README.md", {})).toBe(true); + expect(ignored?.("/tmp/workspace/skills/my-skill/SKILL.md", {})).toBe(true); } finally { await fs.rm(workspaceDir, { recursive: true, force: true }); } @@ -856,7 +867,7 @@ describe("ensureSkillsWatcher", () => { config: { skills: { load: { watchDebounceMs: 10 } } }, }); - const firstVersion = refreshModule.bumpSkillsSnapshotVersion({ + const firstVersion = bumpSkillsSnapshotVersion({ workspaceDir, reason: "watch", changedPath: `${workspaceDir}/skills/demo/SKILL.md`, @@ -870,7 +881,7 @@ describe("ensureSkillsWatcher", () => { expect(nextVersion).toBeGreaterThan(firstVersion); expect(shouldRefreshSnapshotForVersion(firstVersion, nextVersion)).toBe(true); vi.setSystemTime(new Date(nextVersion)); - const followupVersion = refreshModule.bumpSkillsSnapshotVersion({ + const followupVersion = bumpSkillsSnapshotVersion({ workspaceDir, reason: "watch", }); @@ -890,7 +901,7 @@ describe("ensureSkillsWatcher", () => { (target) => target === `${idleWorkspaceDir}/skills`, ); expect(idleSkillsIndex).toBeGreaterThanOrEqual(0); - const firstVersion = refreshModule.bumpSkillsSnapshotVersion({ + const firstVersion = bumpSkillsSnapshotVersion({ workspaceDir: idleWorkspaceDir, reason: "watch", }); @@ -906,7 +917,7 @@ describe("ensureSkillsWatcher", () => { expect(evictedVersion).toBeGreaterThan(firstVersion); expect(shouldRefreshSnapshotForVersion(firstVersion, evictedVersion)).toBe(true); vi.setSystemTime(new Date(evictedVersion)); - const followupVersion = refreshModule.bumpSkillsSnapshotVersion({ + const followupVersion = bumpSkillsSnapshotVersion({ workspaceDir: idleWorkspaceDir, }); expect(followupVersion).toBeGreaterThan(evictedVersion); diff --git a/src/skills/runtime/refresh.ts b/src/skills/runtime/refresh.ts index bbb7d1fd52bb..1eea952dea08 100644 --- a/src/skills/runtime/refresh.ts +++ b/src/skills/runtime/refresh.ts @@ -18,7 +18,7 @@ import { resetSkillsRefreshStateForTest, setSkillsChangeListenerErrorHandler, } from "./refresh-state.js"; -export { bumpSkillsSnapshotVersion, registerSkillsChangeListener } from "./refresh-state.js"; +export { registerSkillsChangeListener } from "./refresh-state.js"; type SkillsPathWatchState = { watcher: FSWatcher; @@ -374,7 +374,7 @@ function isPathInsideAnyRoot(roots: readonly string[], child: string): boolean { return roots.some((root) => isPathInside(root, child)); } -export function shouldIgnoreSkillsWatchPath( +function shouldIgnoreSkillsWatchPath( watchPath: string, stats?: { isDirectory?: () => boolean; isSymbolicLink?: () => boolean }, options: { usePolling?: boolean } = {}, diff --git a/src/skills/runtime/session-snapshot.test.ts b/src/skills/runtime/session-snapshot.test.ts index dcbb04511710..1fbd5bedd233 100644 --- a/src/skills/runtime/session-snapshot.test.ts +++ b/src/skills/runtime/session-snapshot.test.ts @@ -47,13 +47,13 @@ vi.mock("./refresh-state.js", () => ({ shouldRefreshSnapshotForVersion: shouldRefreshSnapshotForVersionMock, })); -const { resolveReusableWorkspaceSkillSnapshot, resetResolvedSkillsCacheForTests } = - await import("./session-snapshot.js"); +let resolveReusableWorkspaceSkillSnapshot: typeof import("./session-snapshot.js").resolveReusableWorkspaceSkillSnapshot; describe("resolveReusableWorkspaceSkillSnapshot", () => { - beforeEach(() => { + beforeEach(async () => { + vi.resetModules(); + ({ resolveReusableWorkspaceSkillSnapshot } = await import("./session-snapshot.js")); vi.clearAllMocks(); - resetResolvedSkillsCacheForTests(); buildWorkspaceSkillSnapshotMock.mockReturnValue({ prompt: "", skills: [], resolvedSkills: [] }); ensureSkillsWatcherMock.mockImplementation(() => undefined); getSkillsSnapshotVersionMock.mockReturnValue(1); diff --git a/src/skills/runtime/session-snapshot.ts b/src/skills/runtime/session-snapshot.ts index e6550b3a61b1..1bcfc6fc82b6 100644 --- a/src/skills/runtime/session-snapshot.ts +++ b/src/skills/runtime/session-snapshot.ts @@ -33,10 +33,6 @@ type ReusableSkillSnapshotResult = { snapshotVersion: number; }; -export function resetResolvedSkillsCacheForTests(): void { - resolvedSkillsCache.clear(); -} - function fingerprintSkillSnapshotConfig(config: OpenClawConfig): string { return crypto .createHash("sha256") diff --git a/src/skills/workshop/curator.test.ts b/src/skills/workshop/curator.test.ts index a41c2592b6a1..9707f25c9855 100644 --- a/src/skills/workshop/curator.test.ts +++ b/src/skills/workshop/curator.test.ts @@ -33,23 +33,93 @@ vi.mock("./store.js", () => ({ })); import { - ARCHIVE_AFTER_MS, - DOCTOR_WEDGED_AFTER_MS, - STALE_AFTER_MS, getSkillCuratorDoctorWarning, getSkillCuratorStatus, pinCuratedSkill, - recordSkillUsage, - registerSkillUsageTracking, restoreCuratedSkill, - runSkillCuratorSweep, + startSkillCuratorMaintenance, unpinCuratedSkill, } from "./curator.js"; +const STALE_AFTER_MS = 30 * 24 * 60 * 60_000; +const ARCHIVE_AFTER_MS = 90 * 24 * 60 * 60_000; +const CURATOR_INITIAL_DELAY_MS = 5 * 60_000; +const DOCTOR_WEDGED_AFTER_MS = 7 * 24 * 60 * 60_000; + let rootDir = ""; let stateDir = ""; let originalStateDir: string | undefined; +function registerSkillUsageTracking(): () => void { + return startSkillCuratorMaintenance({ + onError: (error) => { + throw error; + }, + runSweep: async () => undefined, + }); +} + +async function recordSkillUsage(event: { + skillFile: string; + skillName: string; + skillSource: "bundled" | "unknown" | "workspace"; + agentId?: string; + ts: number; +}): Promise { + const cleanup = registerSkillUsageTracking(); + const now = vi.spyOn(Date, "now").mockReturnValue(event.ts); + try { + emitTrustedSkillUsedDiagnosticEvent( + { + type: "skill.used", + skillName: event.skillName, + skillSource: event.skillSource, + activation: "read", + agentId: event.agentId, + }, + { skillUsage: { skillFile: event.skillFile } }, + ); + } finally { + now.mockRestore(); + } + await waitForDiagnosticEventsDrained(); + cleanup(); +} + +async function runSkillCuratorSweep(options: { + env?: NodeJS.ProcessEnv; + nowMs?: number; +}): Promise { + const nowMs = options.nowMs ?? Date.now(); + vi.useFakeTimers(); + vi.setSystemTime(nowMs - CURATOR_INITIAL_DELAY_MS); + let failure: unknown; + const cleanup = startSkillCuratorMaintenance({ + onError: (error) => { + failure = error; + }, + registerUsageTracking: () => () => undefined, + }); + try { + await vi.advanceTimersByTimeAsync(CURATOR_INITIAL_DELAY_MS); + for (let attempt = 0; attempt < 50; attempt += 1) { + const status = getSkillCuratorStatus({ env: process.env }); + if (failure || status.lastSuccessAtMs === nowMs || status.lastError) { + break; + } + await Promise.resolve(); + } + } finally { + cleanup(); + vi.useRealTimers(); + } + if (failure) { + throw failure instanceof Error + ? failure + : new Error("skill curator sweep failed", { cause: failure }); + } +} + function addAppliedSkill(params: { name: string; appliedAtMs: number; @@ -152,7 +222,7 @@ describe("skill curator usage", () => { const nowMs = Date.now(); const skillFile = path.join(rootDir, "agent", "skills", "daily-brief", "SKILL.md"); addAppliedSkill({ name: "Daily Brief", appliedAtMs: nowMs }); - const unregister = registerSkillUsageTracking({ env: process.env }); + const unregister = registerSkillUsageTracking(); emitTrustedSkillUsedDiagnosticEvent( { type: "skill.used", @@ -187,7 +257,7 @@ describe("skill curator usage", () => { it("skips usage events without a canonical skill file", async () => { const nowMs = Date.now(); addAppliedSkill({ name: "Nameless Usage", appliedAtMs: nowMs }); - const unregister = registerSkillUsageTracking({ env: process.env }); + const unregister = registerSkillUsageTracking(); emitTrustedSkillUsedDiagnosticEvent({ type: "skill.used", skillName: "Nameless Usage", @@ -207,26 +277,20 @@ describe("skill curator usage", () => { it("keeps last-used time monotonic when events arrive out of order", async () => { const skillFile = path.join(rootDir, "agent", "skills", "ordered", "SKILL.md"); addAppliedSkill({ name: "Ordered", appliedAtMs: 0 }); - recordSkillUsage( - { - skillFile, - skillName: "Ordered", - skillSource: "workspace", - agentId: "newer", - ts: 200, - }, - { env: process.env }, - ); - recordSkillUsage( - { - skillFile, - skillName: "Ordered", - skillSource: "workspace", - agentId: "older", - ts: 100, - }, - { env: process.env }, - ); + await recordSkillUsage({ + skillFile, + skillName: "Ordered", + skillSource: "workspace", + agentId: "newer", + ts: 200, + }); + await recordSkillUsage({ + skillFile, + skillName: "Ordered", + skillSource: "workspace", + agentId: "older", + ts: 100, + }); await runSkillCuratorSweep({ env: process.env, nowMs: 201 }); expect(getSkillCuratorStatus({ env: process.env }).skills[0]).toMatchObject({ @@ -236,7 +300,7 @@ describe("skill curator usage", () => { }); it("contains subscriber failures without throwing into the emitter", async () => { - const unregister = registerSkillUsageTracking({ env: process.env }); + const unregister = registerSkillUsageTracking(); expect(() => emitTrustedSkillUsedDiagnosticEvent( { @@ -288,26 +352,20 @@ describe("skill curator lifecycle", () => { addAppliedSkill({ name: "Unused Archive", appliedAtMs: nowMs - ARCHIVE_AFTER_MS - 1 }); await runSkillCuratorSweep({ env: process.env, nowMs }); - recordSkillUsage( - { - skillFile: path.join(rootDir, "agent", "skills", "dormant", "SKILL.md"), - skillName: "Dormant", - skillSource: "workspace", - agentId: "main", - ts: nowMs + 1, - }, - { env: process.env }, - ); - recordSkillUsage( - { - skillFile: path.join(rootDir, "agent", "skills", "deep-archive", "SKILL.md"), - skillName: "Deep Archive", - skillSource: "workspace", - agentId: "main", - ts: nowMs + 1, - }, - { env: process.env }, - ); + await recordSkillUsage({ + skillFile: path.join(rootDir, "agent", "skills", "dormant", "SKILL.md"), + skillName: "Dormant", + skillSource: "workspace", + agentId: "main", + ts: nowMs + 1, + }); + await recordSkillUsage({ + skillFile: path.join(rootDir, "agent", "skills", "deep-archive", "SKILL.md"), + skillName: "Deep Archive", + skillSource: "workspace", + agentId: "main", + ts: nowMs + 1, + }); await runSkillCuratorSweep({ env: process.env, nowMs: nowMs + 2 }); const byKey = new Map( @@ -352,16 +410,13 @@ describe("skill curator lifecycle", () => { proposalId: "shared-name-b", agentDirName: "agent-b", }); - recordSkillUsage( - { - skillFile: firstSkillFile, - skillName: "Shared Name", - skillSource: "workspace", - agentId: "agent-a", - ts: nowMs, - }, - { env: process.env }, - ); + await recordSkillUsage({ + skillFile: firstSkillFile, + skillName: "Shared Name", + skillSource: "workspace", + agentId: "agent-a", + ts: nowMs, + }); await runSkillCuratorSweep({ env: process.env, nowMs }); const status = getSkillCuratorStatus({ env: process.env }); @@ -498,21 +553,36 @@ describe("skill curator lifecycle", () => { resolveManifest = resolve; }), ); - const sweep = runSkillCuratorSweep({ env: process.env, nowMs }); - - expect( - getSkillCuratorDoctorWarning({ - env: process.env, - nowMs: nowMs + DOCTOR_WEDGED_AFTER_MS + 1, - }), - ).toContain("skill curator has not completed a sweep"); - - resolveManifest?.({ - schema: "openclaw.skill-workshop.proposals-manifest.v1", - updatedAt: new Date(nowMs).toISOString(), - proposals: [], + vi.useFakeTimers(); + vi.setSystemTime(nowMs - CURATOR_INITIAL_DELAY_MS); + let failure: unknown; + const cleanup = startSkillCuratorMaintenance({ + onError: (error) => { + failure = error; + }, + registerUsageTracking: () => () => undefined, }); - await sweep; + try { + vi.advanceTimersByTime(CURATOR_INITIAL_DELAY_MS); + await Promise.resolve(); + expect( + getSkillCuratorDoctorWarning({ + env: process.env, + nowMs: nowMs + DOCTOR_WEDGED_AFTER_MS + 1, + }), + ).toContain("skill curator has not completed a sweep"); + + resolveManifest?.({ + schema: "openclaw.skill-workshop.proposals-manifest.v1", + updatedAt: new Date(nowMs).toISOString(), + proposals: [], + }); + await vi.advanceTimersByTimeAsync(0); + expect(failure).toBeUndefined(); + } finally { + cleanup(); + vi.useRealTimers(); + } }); it("filters archived skills from snapshots while retaining stale skills", async () => { diff --git a/src/skills/workshop/curator.ts b/src/skills/workshop/curator.ts index 7ee3ec5cb18f..d9b9444de89d 100644 --- a/src/skills/workshop/curator.ts +++ b/src/skills/workshop/curator.ts @@ -20,11 +20,11 @@ import { readSkillProposalManifest, readSkillProposalRecord } from "./store.js"; import type { SkillProposalRecord } from "./types.js"; // Fixed policy keeps lifecycle behavior predictable and avoids another config surface. -export const STALE_AFTER_MS = 30 * 24 * 60 * 60_000; -export const ARCHIVE_AFTER_MS = 90 * 24 * 60 * 60_000; -export const CURATOR_SWEEP_INTERVAL_MS = 24 * 60 * 60_000; -export const CURATOR_INITIAL_DELAY_MS = 5 * 60_000; -export const DOCTOR_WEDGED_AFTER_MS = 7 * 24 * 60 * 60_000; +const STALE_AFTER_MS = 30 * 24 * 60 * 60_000; +const ARCHIVE_AFTER_MS = 90 * 24 * 60 * 60_000; +const CURATOR_SWEEP_INTERVAL_MS = 24 * 60 * 60_000; +const CURATOR_INITIAL_DELAY_MS = 5 * 60_000; +const DOCTOR_WEDGED_AFTER_MS = 7 * 24 * 60 * 60_000; const log = createSubsystemLogger("skills/curator"); const CURATOR_STATE_ID = 1; @@ -102,7 +102,7 @@ function canonicalSkillKey(name: string): string { return key; } -export function recordSkillUsage( +function recordSkillUsage( event: Pick & { skillFile?: string; }, @@ -154,7 +154,7 @@ export function recordSkillUsage( } /** Register once per Gateway lifetime; listener failures never reach tool execution. */ -export function registerSkillUsageTracking(options: OpenClawStateDatabaseOptions = {}): () => void { +function registerSkillUsageTracking(options: OpenClawStateDatabaseOptions = {}): () => void { return onTrustedInternalDiagnosticEvent((event, metadata, privateData) => { if (!metadata.trusted || event.type !== "skill.used") { return; @@ -357,7 +357,7 @@ function writeSweepFailure( }, options); } -export async function runSkillCuratorSweep( +async function runSkillCuratorSweep( options: CuratorOptions = {}, ): Promise { const nowMs = options.nowMs ?? Date.now();