mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-10 16:14:58 +00:00
fix(skills): scope skill-command APIs to respect agent allowlists (#32155)
* 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>
This commit is contained in:
Altay
@@ -1,7 +1,7 @@
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { beforeAll, describe, expect, it, vi } from "vitest";
|
||||
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", () => ({
|
||||
@@ -44,14 +44,21 @@ vi.mock("../agents/skills.js", () => {
|
||||
return {
|
||||
buildWorkspaceSkillCommandSpecs: (
|
||||
workspaceDir: string,
|
||||
opts?: { reservedNames?: Set<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 resolveWorkspaceSkills(workspaceDir).map((entry) => {
|
||||
return entries.map((entry) => {
|
||||
const base = entry.skillName.replace(/-/g, "_");
|
||||
const name = resolveUniqueName(base, used);
|
||||
return { name, skillName: entry.skillName, description: entry.description };
|
||||
@@ -106,8 +113,20 @@ describe("resolveSkillCommandInvocation", () => {
|
||||
});
|
||||
|
||||
describe("listSkillCommandsForAgents", () => {
|
||||
it("merges command names across agents and de-duplicates", async () => {
|
||||
const baseDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-"));
|
||||
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 });
|
||||
@@ -128,4 +147,153 @@ describe("listSkillCommandsForAgents", () => {
|
||||
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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,7 +1,12 @@
|
||||
import fs from "node:fs";
|
||||
import { listAgentIds, resolveAgentWorkspaceDir } from "../agents/agent-scope.js";
|
||||
import {
|
||||
listAgentIds,
|
||||
resolveAgentSkillsFilter,
|
||||
resolveAgentWorkspaceDir,
|
||||
} from "../agents/agent-scope.js";
|
||||
import { buildWorkspaceSkillCommandSpecs, type SkillCommandSpec } from "../agents/skills.js";
|
||||
import type { OpenClawConfig } from "../config/config.js";
|
||||
import { logVerbose } from "../globals.js";
|
||||
import { getRemoteSkillEligibility } from "../infra/skills-remote.js";
|
||||
import { listChatCommands } from "./commands-registry.js";
|
||||
|
||||
@@ -45,25 +50,57 @@ export function listSkillCommandsForAgents(params: {
|
||||
cfg: OpenClawConfig;
|
||||
agentIds?: string[];
|
||||
}): SkillCommandSpec[] {
|
||||
const mergeSkillFilters = (existing?: string[], incoming?: string[]): string[] | undefined => {
|
||||
// undefined = no allowlist (unrestricted); [] = explicit empty allowlist (no skills).
|
||||
// If any agent is unrestricted for this workspace, keep command discovery unrestricted.
|
||||
if (existing === undefined || incoming === undefined) {
|
||||
return undefined;
|
||||
}
|
||||
// An empty allowlist contributes no skills but does not widen the merge to unrestricted.
|
||||
if (existing.length === 0) {
|
||||
return Array.from(new Set(incoming));
|
||||
}
|
||||
if (incoming.length === 0) {
|
||||
return Array.from(new Set(existing));
|
||||
}
|
||||
return Array.from(new Set([...existing, ...incoming]));
|
||||
};
|
||||
|
||||
const agentIds = params.agentIds ?? listAgentIds(params.cfg);
|
||||
const used = listReservedChatSlashCommandNames();
|
||||
const entries: SkillCommandSpec[] = [];
|
||||
const agentIds = params.agentIds ?? listAgentIds(params.cfg);
|
||||
// Track visited workspace dirs to avoid registering duplicate commands
|
||||
// when multiple agents share the same workspace directory (#5717).
|
||||
const visitedDirs = new Set<string>();
|
||||
// Group by canonical workspace to avoid duplicate registration when multiple
|
||||
// agents share the same directory (#5717), while still honoring per-agent filters.
|
||||
const workspaceFilters = new Map<string, { workspaceDir: string; skillFilter?: string[] }>();
|
||||
for (const agentId of agentIds) {
|
||||
const workspaceDir = resolveAgentWorkspaceDir(params.cfg, agentId);
|
||||
if (!fs.existsSync(workspaceDir)) {
|
||||
logVerbose(`Skipping agent "${agentId}": workspace does not exist: ${workspaceDir}`);
|
||||
continue;
|
||||
}
|
||||
// Resolve to canonical path to handle symlinks and relative paths
|
||||
const canonicalDir = fs.realpathSync(workspaceDir);
|
||||
if (visitedDirs.has(canonicalDir)) {
|
||||
let canonicalDir: string;
|
||||
try {
|
||||
canonicalDir = fs.realpathSync(workspaceDir);
|
||||
} catch {
|
||||
logVerbose(`Skipping agent "${agentId}": cannot resolve workspace: ${workspaceDir}`);
|
||||
continue;
|
||||
}
|
||||
visitedDirs.add(canonicalDir);
|
||||
const skillFilter = resolveAgentSkillsFilter(params.cfg, agentId);
|
||||
const existing = workspaceFilters.get(canonicalDir);
|
||||
if (existing) {
|
||||
existing.skillFilter = mergeSkillFilters(existing.skillFilter, skillFilter);
|
||||
continue;
|
||||
}
|
||||
workspaceFilters.set(canonicalDir, {
|
||||
workspaceDir,
|
||||
skillFilter,
|
||||
});
|
||||
}
|
||||
|
||||
for (const { workspaceDir, skillFilter } of workspaceFilters.values()) {
|
||||
const commands = buildWorkspaceSkillCommandSpecs(workspaceDir, {
|
||||
config: params.cfg,
|
||||
skillFilter,
|
||||
eligibility: { remote: getRemoteSkillEligibility() },
|
||||
reservedNames: used,
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user