From fa0d2f64de08a318c01c3e79c221ee8d64710e8c Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Fri, 19 Jun 2026 14:22:07 -0500 Subject: [PATCH] fix(launch): include Pi native skill roots in 'all' mode; dedup 'discover' force-loads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fast-follow for the two code-review findings on #555. Finding 1 — `all` mode dropped Pi's native skill roots. `mosaic` passes `--no-skills`, which suppresses Pi's own auto-discovery, so the `all` catalog must re-enumerate the native roots (`~/.pi/agent/skills/` and `/.pi/skills/`) explicitly or skills living only there vanish. `discoverPiSkills` now scans those roots too. Also fixes a latent bug: the old enumerator skipped symlinked entries (`!isDirectory()`), but synced fleet skills land as symlinks — they were being dropped. Finding 2 — `discover` mode (which keeps native discovery ON) force-loaded fleet skills unconditionally, double-registering any skill Pi already finds natively. It now filters force-loads against the native-root realpath set. Implementation: realpath-based dedup throughout. New `skillRealPath`, `piNativeSkillRoots`, `enumerateSkillDirs` (accepts dirs + symlinks, dedup by realpath), `piNativeSkillRealPaths`. `mergeSkillArgs` dedups by realpath. `buildPiSkillArgs` gains an injectable 5th param for deterministic tests. Tests: discover-mode native-filter + intra-set dedup cases, plus real-FS coverage of `enumerateSkillDirs` (symlink acceptance, cross-root realpath dedup, SKILL.md gating). 308 pass; typecheck/lint/prettier green. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01QoYiBeKNh3BiYtAJS5Z587 --- packages/mosaic/src/commands/launch.spec.ts | 98 ++++++++++++++++++- packages/mosaic/src/commands/launch.ts | 101 +++++++++++++++++--- 2 files changed, 184 insertions(+), 15 deletions(-) diff --git a/packages/mosaic/src/commands/launch.spec.ts b/packages/mosaic/src/commands/launch.spec.ts index 560c269..c6d47fb 100644 --- a/packages/mosaic/src/commands/launch.spec.ts +++ b/packages/mosaic/src/commands/launch.spec.ts @@ -1,7 +1,11 @@ import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from 'vitest'; import { Command } from 'commander'; +import { mkdtempSync, mkdirSync, writeFileSync, symlinkSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { buildPiSkillArgs, + enumerateSkillDirs, piForceSkillNames, registerRuntimeLaunchers, type RuntimeLaunchHandler, @@ -116,11 +120,101 @@ describe('buildPiSkillArgs', () => { ]); }); - it('force-loads fleet skills even under native Pi discovery', () => { + it('force-loads fleet skills under native Pi discovery when not already discoverable', () => { + // Empty native set => Pi would not find mosaic-tools on its own, so force it. expect( - buildPiSkillArgs([], { MOSAIC_PI_SKILL_MODE: 'discover' }, fakeSkills, fakeForced), + buildPiSkillArgs([], { MOSAIC_PI_SKILL_MODE: 'discover' }, fakeSkills, fakeForced, new Set()), ).toEqual(['--skill', '/skills/mosaic-tools']); }); + + it('discover mode drops a forced skill Pi already discovers natively (no double-load)', () => { + // mosaic-tools is reachable from a Pi native root, so native discovery + // covers it — forcing it again would register the same skill twice. + expect( + buildPiSkillArgs( + [], + { MOSAIC_PI_SKILL_MODE: 'discover' }, + fakeSkills, + fakeForced, + new Set(['/skills/mosaic-tools']), + ), + ).toEqual([]); + }); + + it('discover mode keeps a forced skill that no native root provides', () => { + expect( + buildPiSkillArgs( + [], + { MOSAIC_PI_SKILL_MODE: 'discover' }, + fakeSkills, + fakeForced, + new Set(['/skills/some-other-skill']), + ), + ).toEqual(['--skill', '/skills/mosaic-tools']); + }); + + it('discover mode collapses a forced skill listed twice to a single --skill', () => { + // Mirror 'all' mode: intra-forced-set duplicates (same realpath) dedup. + expect( + buildPiSkillArgs( + [], + { MOSAIC_PI_SKILL_MODE: 'discover' }, + fakeSkills, + ['--skill', '/skills/mosaic-tools', '--skill', '/skills/mosaic-tools'], + new Set(), + ), + ).toEqual(['--skill', '/skills/mosaic-tools']); + }); +}); + +describe('enumerateSkillDirs (real FS)', () => { + let root: string; + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'mosaic-skills-')); + }); + + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); + + function makeSkill(parent: string, name: string): string { + const dir = join(parent, name); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, 'SKILL.md'), `# ${name}\n`); + return dir; + } + + it('accepts a symlinked skill dir (regression: synced fleet skills are symlinks)', () => { + // Real skill lives under `canonical/`; the scanned root only has a symlink to it. + const canonical = makeSkill(join(root, 'canonical'), 'mosaic-tools'); + const scanned = join(root, 'scanned'); + mkdirSync(scanned, { recursive: true }); + symlinkSync(canonical, join(scanned, 'mosaic-tools'), 'dir'); + + expect(enumerateSkillDirs([scanned])).toEqual(['--skill', join(scanned, 'mosaic-tools')]); + }); + + it('dedups by real path when the same skill is reachable from two roots', () => { + // Root A holds the real dir; root B symlinks to it — one --skill, not two. + const rootA = join(root, 'a'); + const rootB = join(root, 'b'); + const real = makeSkill(rootA, 'mosaic-tools'); + mkdirSync(rootB, { recursive: true }); + symlinkSync(real, join(rootB, 'mosaic-tools'), 'dir'); + + expect(enumerateSkillDirs([rootA, rootB])).toEqual(['--skill', real]); + }); + + it('skips directories without a SKILL.md and missing roots', () => { + mkdirSync(join(root, 'present', 'not-a-skill'), { recursive: true }); + makeSkill(join(root, 'present'), 'real-skill'); + + expect(enumerateSkillDirs([join(root, 'present'), join(root, 'does-not-exist')])).toEqual([ + '--skill', + join(root, 'present', 'real-skill'), + ]); + }); }); describe('piForceSkillNames', () => { diff --git a/packages/mosaic/src/commands/launch.ts b/packages/mosaic/src/commands/launch.ts index b7f7bdf..f545b42 100644 --- a/packages/mosaic/src/commands/launch.ts +++ b/packages/mosaic/src/commands/launch.ts @@ -6,7 +6,15 @@ */ import { execFileSync, execSync, spawnSync } from 'node:child_process'; -import { existsSync, mkdirSync, readFileSync, writeFileSync, readdirSync, rmSync } from 'node:fs'; +import { + existsSync, + mkdirSync, + readFileSync, + writeFileSync, + readdirSync, + realpathSync, + rmSync, +} from 'node:fs'; import { createRequire } from 'node:module'; import { homedir } from 'node:os'; import { join, dirname } from 'node:path'; @@ -428,25 +436,74 @@ function ensureRuntimeConfig(runtime: RuntimeName, destPath: string): void { // ─── Pi skill/extension discovery ──────────────────────────────────────────── -function discoverPiSkills(): string[] { +/** Resolve a skill dir to its canonical real path so symlinked duplicates + * (e.g. ~/.pi/agent/skills/X -> ~/.config/mosaic/skills/X) collapse to one key. + * Falls back to the literal path if it can't be resolved (e.g. broken link). */ +function skillRealPath(dir: string): string { + try { + return realpathSync(dir); + } catch { + return dir; + } +} + +/** Skill roots Pi auto-discovers natively (no `--skill` needed): its global + * skills dir and the project-local one relative to the launch cwd. */ +function piNativeSkillRoots(cwd: string = process.cwd()): string[] { + return [join(homedir(), '.pi', 'agent', 'skills'), join(cwd, '.pi', 'skills')]; +} + +/** Enumerate skill dirs under a set of roots, deduped by real path. A directory + * counts as a skill when it (or its symlink target) contains a SKILL.md. + * Exported for tests (real-FS coverage of symlink acceptance + realpath dedup). */ +export function enumerateSkillDirs(roots: string[]): string[] { + const seen = new Set(); const args: string[] = []; - for (const skillsRoot of [join(MOSAIC_HOME, 'skills'), join(MOSAIC_HOME, 'skills-local')]) { + for (const skillsRoot of roots) { if (!existsSync(skillsRoot)) continue; try { for (const entry of readdirSync(skillsRoot, { withFileTypes: true })) { - if (!entry.isDirectory()) continue; + // Synced fleet skills land as symlinks, so accept both dirs and links. + if (!entry.isDirectory() && !entry.isSymbolicLink()) continue; const skillDir = join(skillsRoot, entry.name); - if (existsSync(join(skillDir, 'SKILL.md'))) { - args.push('--skill', skillDir); - } + if (!existsSync(join(skillDir, 'SKILL.md'))) continue; + const key = skillRealPath(skillDir); + if (seen.has(key)) continue; + seen.add(key); + args.push('--skill', skillDir); } } catch { - // skip + // skip unreadable roots } } return args; } +/** Every skill dir Pi would link under `MOSAIC_PI_SKILL_MODE=all`: the Mosaic + * global/local catalog plus Pi's own native roots. `--no-skills` suppresses + * native auto-discovery, so 'all' must re-add the native roots explicitly or + * they would be silently dropped. Deduped by real path. */ +function discoverPiSkills(cwd: string = process.cwd()): string[] { + return enumerateSkillDirs([ + join(MOSAIC_HOME, 'skills'), + join(MOSAIC_HOME, 'skills-local'), + ...piNativeSkillRoots(cwd), + ]); +} + +/** Real paths of skills Pi will auto-discover from its native roots. Used to + * drop redundant force-loads in 'discover' mode (which keeps native discovery + * on) so the same skill is not registered twice. */ +function piNativeSkillRealPaths(cwd: string = process.cwd()): Set { + const args = enumerateSkillDirs(piNativeSkillRoots(cwd)); + const set = new Set(); + for (let i = 1; i < args.length; i += 2) { + const dir = args[i]; + if (dir !== undefined) set.add(skillRealPath(dir)); + } + return set; +} + type PiSkillMode = 'none' | 'all' | 'discover'; function normalizePiSkillMode(env: NodeJS.ProcessEnv): PiSkillMode { @@ -492,15 +549,19 @@ function forcedPiSkillArgs(env: NodeJS.ProcessEnv = process.env): string[] { return args; } -/** Concatenate `--skill ` arg groups, dropping any directory already seen. */ +/** Concatenate `--skill ` arg groups, dropping any skill already seen. + * Dedup is by real path, so a forced skill and the same skill reached via a + * different (e.g. symlinked) directory collapse to a single `--skill`. */ function mergeSkillArgs(...groups: string[][]): string[] { const seen = new Set(); const out: string[] = []; for (const group of groups) { for (let i = 0; i < group.length; i += 2) { const dir = group[i + 1]; - if (group[i] !== '--skill' || dir === undefined || seen.has(dir)) continue; - seen.add(dir); + if (group[i] !== '--skill' || dir === undefined) continue; + const key = skillRealPath(dir); + if (seen.has(key)) continue; + seen.add(key); out.push('--skill', dir); } } @@ -512,17 +573,31 @@ export function buildPiSkillArgs( env: NodeJS.ProcessEnv = process.env, discoveredSkillArgs: string[] = discoverPiSkills(), forcedSkillArgs: string[] = forcedPiSkillArgs(env), + nativeSkillRealPaths: Set = piNativeSkillRealPaths(), ): string[] { const mode = normalizePiSkillMode(env); if (mode === 'discover') { - // Native Pi discovery handles the rest; still force-load the fleet skills. - return [...forcedSkillArgs]; + // Native Pi discovery stays on, so only force-load fleet skills it will NOT + // already find under its native roots — otherwise the same skill is + // registered twice (once natively, once via --skill). mergeSkillArgs first + // collapses any intra-forced-set realpath duplicates, mirroring 'all' mode. + const deduped = mergeSkillArgs(forcedSkillArgs); + const out: string[] = []; + for (let i = 0; i < deduped.length; i += 2) { + const dir = deduped[i + 1]; + if (deduped[i] !== '--skill' || dir === undefined) continue; + if (nativeSkillRealPaths.has(skillRealPath(dir))) continue; + out.push('--skill', dir); + } + return out; } if (mode === 'all') { // 'all' links the full catalog; merge in the forced set so fleet-critical // skills are guaranteed present even if they live only under skills-local/. + // discoverPiSkills already covers Pi's native roots, which `--no-skills` + // would otherwise suppress. return ['--no-skills', ...mergeSkillArgs(discoveredSkillArgs, forcedSkillArgs)]; }