From 1cc45b7debfc6f82e93d066f2673f6161d2f95ce Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Fri, 17 Jul 2026 17:17:35 -0500 Subject: [PATCH] test(#824): define secure skill bridge behavior --- docs/scratchpads/824-mosaic-skill-cli.md | 56 ++++ packages/mosaic/src/commands/skill.spec.ts | 272 ++++++++++++++++++ .../src/runtime/update-checker.reseed.spec.ts | 27 ++ .../mosaic/src/stages/finalize-skills.spec.ts | 24 +- 4 files changed, 378 insertions(+), 1 deletion(-) create mode 100644 docs/scratchpads/824-mosaic-skill-cli.md create mode 100644 packages/mosaic/src/commands/skill.spec.ts diff --git a/docs/scratchpads/824-mosaic-skill-cli.md b/docs/scratchpads/824-mosaic-skill-cli.md new file mode 100644 index 0000000..c1c617c --- /dev/null +++ b/docs/scratchpads/824-mosaic-skill-cli.md @@ -0,0 +1,56 @@ +# Issue #824 — Mosaic skill CLI and Claude bridge auto-sync + +## Objective + +Deliver `mosaic skill register|unregister|list` plus install/upgrade reconciliation of every canonical `~/.config/mosaic/skills/*` entry into `~/.claude/skills/`, without clobbering runtime-owned files or directories. + +## Scope and constraints + +- Issue: mosaicstack/stack#824 +- Branch: `feat/824-mosaic-skill-cli` +- M1 runtime: Claude Code only. +- Pi/Codex parity is documentation-only; no non-Claude bridge implementation. +- Do not author the downstream `mosaic-context-refresh` skill. +- Workers do not modify `docs/TASKS.md`, merge, close #824, or touch `main`. +- TDD is mandatory and red-first; filesystem tests use temporary directories only. +- Budget: no explicit token cap supplied; use a focused single-worker implementation with no new dependencies. + +## Requirements mapping + +1. Register creates the canonical Claude symlink and is idempotent. +2. Names are untrusted: reject empty/escaping/absolute/separator/`..`/leading-dash names before filesystem mutation, with clear CLI stderr and nonzero status. +3. Register repairs only Mosaic-owned dangling symlinks and refuses foreign files, directories, and symlinks. +4. Unregister removes only symlinks pointing inside the canonical Mosaic skills root and is idempotent when absent. +5. List reports registered, dangling, foreign, and canonical-but-unregistered skills. +6. Install and upgrade generically reconcile all canonical skills after framework sync/re-seed, continuing past foreign conflicts without clobbering them. +7. User/developer documentation describes commands, status meanings, security boundaries, and Claude-only M1 scope. + +## Plan + +1. Add co-located failing Vitest coverage for all filesystem behaviors and auto-sync. +2. Run the focused spec and record the expected RED failure. +3. Commit the red contract as `test(#824): ...`. +4. Implement the skill bridge and Commander command registration. +5. Wire reconciliation into wizard finalize and `mosaic update` re-seed, preserving non-clobber behavior. +6. Update canonical docs and sitemap if navigation changes. +7. Run focused tests, package tests, typecheck, lint, and formatting. +8. Commit implementation/docs as `feat(#824): ...`, queue-guard, push, open PR with `Closes #824.`, fire completion event, and notify the coordinator. + +## Progress + +- 2026-07-17: Loaded mission/delivery/TDD/documentation rails, issue #824, active mission state, and relevant installer/update paths. +- 2026-07-17: Confirmed `mosaic update` invokes `framework/install.sh` with `MOSAIC_SYNC_ONLY=1`; that path exits before existing post-install skill linking, leaving newly present canonical skills unregistered. +- 2026-07-17: Coordinator addendum classified the user-supplied skill name and runtime symlink target as a path-traversal/symlink-injection surface. Expanded the initial red contract to reject traversal before mutation, preserve every foreign entry, and unregister Mosaic-owned links only. + +## Tests and evidence + +- RED environment attempt: `pnpm --filter @mosaicstack/mosaic exec vitest run src/commands/skill.spec.ts` initially could not locate Vitest because this fresh worktree had no dependencies. +- Dependency setup: `pnpm install --frozen-lockfile --store-dir /home/hermes/.local/share/pnpm/store` succeeded. The explicit store was required because machine pnpm config incorrectly resolves the default store under `/root`. +- RED verified: focused Vitest suite failed with `Failed to load url ./skill.js ... Does the file exist?`, proving the skill bridge API/implementation is absent before production code. + +## Risks + +- Symlink replacement must use `lstat` semantics so dangling links are detectable without following them. +- Link ownership must be determined lexically/canonically against the canonical skills root without following a foreign destination through an attacker-controlled path. +- Auto-sync must continue across conflicts while never deleting real files/directories or foreign symlinks. +- Claude Code discovers filesystem skills at session launch/reload boundaries; bridge creation makes a later `/reload-skills` or new session able to discover the skill, but cannot mutate an already-cached in-process registry by itself. diff --git a/packages/mosaic/src/commands/skill.spec.ts b/packages/mosaic/src/commands/skill.spec.ts new file mode 100644 index 0000000..77ec664 --- /dev/null +++ b/packages/mosaic/src/commands/skill.spec.ts @@ -0,0 +1,272 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { Command } from 'commander'; +import { + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readlinkSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + listSkills, + registerSkill, + registerSkillCommand, + syncClaudeSkills, + unregisterSkill, + type SkillPaths, +} from './skill.js'; + +describe('Claude skill bridge', () => { + let root: string; + let paths: SkillPaths; + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'mosaic-skill-cli-')); + paths = { + mosaicSkillsDir: join(root, '.config', 'mosaic', 'skills'), + claudeSkillsDir: join(root, '.claude', 'skills'), + }; + mkdirSync(paths.mosaicSkillsDir, { recursive: true }); + }); + + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); + + function createSkill(name: string): string { + const skillDir = join(paths.mosaicSkillsDir, name); + mkdirSync(skillDir, { recursive: true }); + writeFileSync(join(skillDir, 'SKILL.md'), `# ${name}\n`); + return skillDir; + } + + function expectCorrectLink(name: string): void { + const linkPath = join(paths.claudeSkillsDir, name); + expect(lstatSync(linkPath).isSymbolicLink()).toBe(true); + expect(readlinkSync(linkPath)).toBe(join(paths.mosaicSkillsDir, name)); + } + + describe('name validation', () => { + const invalidNames = ['../../etc', '/abs/path', 'a/b', String.raw`a\b`, '-rf', '..']; + + for (const name of invalidNames) { + it(`rejects ${JSON.stringify(name)} before register can escape its roots`, () => { + expect(() => registerSkill(name, paths)).toThrow(/invalid skill name/i); + expect(existsSync(paths.claudeSkillsDir)).toBe(false); + }); + + it(`rejects ${JSON.stringify(name)} before unregister can escape its roots`, () => { + expect(() => unregisterSkill(name, paths)).toThrow(/invalid skill name/i); + expect(existsSync(paths.claudeSkillsDir)).toBe(false); + }); + } + }); + + describe('CLI validation errors', () => { + let previousExitCode: number | string | null | undefined; + + beforeEach(() => { + previousExitCode = process.exitCode; + process.exitCode = undefined; + }); + + afterEach(() => { + process.exitCode = previousExitCode; + }); + + it.each(['register', 'unregister'])( + 'reports invalid %s names on stderr and sets a nonzero exit status', + async (subcommand) => { + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const program = new Command().exitOverride(); + registerSkillCommand(program, paths); + + await program.parseAsync(['node', 'mosaic', 'skill', subcommand, '../../etc']); + + expect(error).toHaveBeenCalledWith(expect.stringMatching(/invalid skill name/i)); + expect(process.exitCode).toBe(1); + expect(existsSync(paths.claudeSkillsDir)).toBe(false); + error.mockRestore(); + }, + ); + }); + + describe('registerSkill', () => { + it('creates the exact canonical symlink and is idempotent', () => { + createSkill('new-skill'); + + expect(registerSkill('new-skill', paths).status).toBe('registered'); + expectCorrectLink('new-skill'); + + expect(registerSkill('new-skill', paths).status).toBe('already-registered'); + expectCorrectLink('new-skill'); + }); + + it('repairs a dangling Mosaic-owned symlink', () => { + createSkill('new-skill'); + mkdirSync(paths.claudeSkillsDir, { recursive: true }); + symlinkSync( + join(paths.mosaicSkillsDir, 'retired-skill'), + join(paths.claudeSkillsDir, 'new-skill'), + ); + + expect(registerSkill('new-skill', paths).status).toBe('repaired'); + expectCorrectLink('new-skill'); + }); + + it.each(['file', 'directory', 'symlink'] as const)( + 'refuses to clobber a foreign %s at the target', + (kind) => { + createSkill('protected'); + mkdirSync(paths.claudeSkillsDir, { recursive: true }); + const target = join(paths.claudeSkillsDir, 'protected'); + const foreign = join(root, 'foreign'); + + if (kind === 'file') writeFileSync(target, 'keep me\n'); + if (kind === 'directory') mkdirSync(target); + if (kind === 'symlink') { + writeFileSync(foreign, 'keep me\n'); + symlinkSync(foreign, target); + } + + expect(() => registerSkill('protected', paths)).toThrow(/foreign|refus/i); + if (kind === 'file') expect(lstatSync(target).isFile()).toBe(true); + if (kind === 'directory') expect(lstatSync(target).isDirectory()).toBe(true); + if (kind === 'symlink') expect(readlinkSync(target)).toBe(foreign); + }, + ); + + it('refuses a symlinked Claude skills ancestor instead of writing outside the bridge root', () => { + createSkill('protected'); + const externalClaude = join(root, 'external-claude'); + mkdirSync(externalClaude); + symlinkSync(externalClaude, join(root, '.claude')); + + expect(() => registerSkill('protected', paths)).toThrow( + /symlink.*ancestor|ancestor.*symlink/i, + ); + expect(existsSync(join(externalClaude, 'skills', 'protected'))).toBe(false); + }); + + it('refuses a symlinked canonical skills root instead of registering an external source', () => { + rmSync(paths.mosaicSkillsDir, { recursive: true }); + const externalSkills = join(root, 'external-skills'); + mkdirSync(join(externalSkills, 'protected'), { recursive: true }); + symlinkSync(externalSkills, paths.mosaicSkillsDir); + + expect(() => registerSkill('protected', paths)).toThrow( + /symlink.*ancestor|ancestor.*symlink/i, + ); + expect(existsSync(paths.claudeSkillsDir)).toBe(false); + }); + + it('refuses a dangling foreign symlink rather than treating it as repairable', () => { + createSkill('protected'); + mkdirSync(paths.claudeSkillsDir, { recursive: true }); + const foreignMissing = join(root, 'foreign-missing'); + const target = join(paths.claudeSkillsDir, 'protected'); + symlinkSync(foreignMissing, target); + + expect(() => registerSkill('protected', paths)).toThrow(/foreign|refus/i); + expect(readlinkSync(target)).toBe(foreignMissing); + }); + }); + + describe('unregisterSkill', () => { + it('removes a Mosaic-owned symlink and is idempotent when absent', () => { + createSkill('removable'); + registerSkill('removable', paths); + + expect(unregisterSkill('removable', paths).status).toBe('unregistered'); + expect(existsSync(join(paths.claudeSkillsDir, 'removable'))).toBe(false); + + expect(unregisterSkill('removable', paths).status).toBe('already-unregistered'); + }); + + it.each(['file', 'directory', 'symlink'] as const)('refuses to remove a foreign %s', (kind) => { + mkdirSync(paths.claudeSkillsDir, { recursive: true }); + const target = join(paths.claudeSkillsDir, 'protected'); + const foreign = join(root, 'foreign'); + + if (kind === 'file') writeFileSync(target, 'keep me\n'); + if (kind === 'directory') mkdirSync(target); + if (kind === 'symlink') { + writeFileSync(foreign, 'keep me\n'); + symlinkSync(foreign, target); + } + + expect(() => unregisterSkill('protected', paths)).toThrow(/foreign|refus/i); + expect(lstatSync(target)).toBeDefined(); + if (kind === 'symlink') expect(readlinkSync(target)).toBe(foreign); + }); + }); + + describe('listSkills', () => { + it('flags registered, unregistered, Mosaic-owned dangling, and foreign entries', () => { + createSkill('registered'); + createSkill('unregistered'); + registerSkill('registered', paths); + symlinkSync(join(paths.mosaicSkillsDir, 'retired'), join(paths.claudeSkillsDir, 'dangling')); + writeFileSync(join(paths.claudeSkillsDir, 'foreign-file'), 'keep me\n'); + symlinkSync(join(root, 'missing-foreign'), join(paths.claudeSkillsDir, 'foreign-link')); + + expect(listSkills(paths)).toEqual([ + expect.objectContaining({ name: 'dangling', status: 'dangling' }), + expect.objectContaining({ name: 'foreign-file', status: 'foreign' }), + expect.objectContaining({ name: 'foreign-link', status: 'foreign-dangling' }), + expect.objectContaining({ name: 'registered', status: 'registered' }), + expect.objectContaining({ name: 'unregistered', status: 'unregistered' }), + ]); + }); + }); + + describe('syncClaudeSkills', () => { + it('generically creates every missing canonical link and repairs managed broken links', () => { + createSkill('added-after-setup'); + createSkill('another-new-skill'); + mkdirSync(paths.claudeSkillsDir, { recursive: true }); + symlinkSync( + join(paths.mosaicSkillsDir, 'retired'), + join(paths.claudeSkillsDir, 'added-after-setup'), + ); + + const result = syncClaudeSkills(paths); + + expect(result).toEqual({ + registered: ['another-new-skill'], + repaired: ['added-after-setup'], + unchanged: [], + conflicts: [], + }); + expectCorrectLink('added-after-setup'); + expectCorrectLink('another-new-skill'); + }); + + it('continues syncing other skills without clobbering foreign entries', () => { + createSkill('blocked'); + createSkill('link-me'); + mkdirSync(paths.claudeSkillsDir, { recursive: true }); + const blocked = join(paths.claudeSkillsDir, 'blocked'); + writeFileSync(blocked, 'keep me\n'); + + const result = syncClaudeSkills(paths); + + expect(result.registered).toEqual(['link-me']); + expect(result.conflicts).toEqual([ + expect.objectContaining({ + name: 'blocked', + reason: expect.stringMatching(/foreign|refus/i), + }), + ]); + expect(readlinkSync(join(paths.claudeSkillsDir, 'link-me'))).toBe( + join(paths.mosaicSkillsDir, 'link-me'), + ); + expect(lstatSync(blocked).isFile()).toBe(true); + }); + }); +}); diff --git a/packages/mosaic/src/runtime/update-checker.reseed.spec.ts b/packages/mosaic/src/runtime/update-checker.reseed.spec.ts index ce067e5..6f6fbba 100644 --- a/packages/mosaic/src/runtime/update-checker.reseed.spec.ts +++ b/packages/mosaic/src/runtime/update-checker.reseed.spec.ts @@ -7,6 +7,7 @@ import { mkdirSync, readdirSync, readFileSync, + readlinkSync, rmSync, statSync, symlinkSync, @@ -300,6 +301,32 @@ describe('repairFleetCommsTools', () => { }); describe('runFrameworkReseed', () => { + it('auto-registers every canonical skill after a successful upgrade re-seed', () => { + const root = mkdtempSync(join(tmpdir(), 'mosaic-reseed-skills-')); + const framework = join(root, 'framework'); + const home = join(root, 'mosaic'); + const claudeSkills = join(root, '.claude', 'skills'); + mkdirSync(framework, { recursive: true }); + mkdirSync(join(home, 'skills', 'added-after-setup'), { recursive: true }); + mkdirSync(join(home, 'skills', 'another-new-skill'), { recursive: true }); + writeFileSync(join(framework, 'install.sh'), '#!/usr/bin/env bash\nexit 0\n', { mode: 0o755 }); + + const res = runFrameworkReseed(framework, home, claudeSkills); + + expect(res.ok).toBe(true); + expect(res.skillSync).toMatchObject({ + registered: ['added-after-setup', 'another-new-skill'], + conflicts: [], + }); + expect(readlinkSync(join(claudeSkills, 'added-after-setup'))).toBe( + join(home, 'skills', 'added-after-setup'), + ); + expect(readlinkSync(join(claudeSkills, 'another-new-skill'))).toBe( + join(home, 'skills', 'another-new-skill'), + ); + rmSync(root, { recursive: true, force: true }); + }); + it('reports not-ok (not throw) when the installer is absent', () => { const missing = mkdtempSync(join(tmpdir(), 'mosaic-noinstaller-')); const res = runFrameworkReseed(missing, join(missing, 'home')); diff --git a/packages/mosaic/src/stages/finalize-skills.spec.ts b/packages/mosaic/src/stages/finalize-skills.spec.ts index 61e427c..c5c92e5 100644 --- a/packages/mosaic/src/stages/finalize-skills.spec.ts +++ b/packages/mosaic/src/stages/finalize-skills.spec.ts @@ -9,7 +9,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, readlinkSync, writeFileSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import type { WizardState } from '../types.js'; @@ -113,6 +113,28 @@ describe('finalizeStage — skill installer', () => { ); } + it('auto-registers every canonical skill even when it was added after initial setup', async () => { + const claudeHome = join(tmp, '.claude'); + const previousClaudeHome = process.env['CLAUDE_HOME']; + process.env['CLAUDE_HOME'] = claudeHome; + mkdirSync(join(tmp, 'skills', 'added-after-setup'), { recursive: true }); + mkdirSync(join(tmp, 'skills', 'another-new-skill'), { recursive: true }); + + try { + await finalizeStage(buildPrompter(), makeState(tmp, []), makeConfigService()); + + expect(readlinkSync(join(claudeHome, 'skills', 'added-after-setup'))).toBe( + join(tmp, 'skills', 'added-after-setup'), + ); + expect(readlinkSync(join(claudeHome, 'skills', 'another-new-skill'))).toBe( + join(tmp, 'skills', 'another-new-skill'), + ); + } finally { + if (previousClaudeHome === undefined) delete process.env['CLAUDE_HOME']; + else process.env['CLAUDE_HOME'] = previousClaudeHome; + } + }); + it('passes MOSAIC_INSTALL_SKILLS with the selected skill list', async () => { const state = makeState(tmp, ['brainstorming', 'lint', 'systematic-debugging']); const p = buildPrompter();