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 `<cwd>/.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoYiBeKNh3BiYtAJS5Z587
283 lines
9.7 KiB
TypeScript
283 lines
9.7 KiB
TypeScript
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,
|
|
} from './launch.js';
|
|
|
|
/**
|
|
* Tests for the commander wiring between `mosaic <runtime>` / `mosaic yolo <runtime>`
|
|
* subcommands and the internal `launchRuntime` dispatcher.
|
|
*
|
|
* Regression target: see mosaicstack/stack#454 — before the fix, `mosaic yolo claude`
|
|
* passed the literal string "claude" as an excess positional argument to the
|
|
* underlying CLI, which Claude Code then interpreted as the first user message.
|
|
*
|
|
* The bug existed because Commander.js includes declared positional arguments
|
|
* (here `<runtime>`) in `cmd.args` alongside any true excess args. The action
|
|
* handler must slice them off before forwarding.
|
|
*/
|
|
|
|
function buildProgram(handler: RuntimeLaunchHandler): Command {
|
|
const program = new Command();
|
|
program.exitOverride(); // prevent process.exit on parse errors
|
|
registerRuntimeLaunchers(program, handler);
|
|
return program;
|
|
}
|
|
|
|
const fakeSkills = ['--skill', '/skills/test-driven-development', '--skill', '/skills/pdf'];
|
|
const fakeForced = ['--skill', '/skills/mosaic-tools'];
|
|
|
|
// `process.exit` returns `never`, so vi.spyOn demands a replacement with the
|
|
// same signature. We throw from the mock to short-circuit into test-land.
|
|
const exitThrows = (): never => {
|
|
throw new Error('process.exit called');
|
|
};
|
|
|
|
describe('registerRuntimeLaunchers — non-yolo subcommands', () => {
|
|
let mockExit: MockInstance<typeof process.exit>;
|
|
|
|
beforeEach(() => {
|
|
// process.exit is called when the yolo action rejects an invalid runtime.
|
|
// Stub it so the assertion catches the rejection instead of terminating
|
|
// the test runner.
|
|
mockExit = vi.spyOn(process, 'exit').mockImplementation(exitThrows);
|
|
});
|
|
|
|
afterEach(() => {
|
|
mockExit.mockRestore();
|
|
});
|
|
|
|
it.each(['claude', 'codex', 'opencode', 'pi'] as const)(
|
|
'forwards %s with empty extraArgs and yolo=false',
|
|
(runtime) => {
|
|
const handler = vi.fn();
|
|
const program = buildProgram(handler);
|
|
program.parse(['node', 'mosaic', runtime]);
|
|
|
|
expect(handler).toHaveBeenCalledTimes(1);
|
|
expect(handler).toHaveBeenCalledWith(runtime, [], false);
|
|
},
|
|
);
|
|
|
|
it('forwards excess args after a non-yolo runtime subcommand', () => {
|
|
const handler = vi.fn();
|
|
const program = buildProgram(handler);
|
|
program.parse(['node', 'mosaic', 'claude', '--print', 'hello']);
|
|
|
|
expect(handler).toHaveBeenCalledWith('claude', ['--print', 'hello'], false);
|
|
});
|
|
});
|
|
|
|
describe('buildPiSkillArgs', () => {
|
|
it('disables auto-discovery but force-loads fleet-critical skills by default', () => {
|
|
expect(buildPiSkillArgs([], {}, fakeSkills, fakeForced)).toEqual([
|
|
'--no-skills',
|
|
'--skill',
|
|
'/skills/mosaic-tools',
|
|
]);
|
|
});
|
|
|
|
it('ignores _runtimeArgs (user --skill flags reach Pi via the launch handler, not here)', () => {
|
|
expect(buildPiSkillArgs(['--skill', '/tmp/custom'], {}, fakeSkills, fakeForced)).toEqual([
|
|
'--no-skills',
|
|
'--skill',
|
|
'/skills/mosaic-tools',
|
|
]);
|
|
});
|
|
|
|
it('emits only --no-skills when no forced skills are present on disk', () => {
|
|
expect(buildPiSkillArgs([], {}, fakeSkills, [])).toEqual(['--no-skills']);
|
|
});
|
|
|
|
it('all-skills mode merges the forced set in without duplicating discovered skills', () => {
|
|
expect(buildPiSkillArgs([], { MOSAIC_PI_SKILL_MODE: 'all' }, fakeSkills, fakeForced)).toEqual([
|
|
'--no-skills',
|
|
'--skill',
|
|
'/skills/test-driven-development',
|
|
'--skill',
|
|
'/skills/pdf',
|
|
'--skill',
|
|
'/skills/mosaic-tools',
|
|
]);
|
|
});
|
|
|
|
it('all-skills mode does not double-load a forced skill already discovered', () => {
|
|
expect(
|
|
buildPiSkillArgs([], { MOSAIC_PI_SKILL_MODE: 'all' }, fakeSkills, ['--skill', '/skills/pdf']),
|
|
).toEqual([
|
|
'--no-skills',
|
|
'--skill',
|
|
'/skills/test-driven-development',
|
|
'--skill',
|
|
'/skills/pdf',
|
|
]);
|
|
});
|
|
|
|
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, 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', () => {
|
|
it('defaults to mosaic-tools when MOSAIC_PI_FORCE_SKILLS is unset', () => {
|
|
expect(piForceSkillNames({})).toEqual(['mosaic-tools']);
|
|
});
|
|
|
|
it('treats an empty string as "disable force-loading" (distinct from unset)', () => {
|
|
expect(piForceSkillNames({ MOSAIC_PI_FORCE_SKILLS: '' })).toEqual([]);
|
|
});
|
|
|
|
it('parses a colon list, trimming blanks and whitespace', () => {
|
|
expect(piForceSkillNames({ MOSAIC_PI_FORCE_SKILLS: 'mosaic-tools: mosaic-gitea ::' })).toEqual([
|
|
'mosaic-tools',
|
|
'mosaic-gitea',
|
|
]);
|
|
});
|
|
});
|
|
|
|
describe('registerRuntimeLaunchers — yolo <runtime>', () => {
|
|
let mockExit: MockInstance<typeof process.exit>;
|
|
let mockError: MockInstance<typeof console.error>;
|
|
|
|
beforeEach(() => {
|
|
mockExit = vi.spyOn(process, 'exit').mockImplementation(exitThrows);
|
|
mockError = vi.spyOn(console, 'error').mockImplementation(() => {});
|
|
});
|
|
|
|
afterEach(() => {
|
|
mockExit.mockRestore();
|
|
mockError.mockRestore();
|
|
});
|
|
|
|
it.each(['claude', 'codex', 'opencode', 'pi'] as const)(
|
|
'does NOT pass the runtime name as an extra arg (regression #454) for yolo %s',
|
|
(runtime) => {
|
|
const handler = vi.fn();
|
|
const program = buildProgram(handler);
|
|
program.parse(['node', 'mosaic', 'yolo', runtime]);
|
|
|
|
expect(handler).toHaveBeenCalledTimes(1);
|
|
// The critical assertion: extraArgs must be empty, not [runtime].
|
|
// Before the fix, cmd.args was [runtime] and the runtime name leaked
|
|
// through to the underlying CLI as an initial positional argument.
|
|
expect(handler).toHaveBeenCalledWith(runtime, [], true);
|
|
},
|
|
);
|
|
|
|
it('forwards true excess args after a yolo runtime', () => {
|
|
const handler = vi.fn();
|
|
const program = buildProgram(handler);
|
|
program.parse(['node', 'mosaic', 'yolo', 'claude', '--print', 'hi']);
|
|
|
|
expect(handler).toHaveBeenCalledWith('claude', ['--print', 'hi'], true);
|
|
});
|
|
|
|
it('rejects an unknown runtime under yolo without invoking the handler', () => {
|
|
const handler = vi.fn();
|
|
const program = buildProgram(handler);
|
|
|
|
expect(() => program.parse(['node', 'mosaic', 'yolo', 'bogus'])).toThrow('process.exit called');
|
|
expect(handler).not.toHaveBeenCalled();
|
|
expect(mockExit).toHaveBeenCalledWith(1);
|
|
});
|
|
});
|