feat(mosaic): add secure skill registration CLI #826
56
docs/scratchpads/824-mosaic-skill-cli.md
Normal file
56
docs/scratchpads/824-mosaic-skill-cli.md
Normal file
@@ -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.
|
||||
272
packages/mosaic/src/commands/skill.spec.ts
Normal file
272
packages/mosaic/src/commands/skill.spec.ts
Normal file
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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'));
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user