diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts new file mode 100644 index 00000000..5a98a49b --- /dev/null +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts @@ -0,0 +1,250 @@ +import { describe, it, expect, beforeAll } from 'vitest' +import { existsSync, readdirSync, readFileSync } from 'fs' +import { join } from 'path' +import { + readSkillsDatasetFromDisk, + datasetSkillDirs, + installedSkillDir, + buildInstalledSkillMd, + assertRootSkillMdMatches, + diffSkillMd, + SKILL_COPY_OPTS, + type DatasetTree, +} from './skill-md-mirror' + +// packages/knowledge-hub/src/tools -> repo root +const REPO_ROOT = join(__dirname, '..', '..', '..', '..') +const DATASET_SKILLS = join(REPO_ROOT, 'packages/knowledge-hub/dataset/.skills') +const ROOT_CLAUDE_SKILLS = join(REPO_ROOT, '.claude/skills') + +const rootMirrorContent = (prefixed: string): string | undefined => { + const p = join(ROOT_CLAUDE_SKILLS, prefixed, 'SKILL.md') + return existsSync(p) ? readFileSync(p, 'utf-8') : undefined +} + +/** Runs `fn`, expecting it to throw, and returns the thrown Error's message. */ +const captureThrownMessage = (fn: () => void): string => { + try { + fn() + } catch (err) { + return (err as Error).message + } + throw new Error('expected the assertion to throw, but it did not') +} + +/** + * Data-driven mirror-equality guard: for EVERY skill dir in the dataset, + * asserts the root `.claude/skills//SKILL.md` is byte-for-byte the + * real `pair update` copy-pipeline transform of its dataset source. The case + * list is derived from the dataset at collection time, so a newly added skill + * is covered automatically with no test edit (AC5) — never a hardcoded count. + */ +describe('per-skill SKILL.md dataset -> root mirror equality (data-driven)', () => { + const tree = readSkillsDatasetFromDisk(DATASET_SKILLS) + const skillDirs = datasetSkillDirs(tree) + let installed: Map + + beforeAll(async () => { + installed = await buildInstalledSkillMd(tree) + }) + + it('discovers dataset skill dirs directly from disk (no hardcoded count)', () => { + expect(skillDirs.length).toBeGreaterThan(0) + expect(skillDirs).toContain('capability/verify-quality') + expect(skillDirs).toContain('process/refine-story') + expect(skillDirs).toContain('next') + }) + + it.each(skillDirs)( + 'root mirror SKILL.md for %s equals the real copy-pipeline transform', + datasetDir => { + const prefixed = installedSkillDir(datasetDir) + const expected = installed.get(prefixed) + expect(expected, `pipeline produced no SKILL.md for ${prefixed}`).toBeDefined() + + // Throws (AC4 missing / AC2+AC3 drift) or passes. The assertion helper + // lives in the production module so this guard and the drift-injection + // tests below exercise the same code path. + expect(() => + assertRootSkillMdMatches(prefixed, expected!, rootMirrorContent(prefixed)), + ).not.toThrow() + }, + ) +}) + +/** + * Directional (dataset -> root): the guard iterates dataset skill dirs, so a + * root-only skill with no dataset source (e.g. `agent-browser`) is never + * asserted and is NOT treated as drift. + */ +describe('directional guard ignores root-only skills with no dataset source', () => { + it('the guard enumerates ONLY the dataset-derived expected set, never a root-only skill', async () => { + // Synthetic dataset that deliberately does NOT contain `agent-browser` (a real root-only skill). + const tree: DatasetTree = { + 'capability/verify-quality/SKILL.md': '---\nname: verify-quality\n---\n\n# vq\n', + 'next/SKILL.md': '---\nname: next\n---\n\n# next\n', + } + // Exercise the guard's actual expected-set construction (not filesystem facts): + const expectedDirs = datasetSkillDirs(tree).map(installedSkillDir) + const installed = await buildInstalledSkillMd(tree) + + // The guard will assert on EXACTLY these dataset-derived, prefixed dirs — the iteration + // domain is dataset -> root, so a root-only skill name is structurally never checked. + expect(expectedDirs.slice().sort()).toEqual([...installed.keys()].sort()) + expect(expectedDirs).toContain('pair-capability-verify-quality') + expect(expectedDirs).not.toContain('agent-browser') + expect([...installed.keys()]).not.toContain('agent-browser') + + // The real root DOES carry agent-browser, yet the dataset-derived expected set above never + // enumerates it (proving direction) while dataset skills DO mirror into the root. + const rootDirs = readdirSync(ROOT_CLAUDE_SKILLS, { withFileTypes: true }) + .filter(e => e.isDirectory()) + .map(e => e.name) + expect(rootDirs).toContain('agent-browser') + expect( + datasetSkillDirs(readSkillsDatasetFromDisk(DATASET_SKILLS)).map(installedSkillDir), + ).not.toContain('agent-browser') + }) +}) + +/** + * SKILL_COPY_OPTS is a local copy of the `skills` asset-registry knobs in + * apps/pair-cli/config.json (what `pair update` actually uses). Pin it so a + * registry change (e.g. prefix `pair` -> `p`) fails HERE — correctly attributed + * — instead of the guard silently computing the wrong root path and blaming the + * mirror / `pair update` (finding: hardcoded duplication of the config). + */ +describe('SKILL_COPY_OPTS stays pinned to the pair-cli skills registry', () => { + it('flatten/prefix/source match apps/pair-cli/config.json asset_registries.skills', () => { + const config = JSON.parse( + readFileSync(join(REPO_ROOT, 'apps/pair-cli/config.json'), 'utf-8'), + ) as { + asset_registries: { skills: { source: string; flatten: boolean; prefix: string } } + } + const registry = config.asset_registries.skills + expect(SKILL_COPY_OPTS.flatten).toBe(registry.flatten) + expect(SKILL_COPY_OPTS.prefix).toBe(registry.prefix) + // the guard reads the dataset from the registry's declared source dir (.skills) + expect(DATASET_SKILLS.endsWith(registry.source)).toBe(true) + }) +}) + +/** + * `diffSkillMd` unit coverage: the compact line-level diff behind the drift + * failure message (finding-2 remediation — replaces the full two-file dump). + * Exercises each edit class directly so a large SKILL.md failure stays readable. + */ +describe('diffSkillMd — compact line-level diff', () => { + it('shows a changed line as a - expected / + actual pair with surrounding context', () => { + const expected = 'a\nb\nc\nd\ne' + const actual = 'a\nb\nX\nd\ne' + const out = diffSkillMd(expected, actual) + expect(out).toContain('-c') + expect(out).toContain('+X') + expect(out).toContain(' b') // unchanged context line kept + expect(out).toContain(' d') + }) + + it('shows pure insertions as + lines (actual longer than expected)', () => { + const out = diffSkillMd('a\nb', 'a\nb\nc\nd') + expect(out).toContain('+c') + expect(out).toContain('+d') + expect(out).not.toContain('-') + }) + + it('shows pure deletions as - lines (expected longer than actual)', () => { + const out = diffSkillMd('a\nb\nc\nd', 'a\nb') + expect(out).toContain('-c') + expect(out).toContain('-d') + expect(out).not.toContain('+') + }) + + it('collapses long runs of unchanged lines with an ellipsis marker', () => { + const many = Array.from({ length: 30 }, (_, i) => `line${i}`).join('\n') + const drifted = many.replace('line15', 'CHANGED') + const out = diffSkillMd(many, drifted) + expect(out).toContain('-line15') + expect(out).toContain('+CHANGED') + expect(out).toContain('…') // distant unchanged lines elided + expect(out).not.toContain('line0\n') // far-from-change context dropped + }) + + it('returns no diff lines for identical input', () => { + // identical input has no changed lines, so nothing is kept — only elision. + expect(diffSkillMd('a\nb\nc', 'a\nb\nc').replace(/…/g, '').trim()).toBe('') + }) +}) + +/** + * Drift-injection: proves the guard FAILS on each drift class the copy + * transform covers, then PASSES once reconciled. A synthetic mini dataset is + * run through the SAME real pipeline; the transformed output is then corrupted + * to simulate a stale root mirror, and `assertRootSkillMdMatches` must throw. + * + * The mini fixture is authored so the real transform performs all three + * content rewrites: frontmatter `name:` sync, relative-link-depth bump + * (`../../` -> `../../../`, triggered by the depth-shifting bare `demo` skill), + * and the `/command` skill-reference rewrite. + */ +describe('drift-injection: guard fails on each drift class, passes when reconciled', () => { + const MINI: DatasetTree = { + 'demo/SKILL.md': + '---\nname: demo\ndescription: "d"\n---\n\n# demo\n\nSee [foo](../../.pair/foo.md). Compose /verify-quality here.\n', + 'capability/verify-quality/SKILL.md': '---\nname: verify-quality\n---\n\n# vq\n', + } + let expected: string + + beforeAll(async () => { + const installed = await buildInstalledSkillMd(MINI) + expected = installed.get('pair-demo')! + }) + + it('the real transform performs all three content rewrites (fixture is meaningful)', () => { + // frontmatter name: sync + expect(expected).toContain('name: pair-demo') + expect(expected).not.toContain('name: demo\n') + // relative-link-depth bump (../../ -> ../../../) + expect(expected).toContain('](../../../.pair/foo.md)') + expect(expected).not.toContain('](../../.pair/foo.md)') + // /command skill-reference rewrite + expect(expected).toContain('/pair-capability-verify-quality') + expect(expected).not.toContain(' /verify-quality ') + }) + + it('passes when the root mirror equals the real transform (reconciled)', () => { + expect(() => assertRootSkillMdMatches('pair-demo', expected, expected)).not.toThrow() + }) + + it('FAILS on frontmatter name: drift (AC2), naming the skill + showing expected vs actual', () => { + const drifted = expected.replace('name: pair-demo', 'name: demo') + expect(() => assertRootSkillMdMatches('pair-demo', expected, drifted)).toThrow(/drifted/) + + // AC2 explicitly requires the failure to NAME the offending skill and SHOW + // expected-vs-actual content — assert the message carries all of it, so a + // regression that dropped the skill name or the diff dump would fail here. + const message = captureThrownMessage(() => + assertRootSkillMdMatches('pair-demo', expected, drifted), + ) + expect(message).toMatch(/pair-demo/) // names the offending skill + expect(message).toContain('--- expected') // labelled expected side of the diff + expect(message).toContain('+++ actual') // labelled actual side of the diff + expect(message).toContain('-name: pair-demo') // expected content shown as a removed diff line + expect(message).toContain('+name: demo') // drifted actual content shown as an added diff line + }) + + it('FAILS on relative-link-depth drift (AC3)', () => { + const drifted = expected.replace('](../../../.pair/foo.md)', '](../../.pair/foo.md)') + expect(() => assertRootSkillMdMatches('pair-demo', expected, drifted)).toThrow(/drifted/) + }) + + it('FAILS on /command skill-reference drift (AC3)', () => { + const drifted = expected.replace('/pair-capability-verify-quality', '/verify-quality') + expect(() => assertRootSkillMdMatches('pair-demo', expected, drifted)).toThrow(/drifted/) + }) + + it('FAILS loudly when the root mirror is missing (AC4)', () => { + expect(() => assertRootSkillMdMatches('pair-demo', expected, undefined)).toThrow( + /missing[\s\S]*pair update/, + ) + }) +}) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts new file mode 100644 index 00000000..f4f5c8a6 --- /dev/null +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.ts @@ -0,0 +1,263 @@ +/** + * Mirror-equality helpers for per-skill `SKILL.md` files. + * + * Every root `.claude/skills//SKILL.md` is GENERATED from its + * canonical dataset source `packages/knowledge-hub/dataset/.skills///SKILL.md` + * by `pair update`. Rather than re-implement that transform, these helpers run + * the REAL copy pipeline — `copyDirectoryWithTransforms` with the exact + * `{ flatten: true, prefix: 'pair' }` options `apps/pair-cli/config.json` + * declares for the `skills` registry — over an in-memory clone of the dataset, + * so a bug in the real pipeline FAILS the guard instead of being masked. + * + * The composed transform covers all four drift classes the guard must catch: + * dir-rename (`transformPath`), frontmatter `name:` sync (`syncFrontmatter`), + * relative-link-depth rewrite (`rewriteLinksAfterTransform`, the `../../`→`../../../` + * bump that the depth-shifting bare `next` skill triggers), and the `/command` + * skill-reference rewrite (`rewriteSkillReferencesInFiles`). + * + * Directional (dataset → root): the map is keyed by dataset skill dirs, so a + * root-only skill with no dataset source (e.g. `agent-browser`) is never + * asserted — it is not drift. + * + * SCOPE (by design, per #352): the guard asserts equality for each skill's + * `SKILL.md` ONLY. Other artifacts a skill dir contributes through the same + * `pair update` transform — sub-docs (e.g. process-review's `merge-and-cascade.md`) + * and `references/*` — are NOT asserted here; extending the invariant to all + * root skill artifacts is tracked as follow-up tech-debt story #384. + */ +import { readdirSync, readFileSync } from 'fs' +import { join, relative, sep } from 'path' +import { + InMemoryFileSystemService, + copyDirectoryWithTransforms, + transformPath, + defaultSyncOptions, +} from '@pair/content-ops' + +/** The exact naming-transform options the `skills` registry uses in config.json. */ +export const SKILL_COPY_OPTS = { flatten: true, prefix: 'pair' } as const + +const SKILL_FILE = 'SKILL.md' + +// Virtual (in-memory) dataset layout that FAITHFULLY mirrors the real +// `pair update` skills-registry paths, not just a convenient shallow layout. +// The real run uses datasetRoot = baseTarget = repo root and a DEEP source +// (`packages/knowledge-hub/dataset/.skills`), so its `sourceContentRoot` +// (= dirname of the source-relative path) is non-trivial and the link +// rewriter's `reRootTarget` branch executes. Seeding the source at the same +// deep path here (rather than a shallow `/ds/.skills`, whose sourceContentRoot +// collapses to '.') means the guard actually DRIVES that re-rooting branch, so +// a pipeline bug isolated to `reRootTarget`/`sourceContentRoot` re-rooting also +// fails the guard. +const VIRTUAL_DATASET_ROOT = '/ds' +const VIRTUAL_SOURCE_REL = 'packages/knowledge-hub/dataset/.skills' +const VIRTUAL_TARGET_REL = '.claude/skills' +const VIRTUAL_SRC = `${VIRTUAL_DATASET_ROOT}/${VIRTUAL_SOURCE_REL}` +const VIRTUAL_DEST = `${VIRTUAL_DATASET_ROOT}/${VIRTUAL_TARGET_REL}` + +/** Posix-relative path under `.skills/` → file content. */ +export type DatasetTree = Record + +/** + * Reads the on-disk dataset `.skills` tree into a posix-keyed content map. + * Keys are paths relative to `skillsDir` (e.g. `capability/verify-quality/SKILL.md`, + * `next/SKILL.md`), so the map is a portable snapshot of the dataset source. + */ +export function readSkillsDatasetFromDisk(skillsDir: string): DatasetTree { + const tree: DatasetTree = {} + + const walk = (dir: string): void => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = join(dir, entry.name) + if (entry.isDirectory()) { + walk(full) + } else { + const rel = relative(skillsDir, full).split(sep).join('/') + tree[rel] = readFileSync(full, 'utf-8') + } + } + } + + walk(skillsDir) + return tree +} + +/** + * The dataset skill directories — every posix dir that directly contains a + * `SKILL.md` (`capability/`, `process/`, or the bare `next`), + * sorted for stable `it.each` ordering. Data-driven: adding a skill to the + * dataset extends this list with no test edit (AC5). + * + * Deliberately NOT reusing `collectSkillDirs` from the sibling + * `skills-guide-mirror.ts`: that one re-walks the disk and returns ANY dir + * holding at least one file, whereas this one enumerates only SKILL.md-bearing + * dirs off the already-read in-memory `tree` — no second disk walk, and a + * `references/`-only subdir is never mistaken for a skill's own directory. + */ +export function datasetSkillDirs(tree: DatasetTree): string[] { + const dirs = new Set() + for (const rel of Object.keys(tree)) { + if (rel.endsWith(`/${SKILL_FILE}`)) { + dirs.add(rel.slice(0, -`/${SKILL_FILE}`.length)) + } + } + return [...dirs].sort() +} + +/** + * Installed (prefixed) directory name for a dataset skill dir, via the real + * `transformPath` with the registry's flatten/prefix options + * (`capability/verify-quality` → `pair-capability-verify-quality`, + * `next` → `pair-next`). + */ +export function installedSkillDir(datasetSkillDir: string): string { + return transformPath(datasetSkillDir, SKILL_COPY_OPTS) +} + +/** + * Runs the REAL `pair update` copy pipeline over an in-memory clone of the + * dataset and returns `installedPrefixedDir → transformed SKILL.md content` + * for every dataset skill dir. No parallel transform logic — a bug in the + * production pipeline surfaces here. + */ +export async function buildInstalledSkillMd(tree: DatasetTree): Promise> { + const initial: Record = {} + for (const [rel, content] of Object.entries(tree)) { + initial[`${VIRTUAL_SRC}/${rel}`] = content + } + + const fileService = new InMemoryFileSystemService(initial, '/', '/') + await copyDirectoryWithTransforms({ + fileService, + srcPath: VIRTUAL_SRC, + destPath: VIRTUAL_DEST, + // Deep source + repo-root dataset root, exactly as the real `pair update` + // resolves them: this makes `sourceContentRoot` non-trivial so the + // link-rewriter's `reRootTarget` branch runs (see VIRTUAL_* note above). + source: VIRTUAL_SOURCE_REL, + target: VIRTUAL_TARGET_REL, + datasetRoot: VIRTUAL_DATASET_ROOT, + // Same SyncOptions the `skills` registry resolves to: default sync options + // (behavior 'overwrite') with the registry's flatten/prefix applied. + options: { ...defaultSyncOptions(), ...SKILL_COPY_OPTS }, + }) + + const installed = new Map() + for (const datasetDir of datasetSkillDirs(tree)) { + const prefixed = installedSkillDir(datasetDir) + installed.set(prefixed, await fileService.readFile(`${VIRTUAL_DEST}/${prefixed}/${SKILL_FILE}`)) + } + return installed +} + +type DiffEdit = { tag: ' ' | '-' | '+'; line: string } + +/** + * Minimal line edit-script for `a` → `b` via an LCS table (suffix form): + * ` ` unchanged, `-` only in `a` (expected), `+` only in `b` (actual). + */ +function lineEditScript(a: string[], b: string[]): DiffEdit[] { + const m = a.length + const n = b.length + const lcs: number[][] = Array.from({ length: m + 1 }, () => new Array(n + 1).fill(0)) + for (let i = m - 1; i >= 0; i--) { + for (let j = n - 1; j >= 0; j--) { + lcs[i]![j] = + a[i] === b[j] ? lcs[i + 1]![j + 1]! + 1 : Math.max(lcs[i + 1]![j]!, lcs[i]![j + 1]!) + } + } + + const edits: DiffEdit[] = [] + let i = 0 + let j = 0 + while (i < m && j < n) { + // i < m and j < n guarantee both are defined (noUncheckedIndexedAccess). + const ai = a[i]! + const bj = b[j]! + if (ai === bj) { + edits.push({ tag: ' ', line: ai }) + i++ + j++ + } else if (lcs[i + 1]![j]! >= lcs[i]![j + 1]!) { + edits.push({ tag: '-', line: ai }) + i++ + } else { + edits.push({ tag: '+', line: bj }) + j++ + } + } + for (const line of a.slice(i)) edits.push({ tag: '-', line }) + for (const line of b.slice(j)) edits.push({ tag: '+', line }) + return edits +} + +/** + * Compact line-level diff of `expected` vs `actual`, showing only the changed + * lines with a little surrounding context rather than dumping both files in + * full — keeps a drift failure readable for a large SKILL.md instead of + * flooding CI with two complete copies. `-` lines are `expected` (dataset → + * real transform), `+` lines are `actual` (root mirror on disk); collapsed + * runs of unchanged context are shown as ` …`. + */ +export function diffSkillMd(expected: string, actual: string): string { + const edits = lineEditScript(expected.split('\n'), actual.split('\n')) + + // Keep only changed lines plus a little context around each; collapse the rest. + const CONTEXT = 2 + const keep = new Array(edits.length).fill(false) + edits.forEach((e, idx) => { + if (e.tag === ' ') return + for (let k = Math.max(0, idx - CONTEXT); k <= Math.min(edits.length - 1, idx + CONTEXT); k++) { + keep[k] = true + } + }) + + const out: string[] = [] + let elided = false + edits.forEach((e, idx) => { + if (keep[idx]) { + out.push(`${e.tag}${e.line}`) + elided = false + } else if (!elided) { + out.push(' …') + elided = true + } + }) + return out.join('\n') +} + +/** + * Asserts the root mirror `SKILL.md` equals the real pipeline transform. + * Throws LOUDLY — naming the skill and giving the `pair update` regenerate + * hint — when the mirror is missing (AC4) or has drifted (AC2/AC3). This is + * the guard's assertion helper, kept in a tested production module (per the + * "gate & tooling code in tested modules" ADL) so both the real on-disk guard + * and the drift-injection tests drive the same code path. + * + * On drift the message carries a compact line-level diff (via `diffSkillMd`) + * rather than a full dump of both files, so the expected-vs-actual view AC2 + * requires stays readable even for a large SKILL.md. + * + * `actual` is `undefined` iff the root mirror file does not exist. + */ +export function assertRootSkillMdMatches( + prefixed: string, + expected: string, + actual: string | undefined, +): void { + if (actual === undefined) { + throw new Error( + `Root mirror SKILL.md missing for skill '${prefixed}': ` + + `.claude/skills/${prefixed}/SKILL.md does not exist. Run 'pair update' to regenerate it.`, + ) + } + if (actual !== expected) { + throw new Error( + `Root mirror SKILL.md for skill '${prefixed}' has drifted from its dataset source ` + + `transform. Run 'pair update' to regenerate .claude/skills/${prefixed}/SKILL.md.\n` + + `--- expected (dataset → real transform)\n` + + `+++ actual (root mirror on disk)\n` + + `${diffSkillMd(expected, actual)}`, + ) + } +}