roboomp
294a61d94d
The previous `anyBuiltInSkillSourceEnabled` mixed the new `enableAgentsUser`/`enableAgentsProject` defaults with the named third-party toggles, so a user who disabled Codex/Claude/Pi to silence third-party CLI skill sources but kept the default `.agent[s]/skills` toggles on still saw unknown providers (`opencode`, `github`, `claude-plugins`, `gemini`) load via the fallback branch. The `agents` provider already has its own explicit branch in `isSourceEnabled`, so the fall-through gate now only inspects the named third-party toggles. Adds a regression test that creates a fake `~/.config/opencode/skills/leaked-opencode` and verifies it stays filtered out when the third-party toggles are off and agents toggles default to on. Addresses PR #2405 review.
489 lines
18 KiB
TypeScript
489 lines
18 KiB
TypeScript
import { describe, expect, it, spyOn } from "bun:test";
|
|
import * as fs from "node:fs/promises";
|
|
import * as os from "node:os";
|
|
import * as path from "node:path";
|
|
import { type Skill as CapabilitySkill, skillCapability } from "@oh-my-pi/pi-coding-agent/capability/skill";
|
|
import { getCapability } from "@oh-my-pi/pi-coding-agent/discovery";
|
|
import { loadSkills, loadSkillsFromDir, type Skill } from "@oh-my-pi/pi-coding-agent/extensibility/skills";
|
|
|
|
const fixturesDir = path.resolve(import.meta.dirname, "fixtures/skills");
|
|
const collisionFixturesDir = path.resolve(import.meta.dirname, "fixtures/skills-collision");
|
|
|
|
const longSkillName = "this-is-a-very-long-skill-name-that-exceeds-the-sixty-four-character-limit-set-by-the-standard";
|
|
const expectedFixtureSkillOrder: string[] = [
|
|
"bad--name",
|
|
"different-name",
|
|
"Invalid_Name",
|
|
longSkillName,
|
|
"unknown-field",
|
|
"valid-skill",
|
|
];
|
|
|
|
/**
|
|
* Disable every named built-in skill source. Used by `loadSkills` option tests
|
|
* that need to isolate a custom directory or assert "no built-in leakage". Tests
|
|
* MUST spread this in: the discovery surface only ignores `~/.<dir>/skills/*` if
|
|
* every provider toggle resolves to false, otherwise stray skills from the
|
|
* developer's real `$HOME` (e.g. `~/.agents/skills/<name>/SKILL.md`) leak into
|
|
* the assertion.
|
|
*/
|
|
const DISABLE_ALL_BUILTIN_SKILLS = {
|
|
enableCodexUser: false,
|
|
enableClaudeUser: false,
|
|
enableClaudeProject: false,
|
|
enablePiUser: false,
|
|
enablePiProject: false,
|
|
enableAgentsUser: false,
|
|
enableAgentsProject: false,
|
|
} as const;
|
|
|
|
describe("skills", () => {
|
|
describe("loadSkillsFromDir", () => {
|
|
const loadFixtureRoot = () => loadSkillsFromDir({ dir: fixturesDir, source: "test" });
|
|
|
|
it("should load a valid skill from a skills root", async () => {
|
|
const { skills, warnings } = await loadFixtureRoot();
|
|
const validSkill = skills.find(skill => skill.name === "valid-skill");
|
|
|
|
expect(validSkill).toBeDefined();
|
|
expect(validSkill?.description).toBe("A valid skill for testing purposes.");
|
|
expect(validSkill?.source).toBe("test");
|
|
expect(warnings).toHaveLength(0);
|
|
});
|
|
|
|
it("should load skill when name doesn't match parent directory", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
|
|
expect(skills.some(skill => skill.name === "different-name")).toBe(true);
|
|
});
|
|
|
|
it("should load skill with invalid name characters", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
|
|
expect(skills.some(skill => skill.name === "Invalid_Name")).toBe(true);
|
|
});
|
|
|
|
it("should load skill when name exceeds 64 characters", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
|
|
expect(
|
|
skills.some(
|
|
skill =>
|
|
skill.name ===
|
|
"this-is-a-very-long-skill-name-that-exceeds-the-sixty-four-character-limit-set-by-the-standard",
|
|
),
|
|
).toBe(true);
|
|
});
|
|
|
|
it("should skip skill when description is missing", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
|
|
expect(skills.some(skill => skill.name === "missing-description")).toBe(false);
|
|
});
|
|
|
|
it("should load skill with unknown frontmatter fields", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
|
|
expect(skills.some(skill => skill.name === "unknown-field")).toBe(true);
|
|
});
|
|
|
|
it("should not load nested skills recursively", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
|
|
expect(skills.some(skill => skill.name === "child-skill")).toBe(false);
|
|
});
|
|
|
|
it("should skip files without frontmatter description", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
|
|
expect(skills.some(skill => skill.name === "no-frontmatter")).toBe(false);
|
|
});
|
|
|
|
it("should load skill with consecutive hyphens in name", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
|
|
expect(skills.some(skill => skill.name === "bad--name")).toBe(true);
|
|
});
|
|
|
|
it("should load all directly nested skills from fixture directory", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
const names = skills.map(skill => skill.name);
|
|
|
|
expect(names).toEqual(
|
|
expect.arrayContaining([
|
|
"valid-skill",
|
|
"different-name",
|
|
"Invalid_Name",
|
|
"this-is-a-very-long-skill-name-that-exceeds-the-sixty-four-character-limit-set-by-the-standard",
|
|
"unknown-field",
|
|
"bad--name",
|
|
]),
|
|
);
|
|
expect(names).not.toContain("child-skill");
|
|
expect(skills).toHaveLength(6);
|
|
});
|
|
|
|
it("should return skills sorted by name (case-insensitive)", async () => {
|
|
const { skills } = await loadFixtureRoot();
|
|
const names = skills.map(skill => skill.name);
|
|
|
|
expect(names).toEqual(expectedFixtureSkillOrder);
|
|
});
|
|
|
|
it("should return empty for non-existent directory", async () => {
|
|
const { skills, warnings } = await loadSkillsFromDir({
|
|
dir: "/non/existent/path",
|
|
source: "test",
|
|
});
|
|
expect(skills).toHaveLength(0);
|
|
expect(warnings).toHaveLength(0);
|
|
});
|
|
|
|
it("should return empty when scanning a single skill directory directly", async () => {
|
|
const { skills } = await loadSkillsFromDir({
|
|
dir: path.join(fixturesDir, "valid-skill"),
|
|
source: "test",
|
|
});
|
|
|
|
expect(skills).toHaveLength(0);
|
|
});
|
|
});
|
|
|
|
describe("loadSkills with options", () => {
|
|
it("should load from customDirectories only when built-ins disabled", async () => {
|
|
const { skills } = await loadSkills({ ...DISABLE_ALL_BUILTIN_SKILLS, customDirectories: [fixturesDir] });
|
|
expect(skills.length).toBeGreaterThan(0);
|
|
// Custom directory skills have source "custom:user"
|
|
expect(skills.every(s => s.source.startsWith("custom"))).toBe(true);
|
|
});
|
|
|
|
it("should return customDirectory skills sorted by name (case-insensitive)", async () => {
|
|
const { skills } = await loadSkills({ ...DISABLE_ALL_BUILTIN_SKILLS, customDirectories: [fixturesDir] });
|
|
|
|
expect(skills.map(s => s.name)).toEqual(expectedFixtureSkillOrder);
|
|
});
|
|
|
|
it("should keep user Claude skills when project .claude/skills is missing", async () => {
|
|
const tempHomeDir = await fs.mkdtemp(path.join(os.tmpdir(), "pi-claude-home-"));
|
|
const tempProjectDir = await fs.mkdtemp(path.join(os.tmpdir(), "pi-claude-project-"));
|
|
|
|
try {
|
|
const userSkillDir = path.join(tempHomeDir, ".claude", "skills", "user-only-skill");
|
|
await fs.mkdir(userSkillDir, { recursive: true });
|
|
await fs.writeFile(
|
|
path.join(userSkillDir, "SKILL.md"),
|
|
[
|
|
"---",
|
|
"name: user-only-skill",
|
|
"description: User-only Claude skill",
|
|
"---",
|
|
"",
|
|
"# User-only skill",
|
|
].join("\n"),
|
|
);
|
|
|
|
const capability = getCapability<CapabilitySkill>(skillCapability.id);
|
|
expect(capability).toBeDefined();
|
|
const claudeProvider = capability?.providers.find(provider => provider.id === "claude");
|
|
expect(claudeProvider).toBeDefined();
|
|
|
|
const result = await claudeProvider!.load({ cwd: tempProjectDir, home: tempHomeDir, repoRoot: null });
|
|
expect(result.items.some(skill => skill.name === "user-only-skill" && skill.level === "user")).toBe(true);
|
|
} finally {
|
|
await fs.rm(tempProjectDir, { recursive: true, force: true });
|
|
await fs.rm(tempHomeDir, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
// Regression for issue #2401: a user who disables the named third-party
|
|
// CLI toggles (codex/claude/native) MUST still see skills from the
|
|
// canonical OMP-native `~/.agent[s]/skills` (the `agents` provider).
|
|
// Pre-fix `loadSkills` gated `agents` on `anyBuiltInSkillSourceEnabled`,
|
|
// so flipping the five third-party toggles off silently disabled it.
|
|
it("should still load ~/.agents/skills when codex/claude/native toggles are off (#2401)", async () => {
|
|
const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "pi-agents-home-"));
|
|
const tempCwd = await fs.mkdtemp(path.join(os.tmpdir(), "pi-agents-cwd-"));
|
|
const skillDir = path.join(tempHome, ".agents", "skills", "user-agents-skill");
|
|
await fs.mkdir(skillDir, { recursive: true });
|
|
await fs.writeFile(
|
|
path.join(skillDir, "SKILL.md"),
|
|
["---", "description: Loaded from ~/.agents/skills", "---", "", "# user-agents-skill"].join("\n"),
|
|
);
|
|
const homedirSpy = spyOn(os, "homedir").mockReturnValue(tempHome);
|
|
try {
|
|
const { skills } = await loadSkills({
|
|
enableCodexUser: false,
|
|
enableClaudeUser: false,
|
|
enableClaudeProject: false,
|
|
enablePiUser: false,
|
|
enablePiProject: false,
|
|
// enableAgentsUser/enableAgentsProject left at their default-true value
|
|
cwd: tempCwd,
|
|
});
|
|
expect(skills.some(s => s.name === "user-agents-skill" && s.source === "agents:user")).toBe(true);
|
|
} finally {
|
|
homedirSpy.mockRestore();
|
|
await fs.rm(tempHome, { recursive: true, force: true });
|
|
await fs.rm(tempCwd, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
it("respects an explicit enableAgentsUser: false (#2401)", async () => {
|
|
const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "pi-agents-home-off-"));
|
|
const tempCwd = await fs.mkdtemp(path.join(os.tmpdir(), "pi-agents-cwd-off-"));
|
|
const skillDir = path.join(tempHome, ".agents", "skills", "opted-out");
|
|
await fs.mkdir(skillDir, { recursive: true });
|
|
await fs.writeFile(
|
|
path.join(skillDir, "SKILL.md"),
|
|
["---", "description: Should be filtered out", "---", "", "# opted-out"].join("\n"),
|
|
);
|
|
const homedirSpy = spyOn(os, "homedir").mockReturnValue(tempHome);
|
|
try {
|
|
const { skills } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
enableAgentsUser: false,
|
|
cwd: tempCwd,
|
|
});
|
|
expect(skills.some(s => s.name === "opted-out")).toBe(false);
|
|
} finally {
|
|
homedirSpy.mockRestore();
|
|
await fs.rm(tempHome, { recursive: true, force: true });
|
|
await fs.rm(tempCwd, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
// Regression for PR #2405 review: the fall-through gate used by
|
|
// unknown third-party providers (opencode/github/claude-plugins/...)
|
|
// MUST NOT consider the OMP-native `enableAgentsUser`/`...Project`
|
|
// toggles. Otherwise a user who disables Codex/Claude/Pi to silence
|
|
// third-party CLI noise but keeps the default agents toggles on still
|
|
// sees opencode skills resurface via the fallback branch.
|
|
it("does not re-enable third-party providers via the agents toggles (PR #2405)", async () => {
|
|
const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), "pi-opencode-home-"));
|
|
const tempCwd = await fs.mkdtemp(path.join(os.tmpdir(), "pi-opencode-cwd-"));
|
|
const opencodeSkillDir = path.join(tempHome, ".config", "opencode", "skills", "leaked-opencode");
|
|
await fs.mkdir(opencodeSkillDir, { recursive: true });
|
|
await fs.writeFile(
|
|
path.join(opencodeSkillDir, "SKILL.md"),
|
|
["---", "description: Should be filtered by third-party gate", "---", "", "# leaked-opencode"].join("\n"),
|
|
);
|
|
const homedirSpy = spyOn(os, "homedir").mockReturnValue(tempHome);
|
|
try {
|
|
const { skills } = await loadSkills({
|
|
enableCodexUser: false,
|
|
enableClaudeUser: false,
|
|
enableClaudeProject: false,
|
|
enablePiUser: false,
|
|
enablePiProject: false,
|
|
// enableAgentsUser / enableAgentsProject default true
|
|
cwd: tempCwd,
|
|
});
|
|
expect(skills.some(s => s.name === "leaked-opencode")).toBe(false);
|
|
} finally {
|
|
homedirSpy.mockRestore();
|
|
await fs.rm(tempHome, { recursive: true, force: true });
|
|
await fs.rm(tempCwd, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
it("should filter out ignoredSkills", async () => {
|
|
const { skills } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [fixturesDir],
|
|
ignoredSkills: ["valid-skill"],
|
|
});
|
|
expect(skills.some(s => s.name === "valid-skill")).toBe(false);
|
|
});
|
|
|
|
it("should support glob patterns in ignoredSkills", async () => {
|
|
const { skills } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [fixturesDir],
|
|
ignoredSkills: ["valid-*"],
|
|
});
|
|
expect(skills.every(s => !s.name.startsWith("valid-"))).toBe(true);
|
|
});
|
|
|
|
it("should skip skills disabled via frontmatter", async () => {
|
|
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-disabled-skill-"));
|
|
const skillDir = path.join(tempDir, "disabled-skill");
|
|
await fs.mkdir(skillDir, { recursive: true });
|
|
await fs.writeFile(
|
|
path.join(skillDir, "SKILL.md"),
|
|
`---
|
|
name: disabled-skill
|
|
description: Should not be discovered.
|
|
enabled: false
|
|
---
|
|
|
|
# Disabled Skill
|
|
`,
|
|
);
|
|
|
|
try {
|
|
const { skills } = await loadSkills({ ...DISABLE_ALL_BUILTIN_SKILLS, customDirectories: [tempDir] });
|
|
expect(skills.some(s => s.name === "disabled-skill")).toBe(false);
|
|
} finally {
|
|
await fs.rm(tempDir, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
it("should hide skills with disable-model-invocation frontmatter (Agent Skills spec)", async () => {
|
|
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-dmi-skill-"));
|
|
const skillDir = path.join(tempDir, "hidden-by-spec");
|
|
await fs.mkdir(skillDir, { recursive: true });
|
|
await fs.writeFile(
|
|
path.join(skillDir, "SKILL.md"),
|
|
`---\nname: hidden-by-spec\ndescription: Should be hidden via Agent Skills standard field.\ndisable-model-invocation: true\n---\n\n# Hidden Skill\n`,
|
|
);
|
|
|
|
try {
|
|
const { skills } = await loadSkills({ ...DISABLE_ALL_BUILTIN_SKILLS, customDirectories: [tempDir] });
|
|
const skill = skills.find(s => s.name === "hidden-by-spec");
|
|
expect(skill).toBeDefined();
|
|
expect(skill!.hide).toBe(true);
|
|
} finally {
|
|
await fs.rm(tempDir, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
it("should let ignoredSkills override includeSkills", async () => {
|
|
const { skills } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [fixturesDir],
|
|
includeSkills: ["valid-*"],
|
|
ignoredSkills: ["valid-skill"],
|
|
});
|
|
expect(skills.every(s => s.name !== "valid-skill")).toBe(true);
|
|
});
|
|
});
|
|
|
|
it("should expand ~ in customDirectories", async () => {
|
|
const tempHomeSkillsDir = await fs.mkdtemp(path.join(os.homedir(), ".pi-skills-test-"));
|
|
const relativeToHome = path.relative(os.homedir(), tempHomeSkillsDir);
|
|
const tildeDir = `~/${relativeToHome.split(path.sep).join("/")}`;
|
|
const skillDir = path.join(tempHomeSkillsDir, "tilde-skill");
|
|
const skillPath = path.join(skillDir, "SKILL.md");
|
|
await fs.mkdir(skillDir, { recursive: true });
|
|
await fs.writeFile(
|
|
skillPath,
|
|
`---
|
|
name: tilde-skill
|
|
description: Skill loaded from a tilde-expanded custom directory.
|
|
---
|
|
|
|
# Tilde Skill
|
|
`,
|
|
);
|
|
|
|
try {
|
|
const { skills: withTilde } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [tildeDir],
|
|
});
|
|
const { skills: withoutTilde } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [tempHomeSkillsDir],
|
|
});
|
|
expect(withTilde.length).toBe(withoutTilde.length);
|
|
expect(withTilde.some(skill => skill.name === "tilde-skill")).toBe(true);
|
|
} finally {
|
|
await fs.rm(tempHomeSkillsDir, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
it("should return empty when all sources disabled and no custom dirs", async () => {
|
|
const { skills } = await loadSkills({ ...DISABLE_ALL_BUILTIN_SKILLS });
|
|
expect(skills).toHaveLength(0);
|
|
});
|
|
|
|
it("should filter skills with includeSkills glob patterns", async () => {
|
|
// Load all skills from fixtures
|
|
const { skills: allSkills } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [fixturesDir],
|
|
});
|
|
expect(allSkills.length).toBeGreaterThan(0);
|
|
|
|
// Filter to only include "valid-skill"
|
|
const { skills: filtered } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [fixturesDir],
|
|
includeSkills: ["valid-skill"],
|
|
});
|
|
expect(filtered).toHaveLength(1);
|
|
expect(filtered[0].name).toBe("valid-skill");
|
|
});
|
|
|
|
it("should support glob patterns in includeSkills", async () => {
|
|
const { skills } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [fixturesDir],
|
|
includeSkills: ["valid-*"],
|
|
});
|
|
expect(skills.length).toBeGreaterThan(0);
|
|
expect(skills.every(s => s.name.startsWith("valid-"))).toBe(true);
|
|
});
|
|
|
|
it("should return all skills when includeSkills is empty", async () => {
|
|
const { skills: withEmpty } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [fixturesDir],
|
|
includeSkills: [],
|
|
});
|
|
const { skills: withoutOption } = await loadSkills({
|
|
...DISABLE_ALL_BUILTIN_SKILLS,
|
|
customDirectories: [fixturesDir],
|
|
});
|
|
expect(withEmpty.length).toBe(withoutOption.length);
|
|
});
|
|
});
|
|
|
|
describe("collision handling", () => {
|
|
it("should detect name collisions and keep first skill", async () => {
|
|
// Load from first directory
|
|
const first = await loadSkillsFromDir({
|
|
dir: path.join(collisionFixturesDir, "first"),
|
|
source: "first",
|
|
});
|
|
|
|
const second = await loadSkillsFromDir({
|
|
dir: path.join(collisionFixturesDir, "second"),
|
|
source: "second",
|
|
});
|
|
|
|
// Both directories should have loaded one skill each
|
|
expect(first.skills).toHaveLength(1);
|
|
expect(second.skills).toHaveLength(1);
|
|
|
|
// Both have the same name "calendar"
|
|
expect(first.skills[0].name).toBe("calendar");
|
|
expect(second.skills[0].name).toBe("calendar");
|
|
|
|
// Simulate the collision behavior from loadSkills()
|
|
const skillMap = new Map<string, Skill>();
|
|
const collisionWarnings: Array<{ skillPath: string; message: string }> = [];
|
|
|
|
for (const skill of first.skills) {
|
|
skillMap.set(skill.name, skill);
|
|
}
|
|
|
|
for (const skill of second.skills) {
|
|
const existing = skillMap.get(skill.name);
|
|
if (existing) {
|
|
collisionWarnings.push({
|
|
skillPath: skill.filePath,
|
|
message: `name collision: "${skill.name}" already loaded from ${existing.filePath}`,
|
|
});
|
|
} else {
|
|
skillMap.set(skill.name, skill);
|
|
}
|
|
}
|
|
|
|
expect(skillMap.size).toBe(1);
|
|
expect(skillMap.get("calendar")?.source).toBe("first");
|
|
expect(collisionWarnings).toHaveLength(1);
|
|
expect(collisionWarnings[0].message).toContain("name collision");
|
|
});
|
|
});
|