diff --git a/src/cli/commands/add.test.ts b/src/cli/commands/add.test.ts new file mode 100644 index 0000000..8481de9 --- /dev/null +++ b/src/cli/commands/add.test.ts @@ -0,0 +1,407 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtemp, mkdir, writeFile, rm, readFile } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { runAdd, AddError } from "./add.js"; +import add from "./add.js"; +import { exec } from "../../utils/exec.js"; +import { resolveScope } from "../../scope.js"; + +const SKILL_MD = (name: string) => `--- +name: ${name} +description: Test skill ${name} +--- + +# ${name} +`; + +describe("runAdd", () => { + let tmpDir: string; + let stateDir: string; + let projectRoot: string; + let repoDir: string; + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), "dotagents-add-")); + stateDir = join(tmpDir, "state"); + projectRoot = join(tmpDir, "project"); + repoDir = join(tmpDir, "repo"); + + process.env["DOTAGENTS_STATE_DIR"] = stateDir; + + // Set up project + await mkdir(join(projectRoot, ".agents", "skills"), { recursive: true }); + await writeFile(join(projectRoot, "agents.toml"), "version = 1\n"); + + // Create a local git repo with skills + await mkdir(repoDir, { recursive: true }); + await exec("git", ["init"], { cwd: repoDir }); + await exec("git", ["config", "user.email", "test@test.com"], { cwd: repoDir }); + await exec("git", ["config", "user.name", "Test"], { cwd: repoDir }); + + await mkdir(join(repoDir, "pdf"), { recursive: true }); + await writeFile(join(repoDir, "pdf", "SKILL.md"), SKILL_MD("pdf")); + await writeFile(join(repoDir, "pdf", "prompt.md"), "Process PDFs"); + + await mkdir(join(repoDir, "skills", "review"), { recursive: true }); + await writeFile(join(repoDir, "skills", "review", "SKILL.md"), SKILL_MD("review")); + + await mkdir(join(repoDir, "skills", "commit"), { recursive: true }); + await writeFile(join(repoDir, "skills", "commit", "SKILL.md"), SKILL_MD("commit")); + + await exec("git", ["add", "."], { cwd: repoDir }); + await exec("git", ["commit", "-m", "initial"], { cwd: repoDir }); + }); + + afterEach(async () => { + delete process.env["DOTAGENTS_STATE_DIR"]; + await rm(tmpDir, { recursive: true }); + }); + + it("adds a single skill via names", async () => { + const scope = resolveScope("project", projectRoot); + const result = await runAdd({ + scope, + specifier: `git:${repoDir}`, + names: ["pdf"], + }); + + expect(result).toBe("pdf"); + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).toContain('name = "pdf"'); + }); + + it("adds multiple skills via names", async () => { + const scope = resolveScope("project", projectRoot); + const result = await runAdd({ + scope, + specifier: `git:${repoDir}`, + names: ["pdf", "review"], + }); + + expect(result).toEqual(["pdf", "review"]); + + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).toContain('name = "pdf"'); + expect(toml).toContain('name = "review"'); + }); + + it("throws when a skill is not found", async () => { + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: `git:${repoDir}`, + names: ["nonexistent"], + }), + ).rejects.toThrow(AddError); + }); + + it("throws when one of multiple skills is not found (fails fast)", async () => { + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: `git:${repoDir}`, + names: ["pdf", "nonexistent"], + }), + ).rejects.toThrow(AddError); + + // "pdf" should NOT have been partially added + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).not.toContain('name = "pdf"'); + }); + + it("throws when a skill already exists in config", async () => { + await writeFile( + join(projectRoot, "agents.toml"), + `version = 1\n\n[[skills]]\nname = "pdf"\nsource = "git:${repoDir}"\n`, + ); + + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: `git:${repoDir}`, + names: ["pdf"], + }), + ).rejects.toThrow(AddError); + }); + + it("throws when one of multiple skills already exists (no partial writes)", async () => { + await writeFile( + join(projectRoot, "agents.toml"), + `version = 1\n\n[[skills]]\nname = "pdf"\nsource = "git:${repoDir}"\n`, + ); + + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: `git:${repoDir}`, + names: ["review", "pdf"], + }), + ).rejects.toThrow(AddError); + + // "review" should NOT have been partially added + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).not.toContain('name = "review"'); + }); + + it("throws when --all is used with names", async () => { + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: `git:${repoDir}`, + names: ["pdf"], + all: true, + }), + ).rejects.toThrow(AddError); + }); + + it("auto-selects when repo has a single skill", async () => { + // Create a repo with only one skill + const singleRepo = join(tmpDir, "single-repo"); + await mkdir(singleRepo, { recursive: true }); + await exec("git", ["init"], { cwd: singleRepo }); + await exec("git", ["config", "user.email", "test@test.com"], { cwd: singleRepo }); + await exec("git", ["config", "user.name", "Test"], { cwd: singleRepo }); + await mkdir(join(singleRepo, "only-skill"), { recursive: true }); + await writeFile(join(singleRepo, "only-skill", "SKILL.md"), SKILL_MD("only-skill")); + await exec("git", ["add", "."], { cwd: singleRepo }); + await exec("git", ["commit", "-m", "initial"], { cwd: singleRepo }); + + const scope = resolveScope("project", projectRoot); + const result = await runAdd({ + scope, + specifier: `git:${singleRepo}`, + interactive: false, + }); + + expect(result).toBe("only-skill"); + }); + + it("throws in non-interactive mode with multiple skills and no names", async () => { + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: `git:${repoDir}`, + interactive: false, + }), + ).rejects.toThrow(/Multiple skills found/); + }); +}); + +describe("runAdd (local sources)", () => { + let tmpDir: string; + let stateDir: string; + let projectRoot: string; + let localSkillsDir: string; + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), "dotagents-add-local-")); + stateDir = join(tmpDir, "state"); + projectRoot = join(tmpDir, "project"); + + process.env["DOTAGENTS_STATE_DIR"] = stateDir; + + // Set up project with a local skills directory inside it + await mkdir(join(projectRoot, ".agents", "skills"), { recursive: true }); + await writeFile(join(projectRoot, "agents.toml"), "version = 1\n"); + + // Create a local directory with multiple skills (inside project root) + localSkillsDir = join(projectRoot, "local-skills"); + await mkdir(join(localSkillsDir, "pdf"), { recursive: true }); + await writeFile(join(localSkillsDir, "pdf", "SKILL.md"), SKILL_MD("pdf")); + + await mkdir(join(localSkillsDir, "skills", "review"), { recursive: true }); + await writeFile(join(localSkillsDir, "skills", "review", "SKILL.md"), SKILL_MD("review")); + + await mkdir(join(localSkillsDir, "skills", "commit"), { recursive: true }); + await writeFile(join(localSkillsDir, "skills", "commit", "SKILL.md"), SKILL_MD("commit")); + }); + + afterEach(async () => { + delete process.env["DOTAGENTS_STATE_DIR"]; + await rm(tmpDir, { recursive: true }); + }); + + it("adds a single local skill without names (reads root SKILL.md)", async () => { + // Create a single-skill local directory with SKILL.md at root + const singleSkillDir = join(projectRoot, "my-skill"); + await mkdir(singleSkillDir, { recursive: true }); + await writeFile(join(singleSkillDir, "SKILL.md"), SKILL_MD("my-skill")); + + const scope = resolveScope("project", projectRoot); + const result = await runAdd({ + scope, + specifier: "path:my-skill", + }); + + expect(result).toBe("my-skill"); + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).toContain('name = "my-skill"'); + }); + + it("adds a single local skill via names", async () => { + const scope = resolveScope("project", projectRoot); + const result = await runAdd({ + scope, + specifier: "path:local-skills", + names: ["pdf"], + }); + + expect(result).toBe("pdf"); + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).toContain('name = "pdf"'); + }); + + it("adds multiple local skills via names", async () => { + const scope = resolveScope("project", projectRoot); + const result = await runAdd({ + scope, + specifier: "path:local-skills", + names: ["pdf", "review"], + }); + + expect(result).toEqual(["pdf", "review"]); + + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).toContain('name = "pdf"'); + expect(toml).toContain('name = "review"'); + }); + + it("throws when a local skill is not found", async () => { + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: "path:local-skills", + names: ["nonexistent"], + }), + ).rejects.toThrow(AddError); + }); + + it("throws when one of multiple local skills is not found (fails fast)", async () => { + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: "path:local-skills", + names: ["pdf", "nonexistent"], + }), + ).rejects.toThrow(AddError); + + // "pdf" should NOT have been partially added + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).not.toContain('name = "pdf"'); + }); + + it("throws when one of multiple local skills already exists (no partial writes)", async () => { + await writeFile( + join(projectRoot, "agents.toml"), + `version = 1\n\n[[skills]]\nname = "pdf"\nsource = "path:local-skills"\n`, + ); + + const scope = resolveScope("project", projectRoot); + await expect( + runAdd({ + scope, + specifier: "path:local-skills", + names: ["review", "pdf"], + }), + ).rejects.toThrow(AddError); + + // "review" should NOT have been partially added + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).not.toContain('name = "review"'); + }); +}); + +describe("add() CLI parsing", () => { + let tmpDir: string; + let stateDir: string; + let projectRoot: string; + let repoDir: string; + let originalExitCode: typeof process.exitCode; + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), "dotagents-add-cli-")); + stateDir = join(tmpDir, "state"); + projectRoot = join(tmpDir, "project"); + repoDir = join(tmpDir, "repo"); + + process.env["DOTAGENTS_STATE_DIR"] = stateDir; + originalExitCode = process.exitCode; + + // Set up project + await mkdir(join(projectRoot, ".agents", "skills"), { recursive: true }); + await writeFile(join(projectRoot, "agents.toml"), "version = 1\n"); + + // Create a local git repo with skills + await mkdir(repoDir, { recursive: true }); + await exec("git", ["init"], { cwd: repoDir }); + await exec("git", ["config", "user.email", "test@test.com"], { cwd: repoDir }); + await exec("git", ["config", "user.name", "Test"], { cwd: repoDir }); + + await mkdir(join(repoDir, "pdf"), { recursive: true }); + await writeFile(join(repoDir, "pdf", "SKILL.md"), SKILL_MD("pdf")); + + await mkdir(join(repoDir, "skills", "review"), { recursive: true }); + await writeFile(join(repoDir, "skills", "review", "SKILL.md"), SKILL_MD("review")); + + await exec("git", ["add", "."], { cwd: repoDir }); + await exec("git", ["commit", "-m", "initial"], { cwd: repoDir }); + }); + + afterEach(async () => { + delete process.env["DOTAGENTS_STATE_DIR"]; + process.exitCode = originalExitCode; + await rm(tmpDir, { recursive: true }); + }); + + it("passes positional skill names to runAdd", async () => { + // We test the full CLI add() by running it against a real project dir + // The project root must be the cwd for resolveDefaultScope + const origCwd = process.cwd(); + process.chdir(projectRoot); + try { + await add([`git:${repoDir}`, "pdf", "review"]); + expect(process.exitCode).toBeUndefined(); + + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).toContain('name = "pdf"'); + expect(toml).toContain('name = "review"'); + } finally { + process.chdir(origCwd); + } + }); + + it("passes repeated --skill flags to runAdd", async () => { + const origCwd = process.cwd(); + process.chdir(projectRoot); + try { + await add([`git:${repoDir}`, "--skill", "pdf", "--skill", "review"]); + expect(process.exitCode).toBeUndefined(); + + const toml = await readFile(join(projectRoot, "agents.toml"), "utf-8"); + expect(toml).toContain('name = "pdf"'); + expect(toml).toContain('name = "review"'); + } finally { + process.chdir(origCwd); + } + }); + + it("errors when mixing positional names and --skill flags", async () => { + const origCwd = process.cwd(); + process.chdir(projectRoot); + try { + await add([`git:${repoDir}`, "pdf", "--skill", "review"]); + expect(process.exitCode).toBe(1); + } finally { + process.chdir(origCwd); + } + }); +}); diff --git a/src/cli/commands/add.ts b/src/cli/commands/add.ts index 8788143..7897986 100644 --- a/src/cli/commands/add.ts +++ b/src/cli/commands/add.ts @@ -5,7 +5,7 @@ import chalk from "chalk"; import { loadConfig } from "../../config/loader.js"; import { isWildcardDep } from "../../config/schema.js"; import { addSkillToConfig, addWildcardToConfig } from "../../config/writer.js"; -import { parseSource, resolveSkill, sourcesMatch } from "../../skills/resolver.js"; +import { parseSource, sourcesMatch, VALID_SKILL_NAME } from "../../skills/resolver.js"; import { discoverAllSkills } from "../../skills/discovery.js"; import { ensureCached } from "../../sources/cache.js"; import { validateTrustedSource, TrustError } from "../../trust/index.js"; @@ -31,13 +31,15 @@ export interface AddOptions { scope: ScopeRoot; specifier: string; ref?: string; - name?: string; + names?: string[]; all?: boolean; interactive?: boolean; } export async function runAdd(opts: AddOptions): Promise { - const { scope, specifier, ref, name: nameOverride, all, interactive } = opts; + const { scope, specifier, ref, names: rawNames, all, interactive } = opts; + // Deduplicate names to prevent writing duplicate config entries + const namesOverride = rawNames ? [...new Set(rawNames)] : rawNames; const { configPath } = scope; // Load config early so we can check trust before any network work @@ -61,7 +63,7 @@ export async function runAdd(opts: AddOptions): Promise { // --all: add a wildcard entry if (all) { - if (nameOverride) { + if (namesOverride?.length) { throw new AddError("Cannot use --all with --name. Use one or the other."); } @@ -80,22 +82,65 @@ export async function runAdd(opts: AddOptions): Promise { return "*"; } - // For git sources, resolve to discover the skill name + // Validate user-provided skill names before any filesystem operations + if (namesOverride?.length) { + for (const name of namesOverride) { + if (!VALID_SKILL_NAME.test(name)) { + throw new AddError( + `Invalid skill name "${name}". Names must start with alphanumeric and contain only alphanumeric, dots, underscores, or hyphens.`, + ); + } + } + } + let skillName: string; if (parsed.type === "local") { - // Local source — resolve and read SKILL.md for the name - const resolved = await resolveSkill( - nameOverride ?? "unknown", - { source: specifier }, - { projectRoot: scope.root }, - ); - if (resolved.type !== "local") throw new AddError("Unexpected resolve type for local source"); + // Local source — resolve the path + const { resolveLocalSource } = await import("../../sources/local.js"); + const localDir = await resolveLocalSource(scope.root, parsed.path!); + + if (namesOverride?.length) { + // User specified name(s), verify each exists in the local directory + const { discoverSkill } = await import("../../skills/discovery.js"); + + for (const name of namesOverride) { + const found = await discoverSkill(localDir, name); + if (!found) { + throw new AddError( + `Skill "${name}" not found in ${sourceForStorage}. ` + + `Use 'dotagents add ${sourceForStorage}' without --name to see available skills.`, + ); + } + } - const { loadSkillMd } = await import("../../skills/loader.js"); - const { join: pathJoin } = await import("node:path"); - const meta = await loadSkillMd(pathJoin(resolved.skillDir, "SKILL.md")); - skillName = nameOverride ?? meta.name; + if (namesOverride.length === 1) { + skillName = namesOverride[0]!; + } else { + // Multiple names — check all for duplicates before writing anything + for (const name of namesOverride) { + if (config.skills.some((s) => s.name === name)) { + throw new AddError( + `Skill "${name}" already exists in agents.toml. Remove it first or use 'dotagents update'.`, + ); + } + } + for (const name of namesOverride) { + await addSkillToConfig(configPath, name, { + source: sourceForStorage, + ...refOpts, + }); + } + await runInstall({ scope }); + return namesOverride; + } + } else { + // No names — load SKILL.md from root for the name + const { loadSkillMd } = await import("../../skills/loader.js"); + const { join: pathJoin } = await import("node:path"); + const meta = await loadSkillMd(pathJoin(localDir, "SKILL.md")); + skillName = meta.name; + } } else { // Git source — clone and discover const url = parsed.url!; @@ -107,17 +152,40 @@ export async function runAdd(opts: AddOptions): Promise { const cached = await ensureCached({ url: cloneUrl, cacheKey, ref: effectiveRef }); - if (nameOverride) { - // User specified name, verify it exists + if (namesOverride?.length) { + // User specified name(s), verify each exists const { discoverSkill } = await import("../../skills/discovery.js"); - const found = await discoverSkill(cached.repoDir, nameOverride); - if (!found) { - throw new AddError( - `Skill "${nameOverride}" not found in ${sourceForStorage}. ` + - `Use 'dotagents add ${sourceForStorage}' without --name to see available skills.`, - ); + + for (const name of namesOverride) { + const found = await discoverSkill(cached.repoDir, name); + if (!found) { + throw new AddError( + `Skill "${name}" not found in ${sourceForStorage}. ` + + `Use 'dotagents add ${sourceForStorage}' without --name to see available skills.`, + ); + } + } + + if (namesOverride.length === 1) { + skillName = namesOverride[0]!; + } else { + // Multiple names — check all for duplicates before writing anything + for (const name of namesOverride) { + if (config.skills.some((s) => s.name === name)) { + throw new AddError( + `Skill "${name}" already exists in agents.toml. Remove it first or use 'dotagents update'.`, + ); + } + } + for (const name of namesOverride) { + await addSkillToConfig(configPath, name, { + source: sourceForStorage, + ...refOpts, + }); + } + await runInstall({ scope }); + return namesOverride; } - skillName = nameOverride; } else { // Discover all skills and pick const skills = await discoverAllSkills(cached.repoDir); @@ -179,11 +247,11 @@ export async function runAdd(opts: AddOptions): Promise { return added; } } else { - // Non-interactive — list them and ask user to re-run with --name or --all + // Non-interactive — list them and ask user to re-run with skill names or --all const names = skills.map((s) => s.meta.name).sort(); throw new AddError( `Multiple skills found in ${sourceForStorage}: ${names.join(", ")}. ` + - `Use --name to specify which one, or --all for all skills.`, + `Specify skill names as arguments, use --skill to specify which ones, or --all for all skills.`, ); } } @@ -214,30 +282,45 @@ export default async function add(args: string[], flags?: { user?: boolean }): P allowPositionals: true, options: { ref: { type: "string" }, - name: { type: "string" }, - skill: { type: "string" }, + name: { type: "string", multiple: true }, + skill: { type: "string", multiple: true }, all: { type: "boolean" }, }, strict: true, }); - const nameValue = values["name"] ?? values["skill"]; - const specifier = positionals[0]; if (!specifier) { - console.error(chalk.red("Usage: dotagents add [--ref ] [--name ] [--all]")); + console.error( + chalk.red("Usage: dotagents add [...] [--skill ...] [--ref ] [--all]"), + ); process.exitCode = 1; return; } + // Collect skill names from positional args and flags + const positionalNames = positionals.slice(1); + const flagNames = [...(values["name"] ?? []), ...(values["skill"] ?? [])]; + + if (positionalNames.length > 0 && flagNames.length > 0) { + console.error( + chalk.red("Cannot mix positional skill names with --skill/--name flags. Use one or the other."), + ); + process.exitCode = 1; + return; + } + + const rawNames = [...positionalNames, ...flagNames]; + const names = rawNames.length > 0 ? [...new Set(rawNames)] : undefined; + try { const scope = flags?.user ? resolveScope("user") : resolveDefaultScope(resolve(".")); - const interactive = process.stdout.isTTY === true && !nameValue && !values["all"]; + const interactive = process.stdout.isTTY === true && !names && !values["all"]; const result = await runAdd({ scope, specifier, ref: values["ref"], - name: nameValue, + names, all: values["all"], interactive, }); diff --git a/src/skills/resolver.ts b/src/skills/resolver.ts index c5407a4..d8c7656 100644 --- a/src/skills/resolver.ts +++ b/src/skills/resolver.ts @@ -175,7 +175,7 @@ export interface NamedResolvedSkill { } /** Skill names must be safe for use in file paths. */ -const VALID_SKILL_NAME = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; +export const VALID_SKILL_NAME = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; /** * Resolve a wildcard dependency: discover all skills from a source and return them.