From 27b2452860aaf086ff9d61e9bf7f305d607a58e3 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 09:20:56 +0200 Subject: [PATCH 1/5] [US-384] test: mirror-equality guard extended to every dataset skill artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T-1..T-5. Guard was SKILL.md-only (#352); now data-driven over every markdown artifact a dataset skill dir contributes — sub-docs today, references/ tomorrow. - datasetSkillArtifacts: recursive, markdown-only, dataset-derived, no count - installedArtifactPath: root location via the REAL transformPath, cross-checked set-for-set against the pipeline's actual output paths (a path-mapping wrong guess can no longer silently drop an artifact from the assertion) - buildInstalledArtifacts: one real copy-pipeline run -> byDatasetPath + producedPaths; replaces buildInstalledSkillMd - assertRootArtifactMatches: names the artifact by DATASET path + generated root path + compact diff; missing root copy points at 'pair update' - drift-injection on non-SKILL.md artifacts: content, sibling self-pointer, link-depth, /command reference, missing file — each proven RED then green Refs: #384 --- .../src/tools/skill-md-mirror.test.ts | 327 +++++++++++++++--- .../src/tools/skill-md-mirror.ts | 156 +++++++-- 2 files changed, 395 insertions(+), 88 deletions(-) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts index 9cc0b6ed..64ea0888 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts @@ -4,9 +4,11 @@ import { join } from 'path' import { readSkillsDatasetFromDisk, datasetSkillDirs, + datasetSkillArtifacts, installedSkillDir, - buildInstalledSkillMd, - assertRootSkillMdMatches, + installedArtifactPath, + buildInstalledArtifacts, + assertRootArtifactMatches, diffSkillMd, SKILL_COPY_OPTS, type DatasetTree, @@ -17,9 +19,19 @@ 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 +/** + * On-disk root mirror of a dataset artifact, or `undefined` when it is missing + * OR unreadable — an unreadable root copy must be reported as such with its + * path, never swallowed into a pass. + */ +const rootMirrorContent = (datasetArtifact: string): string | undefined => { + const p = join(ROOT_CLAUDE_SKILLS, installedArtifactPath(datasetArtifact)) + if (!existsSync(p)) return undefined + try { + return readFileSync(p, 'utf-8') + } catch { + return undefined + } } /** Runs `fn`, expecting it to throw, and returns the thrown Error's message. */ @@ -33,22 +45,25 @@ const captureThrownMessage = (fn: () => void): string => { } /** - * 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. + * Data-driven mirror-equality guard: for EVERY markdown artifact the dataset + * contributes — each skill's `SKILL.md` AND every sub-doc under a skill dir — + * asserts the root `.claude/skills/` 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 or + * sub-doc is covered automatically with no test edit (AC1/AC3) — never a + * hardcoded list and never a count. */ -describe('per-skill SKILL.md dataset -> root mirror equality (data-driven)', () => { +describe('dataset -> root mirror equality for every skill artifact (data-driven)', () => { const tree = readSkillsDatasetFromDisk(DATASET_SKILLS) const skillDirs = datasetSkillDirs(tree) - let installed: Map + const artifacts = datasetSkillArtifacts(tree) + let mirror: Awaited> // Runs the real copy pipeline over the whole dataset (temp-dir I/O for every // skill), so it needs more than vitest's 10s default hookTimeout — the suite // shares the machine with the rest of the turbo test fan-out. beforeAll(async () => { - installed = await buildInstalledSkillMd(tree) + mirror = await buildInstalledArtifacts(tree) }, 120_000) it('discovers dataset skill dirs directly from disk (no hardcoded count)', () => { @@ -58,45 +73,51 @@ describe('per-skill SKILL.md dataset -> root mirror equality (data-driven)', () 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() - }, - ) + // The derivation `installedArtifactPath` uses to locate each root copy must + // reproduce the pipeline's REAL output set exactly. Without this, a + // path-mapping assumption (e.g. assuming a nested `references/` subdir is + // preserved rather than flattened) would silently drop artifacts from the + // guard: `byDatasetPath` would just lack an entry and nothing would fail. + it('locates every artifact exactly where the pipeline actually wrote it', () => { + expect(artifacts.map(installedArtifactPath).sort()).toEqual(mirror.producedPaths) + expect([...mirror.byDatasetPath.keys()].sort()).toEqual(artifacts) + }) + + it.each(artifacts)('root mirror for %s equals the real copy-pipeline transform', artifact => { + const expected = mirror.byDatasetPath.get(artifact) + expect(expected, `pipeline produced no output for ${artifact}`).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(() => + assertRootArtifactMatches(artifact, expected!, rootMirrorContent(artifact)), + ).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. + * Directional (dataset -> root): the guard iterates DATASET artifacts, so a + * root-only file with no dataset source is never asserted and is NOT treated as + * drift — whether it is a whole root-only skill (`agent-browser`) or a stray + * extra file inside an otherwise dataset-backed skill dir (AC5). */ -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 () => { +describe('directional guard ignores root-only artifacts with no dataset source', () => { + it('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) + const expectedPaths = datasetSkillArtifacts(tree).map(installedArtifactPath) + const { producedPaths } = await buildInstalledArtifacts(tree) - // The guard will assert on EXACTLY these dataset-derived, prefixed dirs — the iteration + // The guard will assert on EXACTLY these dataset-derived paths — 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') + expect(expectedPaths.slice().sort()).toEqual(producedPaths) + expect(expectedPaths).toContain('pair-capability-verify-quality/SKILL.md') + expect(expectedPaths.some(p => p.startsWith('agent-browser/'))).toBe(false) // 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. @@ -108,6 +129,18 @@ describe('directional guard ignores root-only skills with no dataset source', () datasetSkillDirs(readSkillsDatasetFromDisk(DATASET_SKILLS)).map(installedSkillDir), ).not.toContain('agent-browser') }) + + it('ignores a root-only EXTRA file inside a dataset-backed skill dir', async () => { + // The dataset skill dir contributes only SKILL.md; the root copy of that same + // skill also carries a hand-added `scratch.md`. The dataset-derived case list + // never mentions it, so it is not asserted and not drift. + const tree: DatasetTree = { 'process/review/SKILL.md': '---\nname: review\n---\n\n# r\n' } + const { byDatasetPath, producedPaths } = await buildInstalledArtifacts(tree) + + expect(producedPaths).toEqual(['pair-process-review/SKILL.md']) + expect([...byDatasetPath.keys()]).toEqual(['process/review/SKILL.md']) + expect(producedPaths).not.toContain('pair-process-review/scratch.md') + }) }) /** @@ -132,6 +165,78 @@ describe('SKILL_COPY_OPTS stays pinned to the pair-cli skills registry', () => { }) }) +/** + * Artifact enumeration (T-1): a skill dir contributes its `SKILL.md` AND every + * other markdown file under it — sub-docs today, `references/*` tomorrow. The + * enumeration is derived from the dataset tree, recursive, markdown-only, and + * never encodes how many artifacts exist. + */ +describe('datasetSkillArtifacts — every markdown artifact the dataset contributes', () => { + const tree = readSkillsDatasetFromDisk(DATASET_SKILLS) + + it('enumerates SKILL.md AND non-SKILL.md sub-docs of the real dataset (no count)', () => { + const artifacts = datasetSkillArtifacts(tree) + // sub-docs the dataset contributes today, asserted by identity not by count + expect(artifacts).toContain('process/review/merge-and-cascade.md') + expect(artifacts).toContain('process/bootstrap/assess-orchestration.md') + // ...alongside the SKILL.md of EVERY dataset skill dir, derived from the dataset + for (const dir of datasetSkillDirs(tree)) { + expect(artifacts).toContain(`${dir}/SKILL.md`) + } + }) + + it('enumerates nested sub-directories recursively (ready for the first references/)', () => { + const nested: DatasetTree = { + 'process/review/SKILL.md': 'a', + 'process/review/references/deep.md': 'b', + } + expect(datasetSkillArtifacts(nested)).toEqual([ + 'process/review/SKILL.md', + 'process/review/references/deep.md', + ]) + }) + + it('excludes non-markdown files (a stray .DS_Store is never asserted as an artifact)', () => { + const withJunk: DatasetTree = { + 'process/review/SKILL.md': 'a', + 'process/review/.DS_Store': 'binary-ish', + 'process/review/diagram.png': 'png', + } + expect(datasetSkillArtifacts(withJunk)).toEqual(['process/review/SKILL.md']) + }) + + it('returns a sorted list so it.each ordering is stable', () => { + const artifacts = datasetSkillArtifacts(tree) + expect(artifacts).toEqual([...artifacts].sort()) + }) +}) + +/** + * `installedArtifactPath` composes the REAL `transformPath` on the artifact's + * dataset directory, exactly as the copy pipeline's per-file transform does — + * including the flatten of a NESTED sub-directory into its own top-level + * prefixed dir (`process/review/references` → `pair-process-review-references`), + * which is emphatically NOT a preserved `references/` subdir. + */ +describe('installedArtifactPath — root location via the real naming transform', () => { + it('maps a skill-dir artifact into that skill prefixed root dir', () => { + expect(installedArtifactPath('process/review/merge-and-cascade.md')).toBe( + 'pair-process-review/merge-and-cascade.md', + ) + expect(installedArtifactPath('next/SKILL.md')).toBe('pair-next/SKILL.md') + }) + + it('flattens a nested sub-directory into its own prefixed dir, as the pipeline does', () => { + expect(installedArtifactPath('process/review/references/deep.md')).toBe( + 'pair-process-review-references/deep.md', + ) + }) + + it('leaves a dataset-root file unprefixed (no directory to transform)', () => { + expect(installedArtifactPath('README.md')).toBe('README.md') + }) +}) + /** * `diffSkillMd` unit coverage: the compact line-level diff behind the drift * failure message (finding-2 remediation — replaces the full two-file dump). @@ -195,11 +300,12 @@ describe('drift-injection: guard fails on each drift class, passes when reconcil '---\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', } + const SKILL = 'demo/SKILL.md' let expected: string beforeAll(async () => { - const installed = await buildInstalledSkillMd(MINI) - expected = installed.get('pair-demo')! + const { byDatasetPath } = await buildInstalledArtifacts(MINI) + expected = byDatasetPath.get(SKILL)! }) it('the real transform performs all three content rewrites (fixture is meaningful)', () => { @@ -215,20 +321,20 @@ describe('drift-injection: guard fails on each drift class, passes when reconcil }) it('passes when the root mirror equals the real transform (reconciled)', () => { - expect(() => assertRootSkillMdMatches('pair-demo', expected, expected)).not.toThrow() + expect(() => assertRootArtifactMatches(SKILL, expected, expected)).not.toThrow() }) - it('FAILS on frontmatter name: drift (AC2), naming the skill + showing expected vs actual', () => { + it('FAILS on frontmatter name: drift (AC2), naming the artifact + showing expected vs actual', () => { const drifted = expected.replace('name: pair-demo', 'name: demo') - expect(() => assertRootSkillMdMatches('pair-demo', expected, drifted)).toThrow(/drifted/) + expect(() => assertRootArtifactMatches(SKILL, 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 + // AC2 explicitly requires the failure to NAME the offending artifact by its + // dataset-relative path and SHOW expected-vs-actual content — assert the + // message carries all of it, so a regression that dropped the name, the + // generated root path, or the diff dump would fail here. + const message = captureThrownMessage(() => assertRootArtifactMatches(SKILL, expected, drifted)) + expect(message).toContain(SKILL) // names the offending artifact by dataset path + expect(message).toContain('.claude/skills/pair-demo/SKILL.md') // and its generated root path 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 @@ -237,17 +343,128 @@ describe('drift-injection: guard fails on each drift class, passes when reconcil 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/) + expect(() => assertRootArtifactMatches(SKILL, 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/) + expect(() => assertRootArtifactMatches(SKILL, expected, drifted)).toThrow(/drifted/) }) it('FAILS loudly when the root mirror is missing (AC4)', () => { - expect(() => assertRootSkillMdMatches('pair-demo', expected, undefined)).toThrow( + expect(() => assertRootArtifactMatches(SKILL, expected, undefined)).toThrow( /missing[\s\S]*pair update/, ) }) }) + +/** + * Drift-injection for NON-`SKILL.md` artifacts (#384's actual subject): a + * sub-doc and a nested `references/` file go through the SAME real pipeline, so + * every drift class must be caught on them too — not just on `SKILL.md`. The + * fixture is authored so the transform genuinely rewrites the sub-doc content + * (link depth + `/command` reference), which is what makes a hand-edit there + * silently lossy today. + */ +describe('drift-injection on sub-docs and nested references (non-SKILL.md artifacts)', () => { + const SUB = 'demo/sub-doc.md' + const NESTED = 'demo/references/deep.md' + const SUB_ROOT = '.claude/skills/pair-demo/sub-doc.md' + const MINI: DatasetTree = { + 'demo/SKILL.md': '---\nname: demo\ndescription: "d"\n---\n\n# demo\n', + // Authored so the transform performs all three sub-doc rewrite classes: + // sibling self-pointer normalisation (`SKILL.md` -> `./SKILL.md`, the one the + // four real dataset sub-docs exercise), relative-link-depth bump, and the + // `/command` -> `/pair-*` skill-reference rewrite. + [SUB]: + '# sub\n\nDisclosed from [SKILL.md](SKILL.md).\nSee [kb](../../.pair/kb.md).\nCompose /verify-quality first.\nTail line kept unchanged.\nAnother tail line.\nAnd a third.\n', + [NESTED]: '# deep\n\nCompose /verify-quality here.\n', + 'capability/verify-quality/SKILL.md': '---\nname: verify-quality\n---\n\n# vq\n', + // non-markdown noise: copied by the pipeline, never asserted as an artifact + 'demo/.DS_Store': 'junk', + } + let mirror: Awaited> + + beforeAll(async () => { + mirror = await buildInstalledArtifacts(MINI) + }, 120_000) + + it('asserts the sub-doc and the nested reference, and NOT the non-markdown file', () => { + expect([...mirror.byDatasetPath.keys()].sort()).toEqual([ + 'capability/verify-quality/SKILL.md', + 'demo/SKILL.md', + NESTED, + SUB, + ]) + expect(mirror.producedPaths).toContain('pair-demo/sub-doc.md') + // a nested subdir is FLATTENED into its own top-level prefixed dir + expect(mirror.producedPaths).toContain('pair-demo-references/deep.md') + expect(mirror.producedPaths.some(p => p.includes('.DS_Store'))).toBe(false) + }) + + it('the transform genuinely rewrites the sub-doc (fixture is meaningful)', () => { + const sub = mirror.byDatasetPath.get(SUB)! + expect(sub).toContain('](./SKILL.md)') // sibling self-pointer normalised + expect(sub).not.toContain('[SKILL.md](SKILL.md)') + expect(sub).toContain('](../../../.pair/kb.md)') // link-depth bump + expect(sub).not.toContain('](../../.pair/kb.md)') + expect(sub).toContain('/pair-capability-verify-quality') // skill-reference rewrite + expect(sub).not.toContain(' /verify-quality ') + }) + + it('rewrites a nested reference file too (flattened into its own prefixed dir)', () => { + expect(mirror.byDatasetPath.get(NESTED)).toContain('/pair-capability-verify-quality') + }) + + it('passes when the sub-doc root copy equals the real transform (reconciled)', () => { + const sub = mirror.byDatasetPath.get(SUB)! + expect(() => assertRootArtifactMatches(SUB, sub, sub)).not.toThrow() + }) + + it('FAILS on sub-doc content drift, naming it by dataset path + compact diff (AC2)', () => { + const sub = mirror.byDatasetPath.get(SUB)! + const drifted = sub.replace('# sub\n', '# sub (hand-edited)\n') + const message = captureThrownMessage(() => assertRootArtifactMatches(SUB, sub, drifted)) + expect(message).toContain(SUB) + expect(message).toContain(SUB_ROOT) + expect(message).toContain('-# sub') + expect(message).toContain('+# sub (hand-edited)') + // compact diff, not a two-file dump: the unchanged tail is elided + expect(message).toContain('…') + }) + + it('FAILS on sub-doc sibling self-pointer drift (AC2)', () => { + const sub = mirror.byDatasetPath.get(SUB)! + const drifted = sub.replace('](./SKILL.md)', '](SKILL.md)') + expect(() => assertRootArtifactMatches(SUB, sub, drifted)).toThrow(/drifted/) + }) + + it('FAILS on sub-doc link-depth drift (AC2)', () => { + const sub = mirror.byDatasetPath.get(SUB)! + const drifted = sub.replace('](../../../.pair/kb.md)', '](../../.pair/kb.md)') + expect(() => assertRootArtifactMatches(SUB, sub, drifted)).toThrow(/drifted/) + }) + + it('FAILS on sub-doc /command skill-reference drift (AC2)', () => { + const sub = mirror.byDatasetPath.get(SUB)! + const drifted = sub.replace('/pair-capability-verify-quality', '/verify-quality') + expect(() => assertRootArtifactMatches(SUB, sub, drifted)).toThrow(/drifted/) + }) + + it('FAILS loudly when the sub-doc root copy is missing, pointing at pair update (AC4)', () => { + const message = captureThrownMessage(() => + assertRootArtifactMatches(SUB, mirror.byDatasetPath.get(SUB)!, undefined), + ) + expect(message).toMatch(/missing[\s\S]*pair update/) + expect(message).toContain(SUB) + expect(message).toContain(SUB_ROOT) + }) + + it('FAILS on nested reference drift, named by its nested dataset path', () => { + const deep = mirror.byDatasetPath.get(NESTED)! + const drifted = `${deep}stray\n` + const message = captureThrownMessage(() => assertRootArtifactMatches(NESTED, deep, drifted)) + expect(message).toContain(NESTED) + expect(message).toContain('.claude/skills/pair-demo-references/deep.md') + }) +}) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts index f4f5c8a6..8424fc97 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.ts @@ -1,8 +1,9 @@ /** - * Mirror-equality helpers for per-skill `SKILL.md` files. + * Mirror-equality helpers for EVERY root skill artifact. * - * Every root `.claude/skills//SKILL.md` is GENERATED from its - * canonical dataset source `packages/knowledge-hub/dataset/.skills///SKILL.md` + * Every root `.claude/skills/**\/*.md` — each skill's `SKILL.md` and every + * sub-doc / `references/*` its directory contributes — is GENERATED from its + * canonical dataset source under `packages/knowledge-hub/dataset/.skills/` * 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` @@ -15,18 +16,19 @@ * 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. + * Directional (dataset → root): the enumeration is keyed by DATASET artifacts, + * so a root-only file with no dataset source (e.g. the whole `agent-browser` + * skill) 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. + * SCOPE (#384, widening #352): the guard asserts equality for every markdown + * artifact the dataset contributes, not only `SKILL.md`. The case list is + * derived from the dataset at collection time and is recursive, so a new + * sub-doc — or the first `references/` subdir — is covered with no test edit + * and no count anywhere. */ import { readdirSync, readFileSync } from 'fs' import { join, relative, sep } from 'path' +import { dirname as posixDirname } from 'path/posix' import { InMemoryFileSystemService, copyDirectoryWithTransforms, @@ -114,13 +116,50 @@ export function installedSkillDir(datasetSkillDir: string): string { return transformPath(datasetSkillDir, SKILL_COPY_OPTS) } +/** + * Every markdown artifact the dataset contributes through the `pair update` + * transform — each skill's `SKILL.md` AND its sub-docs (today + * `process/review/merge-and-cascade.md` & siblings) — as sorted + * dataset-relative posix paths. + * + * The list is derived from the already-read `tree`, so it is recursive by + * construction: a future `references/` subdir is enumerated the day it lands, + * with no test edit. Markdown-only: a stray `.DS_Store` or a diagram is copied + * by the pipeline but is not a *transformed* artifact, so it is never asserted. + * Nothing here encodes HOW MANY artifacts exist. + */ +export function datasetSkillArtifacts(tree: DatasetTree): string[] { + return Object.keys(tree) + .filter(rel => rel.endsWith('.md')) + .sort() +} + +/** + * Root location of a dataset artifact, relative to `.claude/skills/`. + * + * Composes the REAL `transformPath` over the artifact's dataset directory with + * the registry's options — exactly what the copy pipeline's per-file transform + * does (`dirname(file)` → `transformPath` → join the untouched file name). This + * is why a NESTED sub-directory lands in its OWN flattened top-level dir + * (`process/review/references/deep.md` → `pair-process-review-references/deep.md`) + * rather than under a preserved `references/`: the pipeline flattens every + * directory segment, not just the skill's own. + * + * That correspondence is not taken on trust — a guard test asserts this + * derivation reproduces the pipeline's actual output paths set-for-set. + */ +export function installedArtifactPath(datasetArtifact: string): string { + const dir = posixDirname(datasetArtifact) + const fileName = datasetArtifact.slice(dir === '.' ? 0 : dir.length + 1) + return dir === '.' ? fileName : `${transformPath(dir, SKILL_COPY_OPTS)}/${fileName}` +} + /** * 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. + * dataset. No parallel transform logic — a bug in the production pipeline + * surfaces in the guard instead of being masked. */ -export async function buildInstalledSkillMd(tree: DatasetTree): Promise> { +async function runCopyPipeline(tree: DatasetTree): Promise { const initial: Record = {} for (const [rel, content] of Object.entries(tree)) { initial[`${VIRTUAL_SRC}/${rel}`] = content @@ -141,13 +180,60 @@ export async function buildInstalledSkillMd(tree: DatasetTree): Promise { + const out: string[] = [] + for (const entry of await fileService.readdir(dir)) { + const full = `${dir}/${entry.name}` + if (entry.isDirectory()) { + out.push(...(await collectMarkdownUnder(fileService, full, root))) + } else if (entry.name.endsWith('.md')) { + out.push(full.slice(root.length + 1)) + } + } + return out +} + +/** + * The expected root mirror, produced by ONE run of the real copy pipeline: + * + * - `byDatasetPath` — `datasetArtifactPath → transformed content` for every + * markdown artifact the dataset contributes. Keyed by the DATASET path so a + * drift failure is attributed to its canonical source (AC2), not to the + * generated location. + * - `producedPaths` — every markdown path the pipeline actually wrote, relative + * to `.claude/skills/`. Lets the guard cross-check `installedArtifactPath`'s + * derivation against the pipeline's real output set, so a path-mapping + * assumption (notably for nested sub-directories) can never silently exclude + * an artifact from the assertion. + */ +export type InstalledMirror = { + byDatasetPath: Map + producedPaths: string[] +} + +export async function buildInstalledArtifacts(tree: DatasetTree): Promise { + const fileService = await runCopyPipeline(tree) + + const byDatasetPath = new Map() + for (const artifact of datasetSkillArtifacts(tree)) { + const content = fileService.getContent(`${VIRTUAL_DEST}/${installedArtifactPath(artifact)}`) + // Absent iff the pipeline wrote the artifact somewhere else than + // `installedArtifactPath` derives; the cross-check assertion names it. + if (content !== undefined) byDatasetPath.set(artifact, content) + } - 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 { + byDatasetPath, + producedPaths: (await collectMarkdownUnder(fileService, VIRTUAL_DEST, VIRTUAL_DEST)).sort(), } - return installed } type DiffEdit = { tag: ' ' | '-' | '+'; line: string } @@ -227,34 +313,38 @@ export function diffSkillMd(expected: string, actual: string): string { } /** - * 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. + * Asserts one root mirror artifact — a `SKILL.md` or any sub-doc the same + * `pair update` transform generates — equals the real pipeline output. + * Throws LOUDLY, naming the offending artifact by its DATASET-relative path + * (its canonical identity), pointing at the generated root path, 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. + * requires stays readable even for a large artifact. * * `actual` is `undefined` iff the root mirror file does not exist. */ -export function assertRootSkillMdMatches( - prefixed: string, +export function assertRootArtifactMatches( + datasetArtifact: string, expected: string, actual: string | undefined, ): void { + const rootPath = `.claude/skills/${installedArtifactPath(datasetArtifact)}` 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.`, + `Root mirror missing for dataset artifact '${datasetArtifact}': ` + + `${rootPath} 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` + + `Root mirror for dataset artifact '${datasetArtifact}' has drifted from its dataset ` + + `source transform. Run 'pair update' to regenerate ${rootPath}.\n` + `--- expected (dataset → real transform)\n` + `+++ actual (root mirror on disk)\n` + `${diffSkillMd(expected, actual)}`, From 2c1dd33f535e396f2f32433aa80333a214ed1ce8 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 09:24:24 +0200 Subject: [PATCH 2/5] [US-384] docs: fix stale helper name in drift-injection docblock Refs: #384 --- packages/knowledge-hub/src/tools/skill-md-mirror.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts index 64ea0888..7dde5213 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts @@ -287,7 +287,7 @@ describe('diffSkillMd — compact line-level diff', () => { * 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. + * to simulate a stale root mirror, and `assertRootArtifactMatches` must throw. * * The mini fixture is authored so the real transform performs all three * content rewrites: frontmatter `name:` sync, relative-link-depth bump From 406f8a69855c5c5d20242465b7b518026c6c3650 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 10:28:07 +0200 Subject: [PATCH 3/5] =?UTF-8?q?[US-384]=20chore:=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20no=20vacuous=20assertions,=20reuse=20content-ops=20?= =?UTF-8?q?walk,=20#407=20pointers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - non-vacuous AC5: stray root file really written into the pipeline dest (`InstalledMirror.root` handle) instead of asserted-absent with no fixture; `.DS_Store` proven copied via `root.has` instead of a tautology. - root-copy read: EXISTS-but-unreadable rethrown with path + cause, no longer mislabelled "does not exist / run pair update"; catch branch now covered. - reuse `walkMarkdownFiles` from content-ops (drop local re-walk); `posix.dirname` /`posix.basename` instead of hand-rolled slicing + dual path imports. - nested-flatten mapping labelled a known defect (#407) in docblock + test names. - AC labels normalized to #384 numbering; orphan + non-markdown residuals recorded as explicit decisions in the module docblock. 78 tests in skill-md-mirror.test.ts (542 in the package), lint + ts:check clean. --- .../src/tools/skill-md-mirror.test.ts | 99 +++++++++++++---- .../src/tools/skill-md-mirror.ts | 102 +++++++++++++----- 2 files changed, 154 insertions(+), 47 deletions(-) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts index 7dde5213..8e81c4ff 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts @@ -20,17 +20,29 @@ const DATASET_SKILLS = join(REPO_ROOT, 'packages/knowledge-hub/dataset/.skills') const ROOT_CLAUDE_SKILLS = join(REPO_ROOT, '.claude/skills') /** - * On-disk root mirror of a dataset artifact, or `undefined` when it is missing - * OR unreadable — an unreadable root copy must be reported as such with its - * path, never swallowed into a pass. + * On-disk root mirror of a dataset artifact, or `undefined` ONLY when it is + * genuinely absent (which the guard reports as "missing → run `pair update`"). + * + * A copy that EXISTS but cannot be read (EACCES, a directory in its place) is a + * different failure with a different fix: it is rethrown naming the path and the + * underlying cause, never collapsed into `undefined` — which would mislabel it + * as missing and hand the developer a hint (`pair update`) that cannot fix an + * EACCES. `read` is injectable so that branch is actually covered by a test. */ -const rootMirrorContent = (datasetArtifact: string): string | undefined => { +const rootMirrorContent = ( + datasetArtifact: string, + read: (p: string) => string = p => readFileSync(p, 'utf-8'), +): string | undefined => { const p = join(ROOT_CLAUDE_SKILLS, installedArtifactPath(datasetArtifact)) if (!existsSync(p)) return undefined try { - return readFileSync(p, 'utf-8') - } catch { - return undefined + return read(p) + } catch (err) { + throw new Error( + `Root mirror for dataset artifact '${datasetArtifact}' EXISTS at ${p} but is ` + + `unreadable: ${(err as Error).message}. Fix the file/permissions — ` + + `'pair update' cannot regenerate over an unreadable path.`, + ) } } @@ -87,7 +99,7 @@ describe('dataset -> root mirror equality for every skill artifact (data-driven) const expected = mirror.byDatasetPath.get(artifact) expect(expected, `pipeline produced no output for ${artifact}`).toBeDefined() - // Throws (AC4 missing / AC2+AC3 drift) or passes. The assertion helper + // Throws (AC4 missing / AC2 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(() => @@ -131,15 +143,52 @@ describe('directional guard ignores root-only artifacts with no dataset source', }) it('ignores a root-only EXTRA file inside a dataset-backed skill dir', async () => { - // The dataset skill dir contributes only SKILL.md; the root copy of that same - // skill also carries a hand-added `scratch.md`. The dataset-derived case list - // never mentions it, so it is not asserted and not drift. + // The dataset skill dir contributes only SKILL.md; a hand-added `scratch.md` + // is then REALLY written into the installed dir of that same skill (not just + // asserted-absent — the file exists in the destination tree below), and the + // dataset-derived case list still never mentions it: not asserted, not drift. const tree: DatasetTree = { 'process/review/SKILL.md': '---\nname: review\n---\n\n# r\n' } - const { byDatasetPath, producedPaths } = await buildInstalledArtifacts(tree) + const mirror = await buildInstalledArtifacts(tree) + expect(mirror.producedPaths).toEqual(['pair-process-review/SKILL.md']) + + await mirror.root.write('pair-process-review/scratch.md', '# hand-added, no dataset source\n') + const afterStray = await mirror.root.markdownPaths() + // the stray really is on the installed side now... + expect(afterStray).toContain('pair-process-review/scratch.md') + // ...yet the guard's iteration domain (dataset artifacts) is unchanged, so it + // is never asserted and the guard stays green. + expect([...mirror.byDatasetPath.keys()]).toEqual(['process/review/SKILL.md']) + expect(datasetSkillArtifacts(tree).map(installedArtifactPath)).not.toContain( + 'pair-process-review/scratch.md', + ) + }) + +}) - expect(producedPaths).toEqual(['pair-process-review/SKILL.md']) - expect([...byDatasetPath.keys()]).toEqual(['process/review/SKILL.md']) - expect(producedPaths).not.toContain('pair-process-review/scratch.md') +/** + * Missing vs unreadable are DISTINCT failures with distinct fixes: `pair update` + * regenerates a missing copy, but cannot fix an EACCES. The root-copy read must + * therefore never collapse "unreadable" into "missing" (nor, worse, into a pass). + */ +describe('root-copy read distinguishes a missing copy from an unreadable one', () => { + it('reports an EXISTING but unreadable root copy as unreadable, never as missing', () => { + // The catch branch of the root-copy read: an EACCES (or a dir in its place) + // must fail with its path and cause, not be swallowed into `undefined` and + // mislabelled "does not exist. Run 'pair update'". + const artifact = 'next/SKILL.md' + const rootPath = join(ROOT_CLAUDE_SKILLS, installedArtifactPath(artifact)) + expect(existsSync(rootPath)).toBe(true) // precondition: it DOES exist + + const message = captureThrownMessage(() => + rootMirrorContent(artifact, () => { + throw new Error('EACCES: permission denied') + }), + ) + expect(message).toContain(artifact) + expect(message).toContain(rootPath) + expect(message).toContain('unreadable') + expect(message).toContain('EACCES: permission denied') + expect(message).not.toContain('does not exist') }) }) @@ -217,6 +266,12 @@ describe('datasetSkillArtifacts — every markdown artifact the dataset contribu * including the flatten of a NESTED sub-directory into its own top-level * prefixed dir (`process/review/references` → `pair-process-review-references`), * which is emphatically NOT a preserved `references/` subdir. + * + * That nested mapping is CURRENT pipeline behavior and a KNOWN DEFECT — tracked + * as #407 (the sub-doc installs outside its skill dir, both links broken). These + * expectations pin what the pipeline does today so the guard cannot lie about the + * mirror; they must be flipped to the corrected layout by #407's fix, NOT copied + * as the sanctioned way to ship a `references/` sub-doc. */ describe('installedArtifactPath — root location via the real naming transform', () => { it('maps a skill-dir artifact into that skill prefixed root dir', () => { @@ -226,7 +281,7 @@ describe('installedArtifactPath — root location via the real naming transform' expect(installedArtifactPath('next/SKILL.md')).toBe('pair-next/SKILL.md') }) - it('flattens a nested sub-directory into its own prefixed dir, as the pipeline does', () => { + it('flattens a nested sub-dir into its own prefixed dir, as the pipeline does today (#407)', () => { expect(installedArtifactPath('process/review/references/deep.md')).toBe( 'pair-process-review-references/deep.md', ) @@ -341,12 +396,12 @@ describe('drift-injection: guard fails on each drift class, passes when reconcil expect(message).toContain('+name: demo') // drifted actual content shown as an added diff line }) - it('FAILS on relative-link-depth drift (AC3)', () => { + it('FAILS on relative-link-depth drift (AC2)', () => { const drifted = expected.replace('](../../../.pair/foo.md)', '](../../.pair/foo.md)') expect(() => assertRootArtifactMatches(SKILL, expected, drifted)).toThrow(/drifted/) }) - it('FAILS on /command skill-reference drift (AC3)', () => { + it('FAILS on /command skill-reference drift (AC2)', () => { const drifted = expected.replace('/pair-capability-verify-quality', '/verify-quality') expect(() => assertRootArtifactMatches(SKILL, expected, drifted)).toThrow(/drifted/) }) @@ -397,9 +452,13 @@ describe('drift-injection on sub-docs and nested references (non-SKILL.md artifa SUB, ]) expect(mirror.producedPaths).toContain('pair-demo/sub-doc.md') - // a nested subdir is FLATTENED into its own top-level prefixed dir + // a nested subdir is FLATTENED into its own top-level prefixed dir (current + // pipeline behavior — a defect tracked in #407, mirrored here, not endorsed) expect(mirror.producedPaths).toContain('pair-demo-references/deep.md') - expect(mirror.producedPaths.some(p => p.includes('.DS_Store'))).toBe(false) + // The pipeline REALLY copied the non-markdown file (asserted on the + // destination tree, so this cannot pass vacuously) and the guard's asserted + // set above still excludes it. + expect(mirror.root.has('pair-demo/.DS_Store')).toBe(true) }) it('the transform genuinely rewrites the sub-doc (fixture is meaningful)', () => { diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts index 8424fc97..5aaf7b50 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.ts @@ -24,16 +24,32 @@ * artifact the dataset contributes, not only `SKILL.md`. The case list is * derived from the dataset at collection time and is recursive, so a new * sub-doc — or the first `references/` subdir — is covered with no test edit - * and no count anywhere. + * and no count anywhere. Caveat for that first `references/` subdir: today's + * pipeline INSTALLS it wrongly (flattened out of its skill, links broken) — a + * defect tracked in #407; this guard faithfully mirrors that behavior rather + * than endorsing it (see `installedArtifactPath`). + * + * ACCEPTED RESIDUAL — orphans (decided in #384's review): because the guard is + * directional and `pair update` copies with behavior 'overwrite' (no + * mirror-delete), a sub-doc DELETED from the dataset but left behind in + * `.claude/skills//` is neither asserted nor cleaned, and agents keep + * reading it. Detecting it would need a non-directional check (root `.md` under + * `installedSkillDir(datasetDir)` with no dataset source) that must exempt + * root-only skills like `agent-browser`; that inversion is deliberately NOT in + * this guard, whose contract is "every dataset artifact is faithfully + * mirrored". Regenerating deletions belongs to `pair update`, not here. */ import { readdirSync, readFileSync } from 'fs' -import { join, relative, sep } from 'path' -import { dirname as posixDirname } from 'path/posix' +import { join, relative, sep, posix } from 'path' +// Dataset/root artifact paths are ALWAYS posix (they are content identities, not +// host paths), hence `posix.dirname`/`posix.basename`; only the on-disk dataset +// walk above uses the platform-native `join`/`relative`/`sep`. import { InMemoryFileSystemService, copyDirectoryWithTransforms, transformPath, defaultSyncOptions, + walkMarkdownFiles, } from '@pair/content-ops' /** The exact naming-transform options the `skills` registry uses in config.json. */ @@ -88,7 +104,7 @@ export function readSkillsDatasetFromDisk(skillsDir: string): DatasetTree { * 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). + * dataset extends this list with no test edit (AC3). * * Deliberately NOT reusing `collectSkillDirs` from the sibling * `skills-guide-mirror.ts`: that one re-walks the disk and returns ANY dir @@ -124,9 +140,18 @@ export function installedSkillDir(datasetSkillDir: string): string { * * The list is derived from the already-read `tree`, so it is recursive by * construction: a future `references/` subdir is enumerated the day it lands, - * with no test edit. Markdown-only: a stray `.DS_Store` or a diagram is copied - * by the pipeline but is not a *transformed* artifact, so it is never asserted. - * Nothing here encodes HOW MANY artifacts exist. + * with no test edit. Nothing here encodes HOW MANY artifacts exist. + * + * Markdown-only, for two DIFFERENT reasons — kept apart on purpose: + * 1. junk (a stray `.DS_Store`, an editor backup): not content at all, so it + * must NEVER be asserted — the pipeline copies it, the guard ignores it. + * 2. legitimate non-markdown assets (the `templates/*.sh` pattern the root-only + * `agent-browser` skill already ships): real content whose equality is NOT + * guarded — an ACCEPTED RESIDUAL, out of #384's scope by explicit story + * decision (Edge Cases exclude non-markdown), not an oversight. Only + * markdown goes through the content rewrites this guard exists to pin + * (frontmatter sync, link-depth bump, `/command` rewrite); a byte-equality + * check for opaque assets is a different, simpler guard. */ export function datasetSkillArtifacts(tree: DatasetTree): string[] { return Object.keys(tree) @@ -145,12 +170,20 @@ export function datasetSkillArtifacts(tree: DatasetTree): string[] { * rather than under a preserved `references/`: the pipeline flattens every * directory segment, not just the skill's own. * + * That nested-flatten mapping is CURRENT PIPELINE BEHAVIOR, mirrored here + * faithfully — and it is a DEFECT, tracked in #407 (a skill's `references/` + * sub-doc installs outside its skill dir with both links broken). Do NOT read it + * as a sanctioned layout for a new `references/` sub-doc: when #407 fixes the + * transform, this derivation and its tests follow the corrected mapping + * automatically-by-review (the guard composes the real transform, so the + * expectations here must be updated in that PR). + * * That correspondence is not taken on trust — a guard test asserts this * derivation reproduces the pipeline's actual output paths set-for-set. */ export function installedArtifactPath(datasetArtifact: string): string { - const dir = posixDirname(datasetArtifact) - const fileName = datasetArtifact.slice(dir === '.' ? 0 : dir.length + 1) + const dir = posix.dirname(datasetArtifact) + const fileName = posix.basename(datasetArtifact) return dir === '.' ? fileName : `${transformPath(dir, SKILL_COPY_OPTS)}/${fileName}` } @@ -183,22 +216,17 @@ async function runCopyPipeline(tree: DatasetTree): Promise { - const out: string[] = [] - for (const entry of await fileService.readdir(dir)) { - const full = `${dir}/${entry.name}` - if (entry.isDirectory()) { - out.push(...(await collectMarkdownUnder(fileService, full, root))) - } else if (entry.name.endsWith('.md')) { - out.push(full.slice(root.length + 1)) - } - } - return out +/** + * Every markdown path currently in the pipeline's destination tree, relative to + * `.claude/skills/` and sorted. Reuses `content-ops`' own `walkMarkdownFiles` + * (no parallel traversal — the same principle this guard applies to the copy + * transform); it joins with the platform `sep`, so the result is normalised back + * to the posix identities the rest of this module speaks. + */ +async function producedMarkdownPaths(fileService: InMemoryFileSystemService): Promise { + return (await walkMarkdownFiles(VIRTUAL_DEST, fileService)) + .map(p => p.split(sep).join('/').slice(VIRTUAL_DEST.length + 1)) + .sort() } /** @@ -213,10 +241,24 @@ async function collectMarkdownUnder( * derivation against the pipeline's real output set, so a path-mapping * assumption (notably for nested sub-directories) can never silently exclude * an artifact from the assertion. + * - `root` — a handle on that destination tree, addressed in + * `.claude/skills/`-relative terms. It exists so directionality (AC5) can be + * proven against a REAL filesystem state instead of a tautology: `has` shows + * what the pipeline genuinely wrote (including non-markdown it copies but the + * guard ignores), and `write` + `markdownPaths` let a caller drop a root-only + * file in after the run and re-derive the produced set. */ export type InstalledMirror = { byDatasetPath: Map producedPaths: string[] + root: { + /** True iff the pipeline wrote this root-relative path (markdown or not). */ + has: (rootRelPath: string) => boolean + /** Adds a root-only file to the destination, as a hand-edit would. */ + write: (rootRelPath: string, content: string) => Promise + /** Re-derives the destination's markdown paths (post-mutation included). */ + markdownPaths: () => Promise + } } export async function buildInstalledArtifacts(tree: DatasetTree): Promise { @@ -232,7 +274,13 @@ export async function buildInstalledArtifacts(tree: DatasetTree): Promise fileService.existsSync(`${VIRTUAL_DEST}/${rootRelPath}`), + write: (rootRelPath, content) => + fileService.writeFile(`${VIRTUAL_DEST}/${rootRelPath}`, content), + markdownPaths: () => producedMarkdownPaths(fileService), + }, } } @@ -318,7 +366,7 @@ export function diffSkillMd(expected: string, actual: string): string { * Throws LOUDLY, naming the offending artifact by its DATASET-relative path * (its canonical identity), pointing at the generated root path, 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 + * drifted (AC2). 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. From 8f43e6301604face99f41598584b71bf53a36518 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 10:37:49 +0200 Subject: [PATCH 4/5] [US-384] style: prettier formatting of the round-1 review fixes Formatting-only follow-up to 406f8a69 (walkMarkdownFiles reuse + stray-blank line): prettier's arrow-chain wrap in `producedMarkdownPaths`, no behavior change. 78 tests in skill-md-mirror.test.ts still green. --- packages/knowledge-hub/src/tools/skill-md-mirror.test.ts | 1 - packages/knowledge-hub/src/tools/skill-md-mirror.ts | 7 ++++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts index 8e81c4ff..7e066fa8 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts @@ -162,7 +162,6 @@ describe('directional guard ignores root-only artifacts with no dataset source', 'pair-process-review/scratch.md', ) }) - }) /** diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts index 5aaf7b50..f47b336e 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.ts @@ -225,7 +225,12 @@ async function runCopyPipeline(tree: DatasetTree): Promise { return (await walkMarkdownFiles(VIRTUAL_DEST, fileService)) - .map(p => p.split(sep).join('/').slice(VIRTUAL_DEST.length + 1)) + .map(p => + p + .split(sep) + .join('/') + .slice(VIRTUAL_DEST.length + 1), + ) .sort() } From 4b230cbf7a41b8ec41e750712c61f8a34fccace2 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 11:01:06 +0200 Subject: [PATCH 5/5] =?UTF-8?q?[US-384]=20chore:=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20pin=20copy=20behavior,=20fix=20stale/self-contradic?= =?UTF-8?q?tory=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pin registry `behavior` via new `skillCopySyncOptions()` (resolved options the pipeline actually runs); a flip to 'mirror' now fails the pin test instead of silently invalidating the orphan residual - import comment: `sep` also used by `producedMarkdownPaths`, not only the dataset walk - #407 caveat: derivation goes STALE (pipeline placement changes, not transformPath); the produced-paths cross-check fails loudly — no "automatic" self-correction - `rootMirrorContent`: docblock states why the on-disk read stays test-side while the assertion helper lives in the module Co-Authored-By: Claude Opus 5 --- .../src/tools/skill-md-mirror.test.ts | 29 +++++++++++++- .../src/tools/skill-md-mirror.ts | 40 +++++++++++++++---- 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts index 7e066fa8..4a45dd94 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts @@ -11,6 +11,7 @@ import { assertRootArtifactMatches, diffSkillMd, SKILL_COPY_OPTS, + skillCopySyncOptions, type DatasetTree, } from './skill-md-mirror' @@ -28,6 +29,17 @@ const ROOT_CLAUDE_SKILLS = join(REPO_ROOT, '.claude/skills') * underlying cause, never collapsed into `undefined` — which would mislabel it * as missing and hand the developer a hint (`pair update`) that cannot fix an * EACCES. `read` is injectable so that branch is actually covered by a test. + * + * PLACEMENT (deliberate, and the reason it differs from its sibling): the + * assertion helper `assertRootArtifactMatches` lives in the production module + * because TWO callers must drive the same code path (this real on-disk guard and + * the drift-injection suite). This reader has exactly one caller and one job — + * binding the guard to THIS repo checkout (`ROOT_CLAUDE_SKILLS` + real `fs`), + * i.e. test-environment wiring, not logic any production consumer shares — so it + * stays test-side, with its own covering suite below ("root-copy read + * distinguishes a missing copy from an unreadable one"). Rule of thumb for the next + * guard helper: shared/reusable assertion or derivation → module; "where does + * this checkout keep its files" → test file. */ const rootMirrorContent = ( datasetArtifact: string, @@ -197,19 +209,32 @@ describe('root-copy read distinguishes a missing copy from an unreadable one', ( * 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). + * + * `behavior` is pinned too, via the RESOLVED options the guard actually runs + * (`skillCopySyncOptions`): a registry flip to 'mirror' would otherwise leave the + * guard simulating 'overwrite' with nothing failing (the in-memory destination + * starts empty, so the pipeline's stale-entry cleanup is a no-op), and the + * module's ACCEPTED RESIDUAL for orphans — justified by "behavior 'overwrite', + * no mirror-delete" — would become quietly false. */ 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', () => { + it('flatten/prefix/source/behavior 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 } } + asset_registries: { + skills: { source: string; flatten: boolean; prefix: string; behavior: 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) + // ...and runs the pipeline with the registry's declared copy behavior + expect(skillCopySyncOptions().defaultBehavior).toBe(registry.behavior) + expect(skillCopySyncOptions().flatten).toBe(registry.flatten) + expect(skillCopySyncOptions().prefix).toBe(registry.prefix) }) }) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts index f47b336e..39f14347 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.ts @@ -42,8 +42,11 @@ import { readdirSync, readFileSync } from 'fs' import { join, relative, sep, posix } from 'path' // Dataset/root artifact paths are ALWAYS posix (they are content identities, not -// host paths), hence `posix.dirname`/`posix.basename`; only the on-disk dataset -// walk above uses the platform-native `join`/`relative`/`sep`. +// host paths), hence `posix.dirname`/`posix.basename`. The platform-native +// `join`/`relative`/`sep` are used in exactly two places, both converting AT the +// boundary: `readSkillsDatasetFromDisk` walks the real dataset on disk, and +// `producedMarkdownPaths` normalises `walkMarkdownFiles`' platform-joined output +// back to posix. import { InMemoryFileSystemService, copyDirectoryWithTransforms, @@ -55,6 +58,24 @@ import { /** The exact naming-transform options the `skills` registry uses in config.json. */ export const SKILL_COPY_OPTS = { flatten: true, prefix: 'pair' } as const +/** + * The FULL `SyncOptions` the guard runs the pipeline with: content-ops' defaults + * (`defaultBehavior: 'overwrite'`) overlaid with the registry's flatten/prefix. + * + * Exported so the pin test can assert the RESOLVED behavior still equals the + * registry's declared `behavior`, not just flatten/prefix. Without that pin, a + * registry flip to 'mirror' would leave the guard silently simulating + * 'overwrite' — in an in-memory destination that starts empty the pipeline's + * stale-entry cleanup is a no-op, so no other assertion would notice — and the + * ACCEPTED RESIDUAL above, whose whole justification is "behavior 'overwrite', + * no mirror-delete", would become quietly false. That is the same silent-drift + * class this guard exists to close, one level up. + */ +export function skillCopySyncOptions(): ReturnType & + typeof SKILL_COPY_OPTS { + return { ...defaultSyncOptions(), ...SKILL_COPY_OPTS } +} + const SKILL_FILE = 'SKILL.md' // Virtual (in-memory) dataset layout that FAITHFULLY mirrors the real @@ -173,10 +194,12 @@ export function datasetSkillArtifacts(tree: DatasetTree): string[] { * That nested-flatten mapping is CURRENT PIPELINE BEHAVIOR, mirrored here * faithfully — and it is a DEFECT, tracked in #407 (a skill's `references/` * sub-doc installs outside its skill dir with both links broken). Do NOT read it - * as a sanctioned layout for a new `references/` sub-doc: when #407 fixes the - * transform, this derivation and its tests follow the corrected mapping - * automatically-by-review (the guard composes the real transform, so the - * expectations here must be updated in that PR). + * as a sanctioned layout for a new `references/` sub-doc. Nor does it self-correct: + * #407's fix changes the copy pipeline's per-file PLACEMENT (`copy-directory-transforms.ts` + * maps `dirname(file)` through `transformPath` and joins), not `transformPath` itself, + * so this derivation goes STALE — what actually happens is that the produced-paths + * cross-check (derivation vs. the pipeline's real output set) fails loudly, and + * this function plus its expectations must be updated in that PR. * * That correspondence is not taken on trust — a guard test asserts this * derivation reproduces the pipeline's actual output paths set-for-set. @@ -210,8 +233,9 @@ async function runCopyPipeline(tree: DatasetTree): Promise