diff --git a/docs/scratchpads/824-mosaic-skill-cli.md b/docs/scratchpads/824-mosaic-skill-cli.md index 5bb9eca..47be56b 100644 --- a/docs/scratchpads/824-mosaic-skill-cli.md +++ b/docs/scratchpads/824-mosaic-skill-cli.md @@ -102,3 +102,11 @@ A built-CLI temp-home smoke test (no real `~/.claude` or Mosaic config touched) - 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. - Pi does not need this Claude bridge because its Mosaic launcher can consume the canonical root. Codex lifecycle parity remains explicitly deferred. - No deployment surface is affected. + +## PR #826 review remediation + +- 2026-07-17: Exact-head RoR requested changes for two ownership bugs: installer pruning deleted foreign-name links under `MOSAIC_HOME` outside canonical skills, and unregister deleted a same-root link targeting a different skill. It also requested trailing-dot rejection and executable coverage support. +- RED evidence: focused regression run failed 4 tests: register/unregister accepted `safe.`, misdirected unregister did not throw, and the install linker deleted the foreign-name link. +- GREEN evidence: `skill.spec.ts` passes 43/43, including live and dangling foreign-name links in a temp HOME/MOSAIC_HOME and the misdirected unregister invariant. +- Coverage: `vitest run src/commands/skill.spec.ts --coverage` passes configured 85% thresholds for `skill.ts`: 91.05% statements/lines, 86.27% branches, 95.23% functions. +- Full gates: package build passed; package tests passed 69 files / 1,332 tests plus framework shell suite; repository typecheck 42/42, lint 23/23, and format check passed. diff --git a/packages/mosaic/framework/tools/_scripts/mosaic-sync-skills b/packages/mosaic/framework/tools/_scripts/mosaic-sync-skills index 10c128c..c4c3c20 100755 --- a/packages/mosaic/framework/tools/_scripts/mosaic-sync-skills +++ b/packages/mosaic/framework/tools/_scripts/mosaic-sync-skills @@ -242,14 +242,10 @@ prune_stale_links_in_target() { continue fi - resolved="$(readlink -f "$link_path" 2>/dev/null || true)" - if [[ -z "$resolved" ]]; then - rm -f "$link_path" - echo "[mosaic-skills] Removed stale broken skill link: $link_path" - continue - fi - - if [[ "$resolved" == "$MOSAIC_HOME/"* ]]; then + # -m resolves lexical dangling targets too. If resolution fails, ownership + # is unproven and the link must be preserved. + resolved="$(readlink -m "$link_path" 2>/dev/null || true)" + if [[ -n "$resolved" && "$resolved" == "$canonical_real/"* ]]; then rm -f "$link_path" echo "[mosaic-skills] Removed stale retired skill link: $link_path" fi diff --git a/packages/mosaic/package.json b/packages/mosaic/package.json index 0de2b3d..5b045d1 100644 --- a/packages/mosaic/package.json +++ b/packages/mosaic/package.json @@ -53,6 +53,7 @@ }, "devDependencies": { "@types/node": "^22.0.0", + "@vitest/coverage-v8": "^2.0.0", "@types/react": "^18.3.0", "tsx": "^4.0.0", "typescript": "^5.8.0", diff --git a/packages/mosaic/src/commands/skill.spec.ts b/packages/mosaic/src/commands/skill.spec.ts index 969d6f9..cc768e6 100644 --- a/packages/mosaic/src/commands/skill.spec.ts +++ b/packages/mosaic/src/commands/skill.spec.ts @@ -113,6 +113,62 @@ describe('Claude skill bridge', () => { ); }); + describe('CLI status output', () => { + let previousExitCode: number | string | null | undefined; + + beforeEach(() => { + previousExitCode = process.exitCode; + process.exitCode = undefined; + }); + + afterEach(() => { + process.exitCode = previousExitCode; + }); + + async function run(...args: string[]): Promise { + const program = new Command().exitOverride(); + registerSkillCommand(program, paths); + await program.parseAsync(['node', 'mosaic', 'skill', ...args]); + } + + it('reports register repair/idempotency and unregister idempotency statuses', async () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => undefined); + createSkill('status-skill'); + + await run('register', 'status-skill'); + await run('register', 'status-skill'); + rmSync(join(paths.claudeSkillsDir, 'status-skill')); + symlinkSync( + join(paths.mosaicSkillsDir, 'retired'), + join(paths.claudeSkillsDir, 'status-skill'), + ); + await run('register', 'status-skill'); + await run('unregister', 'status-skill'); + await run('unregister', 'status-skill'); + + expect(log.mock.calls.flat()).toEqual([ + 'status-skill: registered', + 'status-skill: already registered', + 'status-skill: repaired dangling registration', + 'status-skill: unregistered', + 'status-skill: already unregistered', + ]); + log.mockRestore(); + }); + + it('reports empty and populated skill lists', async () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => undefined); + + await run('list'); + createSkill('listed'); + await run('list'); + + expect(log).toHaveBeenCalledWith('No Mosaic or Claude Code skills found.'); + expect(log).toHaveBeenCalledWith(expect.stringMatching(/^unregistered\s+listed$/)); + log.mockRestore(); + }); + }); + describe('registerSkill', () => { it('creates the exact canonical symlink and is idempotent', () => { createSkill('new-skill'); diff --git a/packages/mosaic/src/commands/skill.ts b/packages/mosaic/src/commands/skill.ts index fa2aa01..d4a4efa 100644 --- a/packages/mosaic/src/commands/skill.ts +++ b/packages/mosaic/src/commands/skill.ts @@ -88,6 +88,7 @@ export function validateSkillName(name: string): void { if ( name.length === 0 || name.startsWith('-') || + name.endsWith('.') || name.includes('..') || name.includes('/') || name.includes('\\') || @@ -230,6 +231,13 @@ export function unregisterSkill( const targetPath = resolveLinkTarget(linkPath); if (!isInsideSkillsRoot(targetPath, paths.mosaicSkillsDir)) throw foreignTargetError(linkPath); + const expectedTarget = resolve(directChild(paths.mosaicSkillsDir, name)); + if (targetPath !== expectedTarget) { + throw new SkillBridgeError( + `Refusing to unregister misdirected Mosaic skill symlink at ${linkPath}; it points to ${targetPath}, not ${expectedTarget}.`, + ); + } + unlinkSync(linkPath); return { name, status: 'unregistered', linkPath }; } diff --git a/packages/mosaic/vitest.config.ts b/packages/mosaic/vitest.config.ts index 6d8f18f..9476528 100644 --- a/packages/mosaic/vitest.config.ts +++ b/packages/mosaic/vitest.config.ts @@ -5,5 +5,16 @@ export default defineConfig({ globals: true, environment: 'node', testTimeout: 30_000, + coverage: { + provider: 'v8', + include: ['src/commands/skill.ts'], + reporter: ['text', 'json-summary'], + thresholds: { + statements: 85, + branches: 85, + functions: 85, + lines: 85, + }, + }, }, }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 71aeab5..33300ce 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -607,6 +607,9 @@ importers: '@types/react': specifier: ^18.3.0 version: 18.3.28 + '@vitest/coverage-v8': + specifier: ^2.0.0 + version: 2.1.9(vitest@2.1.9(@types/node@22.19.15)(jsdom@29.0.0(@noble/hashes@2.0.1))(lightningcss@1.31.1)) tsx: specifier: ^4.0.0 version: 4.21.0