fix(#824): enforce exact skill link ownership
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful

This commit is contained in:
ms-lead-reviewer
2026-07-17 18:34:06 -05:00
parent d014a7830a
commit 2db67ed572
7 changed files with 91 additions and 8 deletions

View File

@@ -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. - 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. - 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. - 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.

View File

@@ -242,14 +242,10 @@ prune_stale_links_in_target() {
continue continue
fi fi
resolved="$(readlink -f "$link_path" 2>/dev/null || true)" # -m resolves lexical dangling targets too. If resolution fails, ownership
if [[ -z "$resolved" ]]; then # is unproven and the link must be preserved.
rm -f "$link_path" resolved="$(readlink -m "$link_path" 2>/dev/null || true)"
echo "[mosaic-skills] Removed stale broken skill link: $link_path" if [[ -n "$resolved" && "$resolved" == "$canonical_real/"* ]]; then
continue
fi
if [[ "$resolved" == "$MOSAIC_HOME/"* ]]; then
rm -f "$link_path" rm -f "$link_path"
echo "[mosaic-skills] Removed stale retired skill link: $link_path" echo "[mosaic-skills] Removed stale retired skill link: $link_path"
fi fi

View File

@@ -53,6 +53,7 @@
}, },
"devDependencies": { "devDependencies": {
"@types/node": "^22.0.0", "@types/node": "^22.0.0",
"@vitest/coverage-v8": "^2.0.0",
"@types/react": "^18.3.0", "@types/react": "^18.3.0",
"tsx": "^4.0.0", "tsx": "^4.0.0",
"typescript": "^5.8.0", "typescript": "^5.8.0",

View File

@@ -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<void> {
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', () => { describe('registerSkill', () => {
it('creates the exact canonical symlink and is idempotent', () => { it('creates the exact canonical symlink and is idempotent', () => {
createSkill('new-skill'); createSkill('new-skill');

View File

@@ -88,6 +88,7 @@ export function validateSkillName(name: string): void {
if ( if (
name.length === 0 || name.length === 0 ||
name.startsWith('-') || name.startsWith('-') ||
name.endsWith('.') ||
name.includes('..') || name.includes('..') ||
name.includes('/') || name.includes('/') ||
name.includes('\\') || name.includes('\\') ||
@@ -230,6 +231,13 @@ export function unregisterSkill(
const targetPath = resolveLinkTarget(linkPath); const targetPath = resolveLinkTarget(linkPath);
if (!isInsideSkillsRoot(targetPath, paths.mosaicSkillsDir)) throw foreignTargetError(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); unlinkSync(linkPath);
return { name, status: 'unregistered', linkPath }; return { name, status: 'unregistered', linkPath };
} }

View File

@@ -5,5 +5,16 @@ export default defineConfig({
globals: true, globals: true,
environment: 'node', environment: 'node',
testTimeout: 30_000, testTimeout: 30_000,
coverage: {
provider: 'v8',
include: ['src/commands/skill.ts'],
reporter: ['text', 'json-summary'],
thresholds: {
statements: 85,
branches: 85,
functions: 85,
lines: 85,
},
},
}, },
}); });

3
pnpm-lock.yaml generated
View File

@@ -607,6 +607,9 @@ importers:
'@types/react': '@types/react':
specifier: ^18.3.0 specifier: ^18.3.0
version: 18.3.28 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: tsx:
specifier: ^4.0.0 specifier: ^4.0.0
version: 4.21.0 version: 4.21.0