From 501abfdba069fdfb30926bfacee19e516a16aaa3 Mon Sep 17 00:00:00 2001 From: wakqasahmed Date: Fri, 4 Sep 2026 19:05:49 +0200 Subject: [PATCH] Dedupe repeated installs to the same physical directory When multiple selected targets resolve to the same physical directory (e.g. several universal agents all writing to ~/.agents/skills, or two agent-specific dirs that both alias to the canonical dir via a symlink), the install loop previously wiped and rewrote that directory once per target. Cache the install result per resolved write path and reuse it for every subsequent target that maps to the same directory. This also reduces the number of separate rm+mkdir+write cycles issued against a single physical directory in quick succession, which matters when that directory is reached through a symlink to another location (e.g. a Syncthing-synced drive) rather than a plain local path. --- src/add.test.ts | 64 ++++++++++++++++++++++++++++++++++++- src/add.ts | 85 ++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 143 insertions(+), 6 deletions(-) diff --git a/src/add.test.ts b/src/add.test.ts index 714ce74eb..84bf5de8d 100644 --- a/src/add.test.ts +++ b/src/add.test.ts @@ -1,6 +1,14 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { execFileSync } from 'child_process'; -import { existsSync, rmSync, mkdirSync, writeFileSync, lstatSync, readFileSync } from 'fs'; +import { + existsSync, + rmSync, + mkdirSync, + writeFileSync, + lstatSync, + readFileSync, + symlinkSync, +} from 'fs'; import { join } from 'path'; import { tmpdir } from 'os'; import { runCli, stripAnsi } from './test-utils.ts'; @@ -332,6 +340,60 @@ description: Shared install path regression test expect(countPathLinesForSkill(result.stdout, 'shared-skill')).toBe(1); }); + // Regression test for #2098: installing into a global skills dir reached + // through a symlinked ~/.agents (e.g. moved to another drive/location and + // linked back) must succeed, including when several selected agents all + // resolve to that same shared universal directory. + it('installs global skills when ~/.agents is a symlinked directory', () => { + const sourceDir = join(testDir, 'source'); + const skillDir = join(sourceDir, 'skills', 'symlinked-home-skill'); + mkdirSync(skillDir, { recursive: true }); + writeFileSync( + join(skillDir, 'SKILL.md'), + `--- +name: symlinked-home-skill +description: Symlinked home directory regression test +--- + +# Symlinked Home Skill +` + ); + + const homeDir = join(testDir, 'home'); + const realAgentsHome = join(testDir, 'actual-agents-home'); + mkdirSync(homeDir, { recursive: true }); + mkdirSync(realAgentsHome, { recursive: true }); + symlinkSync(realAgentsHome, join(homeDir, '.agents')); + + const result = runCli( + [ + 'add', + sourceDir, + '-y', + '-g', + '--copy', + '--agent', + 'codex', + 'cursor', + 'cline', + 'warp', + 'zed', + ], + testDir, + { HOME: homeDir, USERPROFILE: homeDir } + ); + + expect(result.exitCode).toBe(0); + expect(result.stdout).not.toContain('Failed to install'); + expect(result.stdout).toContain('Installed 1 skill'); + expect(result.stdout).toContain('✓ symlinked-home-skill (copied)'); + + // Content must land at the real (symlink-resolved) location. + const installedSkillMd = join(realAgentsHome, 'skills', 'symlinked-home-skill', 'SKILL.md'); + expect(existsSync(installedSkillMd)).toBe(true); + expect(readFileSync(installedSkillMd, 'utf-8')).toContain('name: symlinked-home-skill'); + }); + it('preserves distinct copied install paths when --copy targets different agent directories', () => { const sourceDir = join(testDir, 'source'); const skillDir = join(sourceDir, 'skills', 'multi-target-skill'); diff --git a/src/add.ts b/src/add.ts index b7ab4b7ac..a723e2e66 100644 --- a/src/add.ts +++ b/src/add.ts @@ -13,6 +13,7 @@ import { installBlobSkillForAgent, isSkillInstalled, getCanonicalPath, + getInstallPath, installWellKnownSkillForAgent, type InstallMode, } from './installer.ts'; @@ -955,12 +956,46 @@ async function handleWellKnownSkills( error?: string; }[] = []; + // See writeCache comment in the disk/blob install loop above: multiple targets + // can resolve to the same physical directory, so install once per resolved + // directory and reuse the result for the rest. + const writeCache = new Map< + string, + { + path: string; + canonicalPath?: string; + mode: InstallMode; + symlinkFailed?: boolean; + skipped?: boolean; + } + >(); + for (const skill of selectedSkills) { for (const agent of targetAgents) { - const result = await installWellKnownSkillForAgent(skill, agent, { - global: installGlobally, - mode: installMode, - }); + const writeKey = + installMode === 'copy' + ? getInstallPath(skill.installName, agent, { global: installGlobally }) + : getCanonicalPath(skill.installName, { global: installGlobally, agent }); + + let result; + const cached = writeCache.get(writeKey); + if (cached) { + result = { success: true as const, ...cached }; + } else { + result = await installWellKnownSkillForAgent(skill, agent, { + global: installGlobally, + mode: installMode, + }); + if (result.success) { + writeCache.set(writeKey, { + path: result.path, + canonicalPath: result.canonicalPath, + mode: result.mode, + symlinkFailed: result.symlinkFailed, + skipped: result.skipped, + }); + } + } results.push({ skill: skill.installName, agent: agents[agent].displayName, @@ -1952,11 +1987,42 @@ export async function runAdd(args: string[], options: AddOptions = {}): Promise< pluginName?: string; }[] = []; + // Multiple selected targets can resolve to the identical physical directory + // (e.g. several universal agents all writing to ~/.agents/skills). Without this + // cache, each target redundantly wipes and rewrites that shared directory in a + // tight loop, which is at best wasteful and, when an ancestor directory is a + // symlink to a location watched by sync software, can fail outright. Install + // once per resolved directory and reuse the result for the rest. + const writeCache = new Map< + string, + { + path: string; + canonicalPath?: string; + mode: InstallMode; + symlinkFailed?: boolean; + skipped?: boolean; + } + >(); + for (const skill of selectedSkills) { for (const target of installTargets) { const { agent, subagent } = target; + const isBlob = blobResult && 'files' in skill; + const installName = isBlob ? (skill as BlobSkill).name : skill.name; + const writeKey = + installMode === 'copy' + ? getInstallPath(installName, agent, { global: installGlobally, eveSubagent: subagent }) + : getCanonicalPath(installName, { + global: installGlobally, + agent, + eveSubagent: subagent, + }); + let result; - if (blobResult && 'files' in skill) { + const cached = writeCache.get(writeKey); + if (cached) { + result = { success: true as const, ...cached }; + } else if (isBlob) { // Blob-based install: write files from snapshot const blobSkill = skill as BlobSkill; result = await installBlobSkillForAgent( @@ -1982,6 +2048,15 @@ export async function runAdd(args: string[], options: AddOptions = {}): Promise< createMissingAgentRoot: explicitlySelectedAgents.has(agent), }); } + if (!cached && result.success) { + writeCache.set(writeKey, { + path: result.path, + canonicalPath: result.canonicalPath, + mode: result.mode, + symlinkFailed: result.symlinkFailed, + skipped: result.skipped, + }); + } results.push({ skill: getSkillDisplayName(skill), agent: targetDisplayName(target),