diff --git a/src/cli/commands/plugin.ts b/src/cli/commands/plugin.ts index d5f4799..e428917 100644 --- a/src/cli/commands/plugin.ts +++ b/src/cli/commands/plugin.ts @@ -224,6 +224,12 @@ async function runUserSyncAndPrint(): Promise<{ ok: boolean; syncData: ReturnTyp } } + if (result.warnings && result.warnings.length > 0) { + console.log('\nWarnings:'); + for (const warning of result.warnings) { + console.log(` ⚠ ${warning}`); + } + } } return { ok: result.success && result.totalFailed === 0, syncData }; diff --git a/src/core/skills.ts b/src/core/skills.ts index be62ab6..9d6dad7 100644 --- a/src/core/skills.ts +++ b/src/core/skills.ts @@ -1,4 +1,4 @@ -import { existsSync } from 'node:fs'; +import { existsSync, lstatSync, type Dirent } from 'node:fs'; import { readFile, readdir } from 'node:fs/promises'; import { basename, join, relative, resolve } from 'node:path'; import { load } from 'js-yaml'; @@ -101,28 +101,50 @@ async function resolvePluginPath( export async function discoverNestedSkillEntries( scanRoot: string, + warnings: string[] = [], ): Promise { - return walkForSkillMd(scanRoot, scanRoot); + return walkForSkillMd(scanRoot, scanRoot, warnings); } async function walkForSkillMd( scanRoot: string, currentDir: string, + warnings: string[], ): Promise { - const entries = await readdir(currentDir, { withFileTypes: true }); + let entries: Dirent[]; + try { + entries = await readdir(currentDir, { withFileTypes: true }); + } catch (err) { + const code = + err && typeof err === 'object' && 'code' in err + ? String((err as NodeJS.ErrnoException).code) + : undefined; + warnings.push( + `Could not read '${currentDir}'${code ? ` (${code})` : ''} while scanning for skills. If this path looks unexpected (e.g. rooted near a drive root), check ~/.allagents/workspace.yaml for a stale plugin path, or rename ~/.allagents to ~/.allagents.bak and reinstall your plugins to reset it.`, + ); + return []; + } const discovered: DiscoveredSkillEntry[] = []; for (const entry of entries) { if (!entry.isDirectory()) continue; const skillPath = join(currentDir, entry.name); + + // Skip symlinks/junctions to avoid unbounded recursion through loops. + try { + if (lstatSync(skillPath).isSymbolicLink()) continue; + } catch { + continue; + } + if (existsSync(join(skillPath, 'SKILL.md'))) { const subpath = relative(scanRoot, skillPath).split(/[\\/]/).join('/'); discovered.push({ name: entry.name, subpath, skillPath }); continue; } - discovered.push(...(await walkForSkillMd(scanRoot, skillPath))); + discovered.push(...(await walkForSkillMd(scanRoot, skillPath, warnings))); } return discovered; diff --git a/src/core/sync.ts b/src/core/sync.ts index 0860d85..bbf2d19 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -1098,10 +1098,19 @@ export function computeDeletedArtifacts( */ async function collectAvailableSkillNames( validPlugins: ValidatedPlugin[], + warnings?: string[], ): Promise> { const names = new Set(); for (const plugin of validPlugins) { - const skills = await collectPluginSkills(plugin.resolved, plugin.plugin); + const skills = await collectPluginSkills( + plugin.resolved, + plugin.plugin, + undefined, + undefined, + undefined, + undefined, + warnings, + ); for (const skill of skills) { names.add(skill.folderName); } @@ -1458,6 +1467,7 @@ async function collectAllSkills( validatedPlugins: ValidatedPlugin[], disabledSkills?: Set, enabledSkills?: Set, + warnings?: string[], ): Promise { const allSkills: CollectedSkillEntry[] = []; @@ -1470,6 +1480,7 @@ async function collectAllSkills( pluginName, enabledSkills, plugin.pluginSkillsConfig, + warnings, ); for (const skill of skills) { @@ -2154,7 +2165,7 @@ export async function syncWorkspace( ? new Set(config.enabledSkills) : undefined; const allSkills = await sw.measure('skill-collection', () => - collectAllSkills(validPlugins, disabledSkillsSet, enabledSkillsSet), + collectAllSkills(validPlugins, disabledSkillsSet, enabledSkillsSet, warnings), ); // Build per-plugin skill name maps (handles conflicts automatically) @@ -2373,7 +2384,10 @@ export async function syncWorkspace( // Compute deleted artifacts: compare previous state vs what was just synced // Collect all skill names from installed plugins (including disabled) so that // skills that are still available but just not synced are not reported as deleted. - const availableSkillNames = await collectAvailableSkillNames(validPlugins); + const availableSkillNames = await collectAvailableSkillNames( + validPlugins, + warnings, + ); const allCopyResultsForState = [ ...pluginResults.flatMap((r) => r.copyResults), ...workspaceFileResults, @@ -2428,6 +2442,7 @@ export async function syncWorkspace( ); } + const uniqueWarnings = [...new Set(warnings)]; return { success: !hasFailures, pluginResults, @@ -2437,7 +2452,7 @@ export async function syncWorkspace( totalGenerated, purgedPaths, ...(deletedArtifacts.length > 0 && { deletedArtifacts }), - ...(warnings.length > 0 && { warnings }), + ...(uniqueWarnings.length > 0 && { warnings: uniqueWarnings }), ...(messages.length > 0 && { messages }), ...(Object.keys(mcpResults).length > 0 && { mcpResults }), ...(nativeResult && { nativeResult }), @@ -2577,7 +2592,7 @@ export async function syncUserWorkspace( ? new Set(config.enabledSkills) : undefined; const allSkills = await sw.measure('skill-collection', () => - collectAllSkills(validPlugins, disabledSkillsSet, enabledSkillsSet), + collectAllSkills(validPlugins, disabledSkillsSet, enabledSkillsSet, warnings), ); const pluginSkillMaps = buildPluginSkillNameMaps(allSkills); @@ -2751,8 +2766,10 @@ export async function syncUserWorkspace( ); // Compute deleted artifacts: compare previous state vs what was just synced - const availableUserSkillNames = - await collectAvailableSkillNames(validPlugins); + const availableUserSkillNames = await collectAvailableSkillNames( + validPlugins, + warnings, + ); const allCopyResultsForState = pluginResults.flatMap((r) => r.copyResults); const resolvedUserMappings = resolveClientMappings( syncClients, @@ -2799,6 +2816,7 @@ export async function syncUserWorkspace( ); } + const uniqueWarnings = [...new Set(warnings)]; return { success: totalFailed === 0, pluginResults, @@ -2807,7 +2825,7 @@ export async function syncUserWorkspace( totalSkipped, totalGenerated, ...(deletedArtifacts.length > 0 && { deletedArtifacts }), - ...(warnings.length > 0 && { warnings }), + ...(uniqueWarnings.length > 0 && { warnings: uniqueWarnings }), ...(messages.length > 0 && { messages }), ...(Object.keys(mcpResults).length > 0 && { mcpResults }), ...(nativeResult && { nativeResult }), diff --git a/src/core/transform.ts b/src/core/transform.ts index 9952b20..39bb373 100644 --- a/src/core/transform.ts +++ b/src/core/transform.ts @@ -485,8 +485,10 @@ export async function collectPluginSkills( pluginName?: string, enabledSkills?: Set, pluginSkillsConfig?: PluginSkillsConfig, + warnings?: string[], ): Promise { const skillsDir = join(pluginPath, 'skills'); + const skillWalkWarnings: string[] = []; // v1 fallback: only apply enabledSkills to plugins that actually have entries in the set const hasEnabledEntries = @@ -498,20 +500,23 @@ export async function collectPluginSkills( let candidateDirs: { name: string; subpath: string; path: string }[]; if (existsSync(skillsDir)) { - const entries = await discoverNestedSkillEntries(skillsDir); + const entries = await discoverNestedSkillEntries( + skillsDir, + skillWalkWarnings, + ); candidateDirs = entries.map((entry) => ({ name: entry.name, subpath: entry.subpath, path: entry.skillPath, })); } else { - const nestedDirs = (await discoverNestedSkillEntries(pluginPath)).map( - (entry) => ({ - name: entry.name, - subpath: entry.subpath, - path: entry.skillPath, - }), - ); + const nestedDirs = ( + await discoverNestedSkillEntries(pluginPath, skillWalkWarnings) + ).map((entry) => ({ + name: entry.name, + subpath: entry.subpath, + path: entry.skillPath, + })); if (nestedDirs.length > 0) { candidateDirs = nestedDirs; @@ -571,6 +576,12 @@ export async function collectPluginSkills( filteredDirs = candidateDirs; } + if (warnings && skillWalkWarnings.length > 0) { + warnings.push( + ...skillWalkWarnings.map((w) => `Plugin '${pluginSource}': ${w}`), + ); + } + return filteredDirs.map((entry) => ({ folderName: entry.name, skillPath: entry.path, diff --git a/src/core/user-workspace.ts b/src/core/user-workspace.ts index 247ffb7..6772c24 100644 --- a/src/core/user-workspace.ts +++ b/src/core/user-workspace.ts @@ -10,6 +10,7 @@ import type { } from '../models/workspace-config.js'; import { getPluginSource } from '../models/workspace-config.js'; import { + isFilesystemRoot, isGitHubUrl, parseGitHubUrl, validatePluginSource, @@ -160,6 +161,12 @@ export async function addUserPlugin( error: `Plugin not found at ${plugin}`, }; } + if (isFilesystemRoot(plugin)) { + return { + success: false, + error: `Plugin source cannot be a filesystem root directory: ${plugin}`, + }; + } } return addPluginToUserConfig(plugin, configPath, undefined, force); diff --git a/src/core/workspace-modify.ts b/src/core/workspace-modify.ts index 91415f5..804f10b 100644 --- a/src/core/workspace-modify.ts +++ b/src/core/workspace-modify.ts @@ -12,6 +12,7 @@ import type { import { getPluginSource } from '../models/workspace-config.js'; import { parseMarketplaceManifest } from '../utils/marketplace-manifest-parser.js'; import { + isFilesystemRoot, isGitHubUrl, parseGitHubUrl, validatePluginSource, @@ -163,6 +164,12 @@ export async function addPlugin( error: `Plugin not found at ${plugin}`, }; } + if (isFilesystemRoot(fullPath) || isFilesystemRoot(plugin)) { + return { + success: false, + error: `Plugin source cannot be a filesystem root directory: ${plugin}`, + }; + } } return await addPluginToConfig(plugin, configPath, undefined, force); diff --git a/src/utils/plugin-path.ts b/src/utils/plugin-path.ts index 3ceac3a..8a48df2 100644 --- a/src/utils/plugin-path.ts +++ b/src/utils/plugin-path.ts @@ -1,5 +1,5 @@ import { existsSync } from 'node:fs'; -import { isAbsolute, join, resolve } from 'node:path'; +import { isAbsolute, join, parse, resolve } from 'node:path'; import { getHomeDir } from '../constants.js'; import { GitCloneError, @@ -405,6 +405,17 @@ export function validatePluginSource(source: string): { return { valid: true }; } +/** + * Check whether a path resolves to the root of a filesystem (e.g. `C:\` on + * Windows or `/` on POSIX). A local-path plugin source can never legitimately + * be a filesystem root — every real plugin is a specific, self-contained + * directory. + */ +export function isFilesystemRoot(candidatePath: string): boolean { + const resolved = resolve(candidatePath); + return resolved === parse(resolved).root; +} + /** * Parsed file source information */ diff --git a/tests/unit/core/skills.test.ts b/tests/unit/core/skills.test.ts index 23f5ef2..5b4868d 100644 --- a/tests/unit/core/skills.test.ts +++ b/tests/unit/core/skills.test.ts @@ -1,9 +1,13 @@ -import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; -import { mkdtemp, rm, mkdir, writeFile } from 'node:fs/promises'; +import { describe, it, expect, beforeEach, afterEach, test } from 'bun:test'; +import { mkdtemp, rm, mkdir, writeFile, symlink } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { dump } from 'js-yaml'; -import { getAllSkillsFromPlugins, type SkillInfo } from '../../../src/core/skills.js'; +import { + getAllSkillsFromPlugins, + discoverNestedSkillEntries, + type SkillInfo, +} from '../../../src/core/skills.js'; import { getPluginCachePath } from '../../../src/utils/plugin-path.js'; describe('getAllSkillsFromPlugins', () => { @@ -240,3 +244,65 @@ describe('getAllSkillsFromPlugins', () => { } }); }); + +describe('discoverNestedSkillEntries error handling', () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), 'allagents-skills-walk-test-')); + }); + + afterEach(async () => { + await rm(tmpDir, { recursive: true, force: true }); + }); + + it('records a warning instead of throwing when the scan root cannot be read', async () => { + // A file (not a directory) makes readdir() throw ENOTDIR, exercising the + // same catch path a real EPERM/EACCES on a restricted directory would. + const notADir = join(tmpDir, 'not-a-directory'); + await writeFile(notADir, 'not a directory'); + + const warnings: string[] = []; + const entries = await discoverNestedSkillEntries(notADir, warnings); + + expect(entries).toEqual([]); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain(notADir); + expect(warnings[0]).toContain('workspace.yaml'); + }); + + it('still discovers sibling skills when one plugin path cannot be read', async () => { + const goodPlugin = join(tmpDir, 'good-plugin'); + await mkdir(join(goodPlugin, 'skills', 'good-skill'), { recursive: true }); + await writeFile(join(goodPlugin, 'skills', 'good-skill', 'SKILL.md'), '# good'); + + const badPlugin = join(tmpDir, 'bad-plugin-is-a-file'); + await writeFile(badPlugin, 'not a directory'); + + const warnings: string[] = []; + const goodEntries = await discoverNestedSkillEntries( + join(goodPlugin, 'skills'), + warnings, + ); + const badEntries = await discoverNestedSkillEntries(badPlugin, warnings); + + expect(goodEntries.map((e) => e.name)).toEqual(['good-skill']); + expect(badEntries).toEqual([]); + expect(warnings).toHaveLength(1); + }); + + test.skipIf(process.platform === 'win32')( + 'skips symlinked directories instead of following loops', + async () => { + const pluginDir = join(tmpDir, 'symlink-plugin'); + await mkdir(join(pluginDir, 'real-skill'), { recursive: true }); + await writeFile(join(pluginDir, 'real-skill', 'SKILL.md'), '# real'); + + // A symlink back to the plugin root would recurse forever if followed. + await symlink(pluginDir, join(pluginDir, 'loop'), 'dir'); + + const entries = await discoverNestedSkillEntries(pluginDir); + expect(entries.map((e) => e.name)).toEqual(['real-skill']); + }, + ); +}); diff --git a/tests/unit/core/sync-resilient.test.ts b/tests/unit/core/sync-resilient.test.ts index c9a15b1..0a2e619 100644 --- a/tests/unit/core/sync-resilient.test.ts +++ b/tests/unit/core/sync-resilient.test.ts @@ -89,6 +89,33 @@ describe('sync resilience - project scope', () => { expect(result.warnings).toBeDefined(); expect(result.warnings!.length).toBe(2); }); + + it('should sync the good plugin and warn (not throw) when another plugin path cannot be scanned for skills', async () => { + // A plugin whose *source* resolves to a real file (not a directory) passes + // plugin validation (existsSync) but can't be readdir'd during skill + // collection — this is what used to crash the whole sync with an uncaught + // EPERM/ENOTDIR (see #433-style bad-path bugs). + const goodPlugin = await createLocalPlugin('good-plugin', 'good-skill'); + const badPluginPath = join(testDir, 'bad-plugin-is-a-file'); + await writeFile(badPluginPath, 'not a directory'); + + await writeProjectConfig({ + repositories: [], + plugins: [goodPlugin, badPluginPath], + clients: ['claude'], + }); + + const result = await syncWorkspace(testDir); + + expect(result.totalCopied).toBeGreaterThan(0); + expect(result.warnings).toBeDefined(); + expect(result.warnings!.some((w) => w.includes(badPluginPath))).toBe(true); + // The same bad path is scanned twice internally (once to collect enabled + // skills, once to collect all skill names for deleted-artifact detection) + // — warnings should be deduplicated rather than shown twice. + const matching = result.warnings!.filter((w) => w.includes(badPluginPath)); + expect(matching).toHaveLength(1); + }); }); describe('sync resilience - user scope', () => { diff --git a/tests/unit/core/user-workspace.test.ts b/tests/unit/core/user-workspace.test.ts index 50848c7..5d39618 100644 --- a/tests/unit/core/user-workspace.test.ts +++ b/tests/unit/core/user-workspace.test.ts @@ -135,6 +135,16 @@ describe('user-workspace', () => { expect(result.error).toContain('Plugin not found at'); expect(result.error).toContain(nonExistentPath); }); + + test('rejects a bare path separator as a local plugin source', async () => { + const { sep } = await import('node:path'); + const result = await addUserPlugin(sep); + expect(result.success).toBe(false); + expect(result.error).toContain('filesystem root'); + + const config = await getUserWorkspaceConfig(); + expect(config?.plugins ?? []).toEqual([]); + }); }); describe('removeUserPlugin', () => { diff --git a/tests/unit/core/workspace-modify.test.ts b/tests/unit/core/workspace-modify.test.ts index 0be9c0f..1844b6c 100644 --- a/tests/unit/core/workspace-modify.test.ts +++ b/tests/unit/core/workspace-modify.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test, beforeEach, afterEach, mock } from 'bun:test'; import { mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; -import { join } from 'node:path'; +import { join, sep } from 'node:path'; import { tmpdir } from 'node:os'; import { dump, load } from 'js-yaml'; import type { WorkspaceConfig } from '../../../src/models/workspace-config.js'; @@ -251,3 +251,30 @@ describe('addPlugin with --force flag', () => { expect(updated.plugins.length).toBe(1); }); }); + +describe('addPlugin rejects filesystem-root local paths', () => { + let testDir = join(tmpdir(), `allagents-root-reject-test-${Date.now()}`); + + beforeEach(() => { + testDir = join(tmpdir(), `allagents-root-reject-test-${Date.now()}`); + mkdirSync(join(testDir, '.allagents'), { recursive: true }); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + }); + + test('rejects a bare path separator as a local plugin source', async () => { + const configPath = join(testDir, '.allagents', 'workspace.yaml'); + const initialConfig = { repositories: [], plugins: [], clients: ['universal'] }; + writeFileSync(configPath, dump(initialConfig, { lineWidth: -1 })); + + const result = await addPlugin(sep, testDir, false); + + expect(result.success).toBe(false); + expect(result.error).toContain('filesystem root'); + + const updated = load(readFileSync(configPath, 'utf-8')) as any; + expect(updated.plugins).toEqual([]); + }); +}); diff --git a/tests/unit/utils/plugin-path.test.ts b/tests/unit/utils/plugin-path.test.ts index 5dfb5ec..04ab3ae 100644 --- a/tests/unit/utils/plugin-path.test.ts +++ b/tests/unit/utils/plugin-path.test.ts @@ -37,6 +37,7 @@ const { validatePluginSource, verifyGitHubUrlExists, formatPluginSource, + isFilesystemRoot, } = await import('../../../src/utils/plugin-path.js'); describe('isGitHubUrl', () => { @@ -340,6 +341,25 @@ describe('validatePluginSource', () => { }); }); +describe('isFilesystemRoot', () => { + it('treats a bare path separator as a filesystem root', () => { + expect(isFilesystemRoot(sep)).toBe(true); + }); + + it('treats the resolved root path itself as a filesystem root', () => { + const root = resolve('.').split(sep)[0] + sep; + expect(isFilesystemRoot(root)).toBe(true); + }); + + it('does not treat a normal plugin directory as a filesystem root', () => { + expect(isFilesystemRoot(join(tmpdir(), 'some-plugin'))).toBe(false); + }); + + it('does not treat a relative path to a subdirectory as a filesystem root', () => { + expect(isFilesystemRoot('./some-plugin')).toBe(false); + }); +}); + describe('formatPluginSource', () => { it('shortens a bare GitHub HTTPS URL to owner/repo', () => { expect(formatPluginSource('https://github.com/anthropics/claude-plugins-official')).toBe(