From 0bb91d7fce391ad301cd37840c352501c38da259 Mon Sep 17 00:00:00 2001 From: Greg Pstrucha <875316+gricha@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:40:36 -0400 Subject: [PATCH] ref(cli): Centralize skill symlink targets Move repeated scope, agent, and legacy target enumeration into a shared pure projection. Keep creation, repair, reporting, and output policy in their commands while preserving ordering, exact-string deduplication, and native skill readers. Verify sync repairs the projected legacy path and cover project and user scope semantics. Co-Authored-By: OpenAI Codex --- .../dotagents/src/cli/commands/doctor.test.ts | 14 +++++ packages/dotagents/src/cli/commands/doctor.ts | 20 ++----- packages/dotagents/src/cli/commands/init.ts | 46 +++++---------- .../src/cli/commands/install/agent-runtime.ts | 34 +++-------- .../dotagents/src/cli/commands/sync.test.ts | 4 +- packages/dotagents/src/cli/commands/sync.ts | 56 ++++--------------- .../src/targets/skill-symlinks.test.ts | 45 +++++++++++++++ .../dotagents/src/targets/skill-symlinks.ts | 37 ++++++++++++ 8 files changed, 136 insertions(+), 120 deletions(-) create mode 100644 packages/dotagents/src/targets/skill-symlinks.test.ts create mode 100644 packages/dotagents/src/targets/skill-symlinks.ts diff --git a/packages/dotagents/src/cli/commands/doctor.test.ts b/packages/dotagents/src/cli/commands/doctor.test.ts index 4084996..cd01785 100644 --- a/packages/dotagents/src/cli/commands/doctor.test.ts +++ b/packages/dotagents/src/cli/commands/doctor.test.ts @@ -102,6 +102,20 @@ describe("runDoctor", () => { expect(check?.message).toContain("pdf"); }); + it("detects a missing agent skill symlink", async () => { + await writeFile( + join(projectRoot, "agents.toml"), + `version = 1\nagents = ["claude", "cursor", "codex"]\n`, + ); + await writeFile(join(projectRoot, ".gitignore"), "agents.lock\n.agents/.gitignore\n"); + await writeFile(join(projectRoot, ".agents", ".gitignore"), "# managed\n"); + + const result = await runDoctor({ scope: resolveScope("project", projectRoot) }); + const check = result.checks.find((c) => c.name === "symlinks"); + expect(check?.status).toBe("warn"); + expect(check?.message).toContain("1 symlink(s)"); + }); + it("detects generated files tracked by git", async () => { // Initialize a git repo so git ls-files works const { execSync } = await import("node:child_process"); diff --git a/packages/dotagents/src/cli/commands/doctor.ts b/packages/dotagents/src/cli/commands/doctor.ts index 0b00479..c4469b6 100644 --- a/packages/dotagents/src/cli/commands/doctor.ts +++ b/packages/dotagents/src/cli/commands/doctor.ts @@ -10,7 +10,7 @@ import { isWildcardDep } from "../../config/schema.js"; import { loadLockfile } from "../../lockfile/loader.js"; import { writeLockfile } from "../../lockfile/writer.js"; import { verifySymlinks } from "../../symlinks/manager.js"; -import { getAgent } from "../../targets/registry.js"; +import { skillSymlinkTargets } from "../../targets/skill-symlinks.js"; import { resolveScope, resolveDefaultScope, ScopeError, type ScopeRoot } from "../../scope.js"; import { exec } from "@sentry/dotagents-lib"; import { isInPlaceSkill } from "../../utils/fs.js"; @@ -194,19 +194,11 @@ export async function runDoctor(opts: DoctorOptions): Promise { // 9. Symlinks (project scope only) if (scope.scope === "project" && existsSync(scope.agentsDir)) { - const targets: string[] = []; - const seenDirs = new Set(); - - for (const target of config.symlinks?.targets ?? []) { - seenDirs.add(target); - targets.push(`${scope.root}/${target}`); - } - for (const agentId of config.agents) { - const agent = getAgent(agentId); - if (!agent?.skillsParentDir || seenDirs.has(agent.skillsParentDir)) {continue;} - seenDirs.add(agent.skillsParentDir); - targets.push(`${scope.root}/${agent.skillsParentDir}`); - } + const targets = skillSymlinkTargets( + scope, + config.agents, + config.symlinks?.targets, + ); if (targets.length > 0) { const issues = await verifySymlinks(scope.agentsDir, targets); diff --git a/packages/dotagents/src/cli/commands/init.ts b/packages/dotagents/src/cli/commands/init.ts index 86ff0c8..b3b4fa9 100644 --- a/packages/dotagents/src/cli/commands/init.ts +++ b/packages/dotagents/src/cli/commands/init.ts @@ -1,12 +1,13 @@ import { existsSync } from "node:fs"; import { chmod, mkdir, readFile, writeFile } from "node:fs/promises"; -import { join, resolve } from "node:path"; +import { join, relative, resolve } from "node:path"; import chalk from "chalk"; import { generateDefaultConfig } from "../../config/writer.js"; import { writeAgentsGitignore, ensureRootGitignoreEntries } from "../../gitignore/writer.js"; import { ensureSkillsSymlink } from "../../symlinks/manager.js"; import { loadConfig } from "../../config/loader.js"; -import { getAgent, allAgentIds, allAgents } from "../../targets/registry.js"; +import { allAgentIds, allAgents } from "../../targets/registry.js"; +import { skillSymlinkTargets } from "../../targets/skill-symlinks.js"; import { parseArgs } from "node:util"; import * as clack from "@clack/prompts"; import { resolveScope, isInsideGitRepo, findGitDir, type ScopeRoot } from "../../scope.js"; @@ -71,36 +72,17 @@ export async function runInit(opts: InitOptions): Promise { // Symlinks — create per-agent symlinks so each agent discovers skills const symlinkResults: { target: string; created: boolean; migrated: string[] }[] = []; - if (scope.scope === "user") { - const seen = new Set(); - for (const agentId of config.agents) { - const agent = getAgent(agentId); - if (!agent?.userSkillsParentDirs) {continue;} - for (const dir of agent.userSkillsParentDirs) { - if (seen.has(dir)) {continue;} - seen.add(dir); - const result = await ensureSkillsSymlink(agentsDir, dir); - symlinkResults.push({ target: dir, ...result }); - } - } - } else { - const targets = config.symlinks?.targets ?? []; - for (const target of targets) { - const targetDir = join(scope.root, target); - const result = await ensureSkillsSymlink(agentsDir, targetDir); - symlinkResults.push({ target, ...result }); - } - - const seenParentDirs = new Set(targets); - for (const agentId of config.agents) { - const agent = getAgent(agentId); - if (!agent?.skillsParentDir) {continue;} - if (seenParentDirs.has(agent.skillsParentDir)) {continue;} - seenParentDirs.add(agent.skillsParentDir); - const targetDir = join(scope.root, agent.skillsParentDir); - const result = await ensureSkillsSymlink(agentsDir, targetDir); - symlinkResults.push({ target: agent.skillsParentDir, ...result }); - } + const symlinkTargets = skillSymlinkTargets( + scope, + config.agents, + config.symlinks?.targets, + ); + for (const target of symlinkTargets) { + const result = await ensureSkillsSymlink(agentsDir, target); + const displayTarget = scope.scope === "project" + ? relative(scope.root, target) + : target; + symlinkResults.push({ target: displayTarget, ...result }); } // Auto-install declared skills (best-effort — may fail offline) diff --git a/packages/dotagents/src/cli/commands/install/agent-runtime.ts b/packages/dotagents/src/cli/commands/install/agent-runtime.ts index 9506661..a8f2c6a 100644 --- a/packages/dotagents/src/cli/commands/install/agent-runtime.ts +++ b/packages/dotagents/src/cli/commands/install/agent-runtime.ts @@ -1,8 +1,7 @@ -import { join } from "node:path"; import type { AgentsConfig } from "../../../config/schema.js"; import type { ScopeRoot } from "../../../scope.js"; -import { getAgent } from "../../../targets/registry.js"; import { ensureSkillsSymlink } from "../../../symlinks/manager.js"; +import { skillSymlinkTargets } from "../../../targets/skill-symlinks.js"; import { projectMcpResolver, toMcpDeclarations, writeMcpConfigs } from "../../../targets/mcp-writer.js"; import { projectHookResolver, toHookDeclarations, writeHookConfigs } from "../../../targets/hook-writer.js"; import { userMcpResolver } from "../../../targets/paths.js"; @@ -19,32 +18,13 @@ export async function writeSkillSymlinks( config: AgentsConfig, scope: ScopeRoot, ): Promise { - if (scope.scope === "user") { - const seen = new Set(); - for (const agentId of config.agents) { - const agent = getAgent(agentId); - if (!agent?.userSkillsParentDirs) {continue;} - for (const dir of agent.userSkillsParentDirs) { - if (seen.has(dir)) {continue;} - seen.add(dir); - await ensureSkillsSymlink(scope.agentsDir, dir); - } - } - return; - } - - const targets = config.symlinks?.targets ?? []; + const targets = skillSymlinkTargets( + scope, + config.agents, + config.symlinks?.targets, + ); for (const target of targets) { - await ensureSkillsSymlink(scope.agentsDir, join(scope.root, target)); - } - - const seenParentDirs = new Set(targets); - for (const agentId of config.agents) { - const agent = getAgent(agentId); - if (!agent?.skillsParentDir) {continue;} - if (seenParentDirs.has(agent.skillsParentDir)) {continue;} - seenParentDirs.add(agent.skillsParentDir); - await ensureSkillsSymlink(scope.agentsDir, join(scope.root, agent.skillsParentDir)); + await ensureSkillsSymlink(scope.agentsDir, target); } } diff --git a/packages/dotagents/src/cli/commands/sync.test.ts b/packages/dotagents/src/cli/commands/sync.test.ts index 54da37d..dd46697 100644 --- a/packages/dotagents/src/cli/commands/sync.test.ts +++ b/packages/dotagents/src/cli/commands/sync.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { mkdtemp, mkdir, readFile, writeFile, rm } from "node:fs/promises"; +import { lstat, mkdtemp, mkdir, readFile, writeFile, rm } from "node:fs/promises"; import { existsSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; @@ -344,6 +344,7 @@ describe("runSync", () => { const result = await runSync({ scope: resolveScope("project", projectRoot) }); expect(result.symlinksRepaired).toBe(1); + expect((await lstat(join(projectRoot, ".claude", "skills"))).isSymbolicLink()).toBe(true); }); it("regenerates gitignore", async () => { @@ -386,6 +387,7 @@ describe("runSync", () => { const result = await runSync({ scope: resolveScope("project", projectRoot) }); expect(result.symlinksRepaired).toBe(1); + expect((await lstat(join(projectRoot, ".claude", "skills"))).isSymbolicLink()).toBe(true); }); it("repairs missing hook configs", async () => { diff --git a/packages/dotagents/src/cli/commands/sync.ts b/packages/dotagents/src/cli/commands/sync.ts index 21c8ce1..6357434 100644 --- a/packages/dotagents/src/cli/commands/sync.ts +++ b/packages/dotagents/src/cli/commands/sync.ts @@ -10,7 +10,7 @@ import { writeLockfile } from "../../lockfile/writer.js"; import { addSkillToConfig } from "../../config/writer.js"; import { writeAgentsGitignore, checkRootGitignoreEntries } from "../../gitignore/writer.js"; import { ensureSkillsSymlink, verifySymlinks } from "../../symlinks/manager.js"; -import { getAgent } from "../../targets/registry.js"; +import { skillSymlinkTargets } from "../../targets/skill-symlinks.js"; import { verifyMcpConfigs, writeMcpConfigs, toMcpDeclarations, projectMcpResolver } from "../../targets/mcp-writer.js"; import { verifyHookConfigs, writeHookConfigs, toHookDeclarations, projectHookResolver } from "../../targets/hook-writer.js"; import { pruneSubagentConfigs, verifySubagentConfigs, writeSubagentConfigs, projectSubagentResolver, userSubagentResolver } from "../../subagents/writer.js"; @@ -164,51 +164,15 @@ export async function runSync(opts: SyncOptions): Promise { // 4. Verify and repair symlinks let symlinksRepaired = 0; - - if (scope.scope === "user") { - const seen = new Set(); - const targets: string[] = []; - for (const agentId of config.agents) { - const agent = getAgent(agentId); - if (!agent?.userSkillsParentDirs) {continue;} - for (const dir of agent.userSkillsParentDirs) { - if (seen.has(dir)) {continue;} - seen.add(dir); - targets.push(dir); - } - } - - const symlinkIssues = await verifySymlinks(agentsDir, targets); - for (const issue of symlinkIssues) { - await ensureSkillsSymlink(agentsDir, issue.target); - symlinksRepaired++; - } - } else { - const legacyTargets = config.symlinks?.targets ?? []; - const legacyIssues = await verifySymlinks( - agentsDir, - legacyTargets.map((t) => join(scope.root, t)), - ); - for (const issue of legacyIssues) { - await ensureSkillsSymlink(agentsDir, join(scope.root, issue.target)); - symlinksRepaired++; - } - - const seenParentDirs = new Set(legacyTargets); - const agentTargets: string[] = []; - for (const agentId of config.agents) { - const agent = getAgent(agentId); - if (!agent?.skillsParentDir) {continue;} - if (seenParentDirs.has(agent.skillsParentDir)) {continue;} - seenParentDirs.add(agent.skillsParentDir); - agentTargets.push(join(scope.root, agent.skillsParentDir)); - } - - const agentSymlinkIssues = await verifySymlinks(agentsDir, agentTargets); - for (const issue of agentSymlinkIssues) { - await ensureSkillsSymlink(agentsDir, issue.target); - symlinksRepaired++; - } + const symlinkTargets = skillSymlinkTargets( + scope, + config.agents, + config.symlinks?.targets, + ); + const symlinkIssues = await verifySymlinks(agentsDir, symlinkTargets); + for (const issue of symlinkIssues) { + await ensureSkillsSymlink(agentsDir, issue.target); + symlinksRepaired++; } // 5. Verify and repair MCP configs diff --git a/packages/dotagents/src/targets/skill-symlinks.test.ts b/packages/dotagents/src/targets/skill-symlinks.test.ts new file mode 100644 index 0000000..c6ce124 --- /dev/null +++ b/packages/dotagents/src/targets/skill-symlinks.test.ts @@ -0,0 +1,45 @@ +import { homedir } from "node:os"; +import { join, resolve } from "node:path"; +import { describe, expect, it } from "vitest"; +import { resolveScope } from "../scope.js"; +import { skillSymlinkTargets } from "./skill-symlinks.js"; + +describe("skillSymlinkTargets", () => { + it("returns legacy targets before deduplicated agent targets", () => { + const scope = resolveScope("project", "/workspace/project"); + + expect( + skillSymlinkTargets( + scope, + ["claude", "cursor", "codex"], + [".legacy", ".claude", ".legacy"], + ), + ).toEqual([ + join(scope.root, ".legacy"), + join(scope.root, ".claude"), + ]); + }); + + it("returns deduplicated user targets and skips native readers", () => { + const scope = resolveScope("user"); + + expect( + skillSymlinkTargets( + scope, + ["claude", "cursor", "codex", "vscode", "opencode"], + [".legacy"], + ), + ).toEqual([join(homedir(), ".claude")]); + }); + + it("returns absolute project targets for a relative scope root", () => { + const scope = resolveScope("project", "relative/project"); + + expect( + skillSymlinkTargets(scope, ["claude"], ["/legacy"]), + ).toEqual([ + resolve(join("relative/project", "/legacy")), + resolve(join("relative/project", ".claude")), + ]); + }); +}); diff --git a/packages/dotagents/src/targets/skill-symlinks.ts b/packages/dotagents/src/targets/skill-symlinks.ts new file mode 100644 index 0000000..dc7420c --- /dev/null +++ b/packages/dotagents/src/targets/skill-symlinks.ts @@ -0,0 +1,37 @@ +import { join, resolve } from "node:path"; +import type { ScopeRoot } from "../scope.js"; +import { getAgent } from "./registry.js"; + +/** Owns the shared projection of configured agents and legacy entries to absolute symlink targets. */ +export function skillSymlinkTargets( + scope: ScopeRoot, + agentIds: readonly string[], + legacyTargets: readonly string[] = [], +): string[] { + const seen = new Set(); + const targets: string[] = []; + + if (scope.scope === "project") { + for (const target of legacyTargets) { + if (seen.has(target)) {continue;} + seen.add(target); + targets.push(target); + } + for (const agentId of agentIds) { + const target = getAgent(agentId)?.skillsParentDir; + if (!target || seen.has(target)) {continue;} + seen.add(target); + targets.push(target); + } + return targets.map((target) => resolve(join(scope.root, target))); + } + + for (const agentId of agentIds) { + for (const target of getAgent(agentId)?.userSkillsParentDirs ?? []) { + if (seen.has(target)) {continue;} + seen.add(target); + targets.push(target); + } + } + return targets; +}