mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-20 06:20:55 +00:00
* refactor(skills): use explicit skill-command scope APIs * test(skills): cover scoped listing and telegram allowlist * fix(skills): add mergeSkillFilters edge-case tests and simplify dead code Cover unrestricted-co-tenant and empty-allowlist merge paths in skill-commands tests. Remove dead ternary in bot-handlers pagination. Add clarifying comments on undefined vs [] filter semantics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(skills): collapse scope functions into single listSkillCommandsForAgents Replace listSkillCommandsForAgentIds, listSkillCommandsForAllAgents, and the deprecated listSkillCommandsForAgents with a single function that accepts optional agentIds and falls back to all agents when omitted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(skills): harden realpathSync race and add missing test coverage - Wrap fs.realpathSync in try-catch to gracefully skip workspaces that disappear between existsSync and realpathSync (TOCTOU race). - Log verbose diagnostics for missing/unresolvable workspace paths. - Add test for overlapping allowlists deduplication on shared workspaces. - Add test for graceful skip of missing workspaces. - Add test for pagination callback without agent suffix (default agent). - Clean up temp directories in skill-commands tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(telegram): warn when nativeSkillsEnabled but no agent route is bound Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use runtime.log instead of nonexistent runtime.warn Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
300 lines
10 KiB
TypeScript
300 lines
10 KiB
TypeScript
import fs from "node:fs/promises";
|
|
import os from "node:os";
|
|
import path from "node:path";
|
|
import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
|
|
|
|
// Avoid importing the full chat command registry for reserved-name calculation.
|
|
vi.mock("./commands-registry.js", () => ({
|
|
listChatCommands: () => [],
|
|
}));
|
|
|
|
vi.mock("../infra/skills-remote.js", () => ({
|
|
getRemoteSkillEligibility: () => ({}),
|
|
}));
|
|
|
|
// Avoid filesystem-driven skill scanning for these unit tests; we only need command naming semantics.
|
|
vi.mock("../agents/skills.js", () => {
|
|
function resolveUniqueName(base: string, used: Set<string>): string {
|
|
let name = base;
|
|
let suffix = 2;
|
|
while (used.has(name.toLowerCase())) {
|
|
name = `${base}_${suffix}`;
|
|
suffix += 1;
|
|
}
|
|
used.add(name.toLowerCase());
|
|
return name;
|
|
}
|
|
|
|
function resolveWorkspaceSkills(
|
|
workspaceDir: string,
|
|
): Array<{ skillName: string; description: string }> {
|
|
const dirName = path.basename(workspaceDir);
|
|
if (dirName === "main") {
|
|
return [{ skillName: "demo-skill", description: "Demo skill" }];
|
|
}
|
|
if (dirName === "research") {
|
|
return [
|
|
{ skillName: "demo-skill", description: "Demo skill 2" },
|
|
{ skillName: "extra-skill", description: "Extra skill" },
|
|
];
|
|
}
|
|
return [];
|
|
}
|
|
|
|
return {
|
|
buildWorkspaceSkillCommandSpecs: (
|
|
workspaceDir: string,
|
|
opts?: { reservedNames?: Set<string>; skillFilter?: string[] },
|
|
) => {
|
|
const used = new Set<string>();
|
|
for (const reserved of opts?.reservedNames ?? []) {
|
|
used.add(String(reserved).toLowerCase());
|
|
}
|
|
const filter = opts?.skillFilter;
|
|
const entries =
|
|
filter === undefined
|
|
? resolveWorkspaceSkills(workspaceDir)
|
|
: resolveWorkspaceSkills(workspaceDir).filter((entry) =>
|
|
filter.some((skillName) => skillName === entry.skillName),
|
|
);
|
|
|
|
return entries.map((entry) => {
|
|
const base = entry.skillName.replace(/-/g, "_");
|
|
const name = resolveUniqueName(base, used);
|
|
return { name, skillName: entry.skillName, description: entry.description };
|
|
});
|
|
},
|
|
};
|
|
});
|
|
|
|
let listSkillCommandsForAgents: typeof import("./skill-commands.js").listSkillCommandsForAgents;
|
|
let resolveSkillCommandInvocation: typeof import("./skill-commands.js").resolveSkillCommandInvocation;
|
|
|
|
beforeAll(async () => {
|
|
({ listSkillCommandsForAgents, resolveSkillCommandInvocation } =
|
|
await import("./skill-commands.js"));
|
|
});
|
|
|
|
describe("resolveSkillCommandInvocation", () => {
|
|
it("matches skill commands and parses args", () => {
|
|
const invocation = resolveSkillCommandInvocation({
|
|
commandBodyNormalized: "/demo_skill do the thing",
|
|
skillCommands: [{ name: "demo_skill", skillName: "demo-skill", description: "Demo" }],
|
|
});
|
|
expect(invocation?.command.skillName).toBe("demo-skill");
|
|
expect(invocation?.args).toBe("do the thing");
|
|
});
|
|
|
|
it("supports /skill with name argument", () => {
|
|
const invocation = resolveSkillCommandInvocation({
|
|
commandBodyNormalized: "/skill demo_skill do the thing",
|
|
skillCommands: [{ name: "demo_skill", skillName: "demo-skill", description: "Demo" }],
|
|
});
|
|
expect(invocation?.command.name).toBe("demo_skill");
|
|
expect(invocation?.args).toBe("do the thing");
|
|
});
|
|
|
|
it("normalizes /skill lookup names", () => {
|
|
const invocation = resolveSkillCommandInvocation({
|
|
commandBodyNormalized: "/skill demo-skill",
|
|
skillCommands: [{ name: "demo_skill", skillName: "demo-skill", description: "Demo" }],
|
|
});
|
|
expect(invocation?.command.name).toBe("demo_skill");
|
|
expect(invocation?.args).toBeUndefined();
|
|
});
|
|
|
|
it("returns null for unknown commands", () => {
|
|
const invocation = resolveSkillCommandInvocation({
|
|
commandBodyNormalized: "/unknown arg",
|
|
skillCommands: [{ name: "demo_skill", skillName: "demo-skill", description: "Demo" }],
|
|
});
|
|
expect(invocation).toBeNull();
|
|
});
|
|
});
|
|
|
|
describe("listSkillCommandsForAgents", () => {
|
|
const tempDirs: string[] = [];
|
|
const makeTempDir = async (prefix: string) => {
|
|
const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix));
|
|
tempDirs.push(dir);
|
|
return dir;
|
|
};
|
|
afterAll(async () => {
|
|
await Promise.all(
|
|
tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })),
|
|
);
|
|
});
|
|
|
|
it("lists all agents when agentIds is omitted", async () => {
|
|
const baseDir = await makeTempDir("openclaw-skills-");
|
|
const mainWorkspace = path.join(baseDir, "main");
|
|
const researchWorkspace = path.join(baseDir, "research");
|
|
await fs.mkdir(mainWorkspace, { recursive: true });
|
|
await fs.mkdir(researchWorkspace, { recursive: true });
|
|
|
|
const commands = listSkillCommandsForAgents({
|
|
cfg: {
|
|
agents: {
|
|
list: [
|
|
{ id: "main", workspace: mainWorkspace },
|
|
{ id: "research", workspace: researchWorkspace },
|
|
],
|
|
},
|
|
},
|
|
});
|
|
const names = commands.map((entry) => entry.name);
|
|
expect(names).toContain("demo_skill");
|
|
expect(names).toContain("demo_skill_2");
|
|
expect(names).toContain("extra_skill");
|
|
});
|
|
|
|
it("scopes to specific agents when agentIds is provided", async () => {
|
|
const baseDir = await makeTempDir("openclaw-skills-filter-");
|
|
const researchWorkspace = path.join(baseDir, "research");
|
|
await fs.mkdir(researchWorkspace, { recursive: true });
|
|
|
|
const commands = listSkillCommandsForAgents({
|
|
cfg: {
|
|
agents: {
|
|
list: [{ id: "research", workspace: researchWorkspace, skills: ["extra-skill"] }],
|
|
},
|
|
},
|
|
agentIds: ["research"],
|
|
});
|
|
|
|
expect(commands.map((entry) => entry.name)).toEqual(["extra_skill"]);
|
|
expect(commands.map((entry) => entry.skillName)).toEqual(["extra-skill"]);
|
|
});
|
|
|
|
it("prevents cross-agent skill leakage when each agent has an allowlist", async () => {
|
|
const baseDir = await makeTempDir("openclaw-skills-leak-");
|
|
const mainWorkspace = path.join(baseDir, "main");
|
|
const researchWorkspace = path.join(baseDir, "research");
|
|
await fs.mkdir(mainWorkspace, { recursive: true });
|
|
await fs.mkdir(researchWorkspace, { recursive: true });
|
|
|
|
const commands = listSkillCommandsForAgents({
|
|
cfg: {
|
|
agents: {
|
|
list: [
|
|
{ id: "main", workspace: mainWorkspace, skills: ["demo-skill"] },
|
|
{ id: "research", workspace: researchWorkspace, skills: ["extra-skill"] },
|
|
],
|
|
},
|
|
},
|
|
agentIds: ["main", "research"],
|
|
});
|
|
|
|
expect(commands.map((entry) => entry.skillName)).toEqual(["demo-skill", "extra-skill"]);
|
|
expect(commands.map((entry) => entry.name)).toEqual(["demo_skill", "extra_skill"]);
|
|
});
|
|
|
|
it("merges allowlists for agents that share one workspace", async () => {
|
|
const baseDir = await makeTempDir("openclaw-skills-shared-");
|
|
const sharedWorkspace = path.join(baseDir, "research");
|
|
await fs.mkdir(sharedWorkspace, { recursive: true });
|
|
|
|
const commands = listSkillCommandsForAgents({
|
|
cfg: {
|
|
agents: {
|
|
list: [
|
|
{ id: "main", workspace: sharedWorkspace, skills: ["demo-skill"] },
|
|
{ id: "research", workspace: sharedWorkspace, skills: ["extra-skill"] },
|
|
],
|
|
},
|
|
},
|
|
agentIds: ["main", "research"],
|
|
});
|
|
|
|
expect(commands.map((entry) => entry.skillName)).toEqual(["demo-skill", "extra-skill"]);
|
|
expect(commands.map((entry) => entry.name)).toEqual(["demo_skill", "extra_skill"]);
|
|
});
|
|
|
|
it("deduplicates overlapping allowlists for shared workspace", async () => {
|
|
const baseDir = await makeTempDir("openclaw-skills-overlap-");
|
|
const sharedWorkspace = path.join(baseDir, "research");
|
|
await fs.mkdir(sharedWorkspace, { recursive: true });
|
|
|
|
const commands = listSkillCommandsForAgents({
|
|
cfg: {
|
|
agents: {
|
|
list: [
|
|
{ id: "agent-a", workspace: sharedWorkspace, skills: ["extra-skill"] },
|
|
{ id: "agent-b", workspace: sharedWorkspace, skills: ["extra-skill", "demo-skill"] },
|
|
],
|
|
},
|
|
},
|
|
agentIds: ["agent-a", "agent-b"],
|
|
});
|
|
|
|
// Both agents allowlist "extra-skill"; it should appear once, not twice.
|
|
expect(commands.map((entry) => entry.skillName)).toEqual(["demo-skill", "extra-skill"]);
|
|
expect(commands.map((entry) => entry.name)).toEqual(["demo_skill", "extra_skill"]);
|
|
});
|
|
|
|
it("keeps workspace unrestricted when one co-tenant agent has no skills filter", async () => {
|
|
const baseDir = await makeTempDir("openclaw-skills-unfiltered-");
|
|
const sharedWorkspace = path.join(baseDir, "research");
|
|
await fs.mkdir(sharedWorkspace, { recursive: true });
|
|
|
|
const commands = listSkillCommandsForAgents({
|
|
cfg: {
|
|
agents: {
|
|
list: [
|
|
{ id: "restricted", workspace: sharedWorkspace, skills: ["extra-skill"] },
|
|
{ id: "unrestricted", workspace: sharedWorkspace },
|
|
],
|
|
},
|
|
},
|
|
agentIds: ["restricted", "unrestricted"],
|
|
});
|
|
|
|
const skillNames = commands.map((entry) => entry.skillName);
|
|
expect(skillNames).toContain("demo-skill");
|
|
expect(skillNames).toContain("extra-skill");
|
|
});
|
|
|
|
it("merges empty allowlist with non-empty allowlist for shared workspace", async () => {
|
|
const baseDir = await makeTempDir("openclaw-skills-empty-");
|
|
const sharedWorkspace = path.join(baseDir, "research");
|
|
await fs.mkdir(sharedWorkspace, { recursive: true });
|
|
|
|
const commands = listSkillCommandsForAgents({
|
|
cfg: {
|
|
agents: {
|
|
list: [
|
|
{ id: "locked", workspace: sharedWorkspace, skills: [] },
|
|
{ id: "partial", workspace: sharedWorkspace, skills: ["extra-skill"] },
|
|
],
|
|
},
|
|
},
|
|
agentIds: ["locked", "partial"],
|
|
});
|
|
|
|
expect(commands.map((entry) => entry.skillName)).toEqual(["extra-skill"]);
|
|
});
|
|
|
|
it("skips agents with missing workspaces gracefully", async () => {
|
|
const baseDir = await makeTempDir("openclaw-skills-missing-");
|
|
const validWorkspace = path.join(baseDir, "research");
|
|
const missingWorkspace = path.join(baseDir, "nonexistent");
|
|
await fs.mkdir(validWorkspace, { recursive: true });
|
|
|
|
const commands = listSkillCommandsForAgents({
|
|
cfg: {
|
|
agents: {
|
|
list: [
|
|
{ id: "valid", workspace: validWorkspace },
|
|
{ id: "broken", workspace: missingWorkspace },
|
|
],
|
|
},
|
|
},
|
|
agentIds: ["valid", "broken"],
|
|
});
|
|
|
|
// The valid agent's skills should still be listed despite the broken one.
|
|
expect(commands.length).toBeGreaterThan(0);
|
|
expect(commands.map((entry) => entry.skillName)).toContain("demo-skill");
|
|
});
|
|
});
|