[US-352] chore: extend mirror-equality guard to per-skill SKILL.md pairs

[rucka]

PR Title: [US-352] chore: extend mirror-equality guard to per-skill SKILL.md pairs
Story/Epic: Closes #352
Type: Chore (tech-debt)
Priority: P2

Summary

What Changed

Adds a consolidated, data-driven it.each guard (packages/knowledge-hub/src/tools/skill-md-mirror.test.ts + co-located production helper skill-md-mirror.ts) asserting every root .claude/skills/<prefixed>/SKILL.md is byte-for-byte the real pair update copy-pipeline transform of its dataset source (packages/knowledge-hub/dataset/.skills/<cat>/<name>/SKILL.md).

Why This Change

Per-skill SKILL.md drift between the dataset (canonical source) and the installed root mirror was previously caught by hand (PR #350). This guard closes that structural gap by making the invariant CI-enforced, additive, and non-blocking (R7.2 tech-debt tier).

Story Context

User Story: As a Pair maintainer/reviewer, I want an automated mirror-equality guard for per-skill SKILL.md so drift is caught by CI instead of manual review.
Acceptance Criteria: AC1-AC5 all covered — see Testing section below for the mapping.

Note: the story title says "all 36 skills" — that count is stale. The dataset currently has 39 skill dirs (root .claude/skills has 40, +1 for agent-browser which has no dataset source). The guard is data-driven (iterates datasetSkillDirs() derived from the dataset tree at collection time), so it asserts against whatever count currently exists and needs no edit when skills are added/removed (AC5).

Changes Made

Implementation Details

• Real pipeline reuse, no re-implementation: buildInstalledSkillMd() seeds an InMemoryFileSystemService from the on-disk dataset .skills tree and runs the REAL copyDirectoryWithTransforms from @pair/content-ops with the exact { flatten: true, prefix: 'pair' } options apps/pair-cli/config.json declares for the skills registry — composing dir-rename (transformPath), frontmatter name: sync (syncFrontmatter), relative-link-depth rewrite (rewriteLinksAfterTransform), and /command skill-reference rewrite (rewriteSkillReferencesInFiles) in one real transform, so a bug in the production pipeline fails the guard instead of being masked by a parallel re-implementation.
• Data-driven case list: datasetSkillDirs() derives every dataset skill dir (capability/<name>, process/<name>, bare next) directly from the dataset tree read off disk — no hardcoded count, sorted for stable it.each ordering.
• Directional (dataset → root), not bijective: the guard iterates dataset skill dirs only; a root-only skill with no dataset source (agent-browser) is never asserted and is explicitly tested as NOT drift.
• Loud failure assertion helper in a tested production module: assertRootSkillMdMatches() lives in skill-md-mirror.ts (not inline in the test) per the "gate & tooling code in tested modules" ADL — both the real on-disk guard and the drift-injection tests exercise the same code path. Throws naming the skill + a pair update regenerate hint on missing mirror (AC4) or content drift (AC2/AC3).

Files Changed

• Added: packages/knowledge-hub/src/tools/skill-md-mirror.ts (production helper: dataset reader, dataset-dir discovery, real-pipeline runner, assertion helper)
• Added: packages/knowledge-hub/src/tools/skill-md-mirror.test.ts (consolidated it.each guard + drift-injection suite)

No other files touched — confined to packages/knowledge-hub per the story's DoD scope constraint (no changes to content-ops, pair-cli, templates, other skills, or .claude/skills).

Testing

Test Coverage

• Unit Tests: 47 new tests in skill-md-mirror.test.ts (data-driven it.each over all current dataset skill dirs + directional edge case + 6 drift-injection cases)
• Manual Testing: locally re-ran the suite after temporarily corrupting a root mirror file to confirm the guard fails loudly, then reverted — in addition to the drift-injection suite that does this programmatically

AC → Test Mapping

AC	Covered by
AC1 (every dataset skill passes, byte-for-byte, when in sync)	it.each(skillDirs) main guard
AC2 (frontmatter name: drift fails)	drift-injection: FAILS on frontmatter name: drift
AC3 (link-depth / /command drift fails)	drift-injection: FAILS on relative-link-depth drift, FAILS on /command skill-reference drift
AC4 (missing root mirror fails loudly)	drift-injection: FAILS loudly when the root mirror is missing
AC5 (new dataset skill covered automatically)	datasetSkillDirs() reads the dataset tree at collection time — no hardcoded count; asserted via discovers dataset skill dirs directly from disk
Directional business rule (agent-browser not drift)	directional guard ignores root-only skills with no dataset source

Test Results

Test Suite: ✅ Passing — pnpm --filter @pair/knowledge-hub test (415 tests, 47 new)
Lint: ✅ Clean — pnpm lint --filter @pair/knowledge-hub
Type-check: ✅ Clean — pnpm ts:check (packages/knowledge-hub)

Quality Assurance

Code Quality Checklist

• Code follows established style guides and conventions
• Functions/classes documented (module-level rationale comment explains why the skills-guide mirror helpers alone were insufficient for per-skill SKILL.md, which changes directory depth on copy)
• No debugging code or console logs left behind (the content-ops copy pipeline emits its own informational logs during the in-memory run; not something this PR controls)

Risk Assessment

Classification carried over from refinement: risk:yellow (KB-default service criticality, no Criticality Table) · cost:green (isolated, test-only, reuses existing exported content-ops transforms — no production behavior change).

Risk	Impact	Probability	Mitigation
Re-implementing the transform diverges from the real pipeline	Medium	Medium	Drives the real copyDirectoryWithTransforms via in-memory FS — no parallel logic
Hardcoded skill count going stale again (story title said "36", actual is 39)	Low	Medium	datasetSkillDirs() iterates the dataset tree at collection time; never a fixed count
Root-only skills (agent-browser) misread as drift	Medium	High	Directional dataset→root iteration; explicit test asserts it's ignored

Reviewer Guide

Review Focus Areas

1. Confirm the guard genuinely drives the production copyDirectoryWithTransforms (no parallel transform logic reimplemented) — see buildInstalledSkillMd() in skill-md-mirror.ts.
2. Confirm directionality: dataset → root only, agent-browser correctly excluded.
3. Confirm the drift-injection suite exercises all 4 drift classes named in the ACs (frontmatter, link-depth, /command, missing mirror) through the same assertRootSkillMdMatches helper the real guard uses.

Testing the Changes

git checkout chore/US-352-skill-md-mirror-equality-guard
pnpm --filter @pair/knowledge-hub test

Key Test Scenarios

1. Main guard: every current dataset skill (39) passes against the on-disk root mirror.
2. Directional edge case: agent-browser present in root but absent from dataset is not flagged.
3. Drift-injection: synthetic demo skill run through the real pipeline, then corrupted per drift class, proving the guard fails then passes when reconciled.

Dependencies & Related Work

Related PRs

• Related: [#249] feat: pair-next --root + --filter scoped selection #350 (the manual fix this guard now automates), extends skills-guide-mirror.test.ts's existing mirror-equality pattern to a new artifact set.

Notes for reviewer (not visible elsewhere)

• Scope directive constrained this PR to packages/knowledge-hub/src/tools only — no content-ops, pair-cli, process-review, templates, pair-next, or .claude/skills changes, consistent with the story's DoD.
• No ADR/ADL needed: this is additive test tooling reusing already-exported @pair/content-ops functions, no new architectural decision.

[rucka]

Code Review — PR #377

Review Information

PR Number: #377
Author: (branch chore/US-352-skill-md-mirror-equality-guard)
Reviewer: Independent reviewer (/pair-process-review)
Review Date: 2026-07-24
Story/Epic: US-352 (#352) — tech-debt: extend mirror-equality guard to per-skill SKILL.md
Review Type: Refactor / test-tooling (tech-debt, R7.2, non-blocking)
Review head: ca6cb4e

Review Summary

Overall Assessment

• Approved with Comments — all acceptance criteria met, all gates green; only minor, non-blocking observations noted.

Key Changes Summary

Two new, additive files under packages/knowledge-hub/src/tools/:

• skill-md-mirror.ts — production helper: on-disk dataset reader, dataset-dir discovery, an in-memory runner of the real @pair/content-ops copyDirectoryWithTransforms pipeline ({ flatten: true, prefix: 'pair' }), and a loud assertRootSkillMdMatches helper.
• skill-md-mirror.test.ts — one consolidated it.each guard over every dataset skill dir + directional edge case (agent-browser) + a 6-case drift-injection suite.

Business Value Validation

Closes the structural gap that PR #350 patched by hand: per-skill SKILL.md drift between the canonical dataset and the installed root mirror is now CI-enforced, data-driven, and directional. Verified locally (see below).

Verification Performed (independent)

Ran in a detached worktree pinned to the PR head (ca6cb4e), after an offline install + building @pair/content-ops:

skill-md-mirror.test.ts .......... 47/47 PASS
@pair/knowledge-hub full suite .... 415/415 PASS (19 files)
lint (eslint) ..................... exit 0
ts:check (tsc --noEmit) ........... exit 0
git diff --name-only main...HEAD .. only the 2 new files (scope confined ✓)

The it.each byte-for-byte equality passing across all 39 dataset skills is direct empirical evidence the guard reproduces the real pipeline output for the current corpus.

AC → verified

AC	Verdict	Evidence
AC1 byte-for-byte for every dataset skill	✅	it.each(skillDirs) — 39 cases pass
AC2 frontmatter name: drift fails	✅	drift-injection case throws /drifted/
AC3 link-depth + /command drift fails	✅	two drift-injection cases pass
AC4 missing mirror fails loudly	✅	throws /missing[\s\S]*pair update/
AC5 data-driven, no hardcoded count	✅	datasetSkillDirs() reads tree at collection; length>0 guard blocks empty false-green
Directional (agent-browser ignored)	✅	dedicated test
Faithful transform (no re-impl)	✅ (see MIN-1 caveat)	drives real copyDirectoryWithTransforms

Detailed Review Comments

Positive Feedback

• Genuinely drives the real production pipeline via an in-memory FS — no parallel transform re-implementation. A pipeline regression surfaces here (with the one branch caveat in MIN-1).
• Empty-glob false-green is explicitly defended (skillDirs.length > 0 + named anchor assertions).
• Drift-injection fixture is well-constructed: it forces all three content rewrites (frontmatter sync, link-depth bump, /command) and self-verifies that it does so before testing failure.
• Directionality (dataset → root) is correct and explicitly tested.
• Scope is exactly as the DoD demands — two additive files, zero production-behavior change.

Minor Issues 💡

• MIN-1 — skill-md-mirror.ts:buildInstalledSkillMd (virtual /ds layout) — The guard seeds the dataset at /ds/.skills → /ds/.claude/skills, so sourceContentRoot = dirname('.skills') = '.' and is never passed. The real pair update runs with datasetRoot = baseTarget (repo root) and a deep source (packages/knowledge-hub/dataset/.skills), so its sourceContentRoot is non-trivial and the link-rewriter's reRootTarget branch executes. For the current corpus the two converge to identical relative links (verified byte-for-byte), so this is not a current bug — but the guard does not exercise the reRootTarget / sourceContentRoot re-rooting path, so a pipeline bug isolated to that branch would not be caught. This narrows the "a bug in the production pipeline fails the guard" claim slightly. Recommendation: either note the limitation in the module doc, or seed the virtual dataset at a nested path (e.g. /ds/pkg/dataset/.skills → /ds/.claude/skills) to also drive reRootTarget.

• MIN-2 — skill-md-mirror.test.ts drift assertions (.toThrow(/drifted/)) — AC2 requires the failure to name the skill and show expected-vs-actual. assertRootSkillMdMatches does include both, but the tests only match /drifted/, so a regression that dropped the skill name or the expected/actual diff from the message would still pass. Recommendation: strengthen at least one drift case to also assert the skill name (/pair-demo/) and evidence of the expected-vs-actual dump.

• MIN-3 — coverage: SKILL.md only (auxiliary skill files unguarded) — Multi-file skills mirror auxiliary files the same pipeline transforms (pair-process-review/{degradation-levels,merge-and-cascade}.md, pair-process-bootstrap/assess-orchestration.md, pair-process-implement/post-review-merge.md). The guard asserts only SKILL.md, so drift in those files is uncaught. This is within this story's stated AC scope (AC1-AC5 are SKILL.md-specific by design) and is a distinct invariant — deferred to tech-debt: extend per-skill mirror-equality guard to auxiliary skill files (not just SKILL.md) #379. Non-blocking.

Questions ❓

• Q-1 — skill-md-mirror.ts:datasetSkillDirs — The story's Dependencies suggested reusing collectSkillDirs from skills-guide-mirror.ts; the PR instead adds readSkillsDatasetFromDisk + datasetSkillDirs. This reads as justified (the guard needs file contents to seed the in-memory FS, must also work on the in-memory MINI fixture where a disk-walk can't run, and keying on SKILL.md is more precise than collectSkillDirs' any-file heuristic). Flagging only for transparency — not a defect.

Nit

• N-1 — test stdout noise — the in-memory pipeline run emits many ℹ️ Skill reference rewriter / Copied contents / Link rewriter lines. Harmless, pre-existing @pair/content-ops logger behavior not controlled by this PR. Optional: silence the content-ops logger for this test.

Security Review

Test-only guard: reads local files, runs an in-memory FS transform. No authn/authz, secrets, PII, or network. Verdict: green — no findings.

Testing Review

Adequate and well-structured. Consolidated it.each (per the cross-cutting-invariant convention), independent tests, meaningful assertions, drift-injection proving fail→pass reconciliation. Coverage of the four named drift classes confirmed. See MIN-2 for a message-assertion tightening.

Risk Assessment

Classification carried from refinement — risk:yellow · cost:green — confirmed, not raised: test-only, isolated, no production behavior change; security green; no new dependency (reuses adopted @pair/content-ops within the same bounded context).

Adoption Compliance

• Reuses already-exported @pair/content-ops transforms — no new dependency, no ADR needed (additive test tooling).
• Helper lives in a tested production module (skill-md-mirror.ts) per the "gate & tooling code in tested modules" ADL; both the on-disk guard and the drift-injection tests drive the same code path. ✅
• Follows the established skills-guide-mirror.test.ts mirror-equality pattern (incl. the __dirname REPO_ROOT convention). ✅

Tech Debt

This PR reduces debt (automates the manual #350 check). No new debt introduced. One adjacent coverage gap tracked as #379 (auxiliary skill files).

Definition of Done

All DoD items met: ACs implemented & verified; consolidated data-driven it.each; real-pipeline reuse; suite green + drift-injection demonstrated; lint/type-check green; scope confined to packages/knowledge-hub.

---

Decision: APPROVED WITH COMMENTS. No critical or major issues; all AC met; all gates pass. The minor items (MIN-1/MIN-2/N-1) are optional polish; MIN-3 is deferred to #379. Safe to merge at the human gate.

[rucka]

In reply to the code review.

Review <-> fix loop status — escalating to human review

Summary of the review/fix loop so far (full detail in .pair/working/reviews/352.md):

Round 1 (commit ca01f21) — 2 Minor findings, both resolved:

• Guard didn't exercise the reRootTarget / sourceContentRoot re-rooting branch (shallow virtual dataset). Fixed by seeding the virtual dataset at the same deep path (packages/knowledge-hub/dataset/.skills -> .claude/skills) the real pair update uses, so the guard now exercises the re-rooting logic. Verified via test output showing the link-rewriter actually firing.
• Drift-injection tests only asserted .toThrow(/drifted/), under-verifying AC2 (name the offending skill, show expected vs actual). Fixed by strengthening assertions to check the skill name and the expected/actual dumps are both present in the thrown message.

Round 2 (commit cac7a54) — 1 Minor finding, resolved:

• Production file not prettier-formatted (redundant parens at skill-md-mirror.ts:89). Fixed; prettier:check now clean for this file.

Round 3 (re-review, no new commit) — re-dispatched the same prettier finding; verified already resolved by cac7a54 (pushed to origin/chore/US-352-skill-md-mirror-equality-guard). No further action needed.

Still-open actionable findings (not yet fixed):

1. [Minor] packages/knowledge-hub/src/tools/skill-md-mirror.ts:32,46-50 — SKILL_COPY_OPTS (flatten:true, prefix:'pair') and the VIRTUAL_* source/target paths hardcode values that duplicate the skills registry block in apps/pair-cli/config.json. If that config's flatten/prefix/source/target ever changes, pair update would regenerate the mirror with new values while the guard's stale constants would misattribute the resulting failure to "run pair update" rather than to the stale guard constant itself. Low probability/impact (the on-disk byte comparison eventually surfaces a mismatch), documented in the code comment. Recommendation: derive flatten/prefix/source from apps/pair-cli/config.json read-only, or add a test pinning SKILL_COPY_OPTS to the config.json registry values so a config change fails loudly with correct attribution.

2. [Minor] packages/knowledge-hub/src/tools/skill-md-mirror.test.ts:78-89 — the test "directional guard ignores root-only skills with no dataset source" asserts environment facts (agent-browser exists under root .claude/skills and is absent from dataset-derived installed dirs) rather than exercising the guard's iteration set directly. Reasonable proxy today, but brittle — breaks if agent-browser is renamed/removed even though the directional logic is unchanged. Recommendation: assert the property structurally (dataset-derived installed dirs is a strict subset of root skill dirs) instead of pinning the literal example.

3. [Questions] packages/knowledge-hub/src/tools/skill-md-mirror.ts:60-103 — the story's Technical Analysis said the guard would reuse collectSkillDirs/buildDatasetSkillNameMap from skills-guide-mirror.ts; the PR instead adds its own dataset-dir discovery (readSkillsDatasetFromDisk + datasetSkillDirs) and installedSkillDir (direct transformPath). Defensible (needs the full content tree in memory anyway; transformPath is more direct), dup:check passes, not flagged as duplication — but is conceptual duplication of skill-dir discovery across two sibling tools. Question for the human: is the deviation from the stated reuse intentional? If so, a one-line module-comment note would prevent a future reader assuming an oversight; no code change required if intentional.

None of these three block correctness today; they're maintainability/consistency concerns and one open design question. Escalating per the review<->fix loop's non-convergence threshold rather than auto-resolving further — requesting human triage on whether findings 1-2 warrant another fix round and on the intent behind finding 3.

[rucka]

Code Review — PR #377

Review Information

PR Number: #377
Author: (branch chore/US-352-skill-md-mirror-equality-guard)
Reviewer: Independent reviewer (Claude Code, /pair-process-review)
Review Date: 2026-07-24
Story/Epic: #352 — tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs
Review Type: Refactor / tech-debt (test-only guard)

Review Summary

Overall Assessment

• Approved with Comments — Minor / by-design notes only; can merge.

Key Changes Summary

Adds a data-driven it.each guard (skill-md-mirror.test.ts) + co-located production helper (skill-md-mirror.ts) that asserts every root .claude/skills/<prefixed>/SKILL.md is byte-for-byte the real pair update copy-pipeline transform of its dataset source. Reuses the actual copyDirectoryWithTransforms from @pair/content-ops over an in-memory FS — no parallel transform logic. Two files, both under packages/knowledge-hub/src/tools/.

Business Value Validation

Closes the structural gap that PR #350 had to catch by hand: per-skill SKILL.md drift between the canonical dataset and the installed root mirror is now CI-enforced. Confirmed delivered.

Independent Verification (executed in a detached worktree at the PR head)

Gate	Result
vitest run skill-md-mirror.test.ts	✅ 47/47 pass
Full @pair/knowledge-hub suite	✅ 415/415 pass (47 new), 19 files
Lint (@pair/knowledge-hub)	✅ clean
Type-check (tsc -p tsconfig.json)	✅ clean
Scope containment	✅ exactly 2 files, both packages/knowledge-hub/src/tools/
Dataset skill count	39 dataset skills; root has 40 (agent-browser root-only)

Acceptance Criteria — verified against the diff + code (not just claims)

• AC1 (every dataset skill byte-for-byte in sync) — ✅ main it.each guard passes on real on-disk mirror (39 skills).
• AC2 (frontmatter name: drift fails, naming skill + expected-vs-actual) — ✅ drift-injection asserts the thrown message carries the skill name, --- expected, --- actual, and both content variants.
• AC3 (link-depth ../../→../../../ and /command drift fail) — ✅ both covered; a "fixture is meaningful" test proves the real transform actually performs all three rewrites, so the drift tests are not no-ops. Confirmed the real next skill exercises the depth-bump on actual data.
• AC4 (missing root mirror fails loudly) — ✅ throws missing … pair update.
• AC5 (new skill covered automatically, data-driven, no hardcoded count) — ✅ datasetSkillDirs() derives cases from the dataset tree at collection time.
• Business rules — ✅ directional (dataset→root, agent-browser excluded, explicitly tested); ✅ faithful transform (drives real copyDirectoryWithTransforms — verified against production caller apps/pair-cli/src/registry/operations.ts and config.json's skills registry); ✅ consolidated one file; ✅ gate/tooling helper in a tested production module; ✅ R7.2 additive (green on current content).

Security Review

Test-only guard — no authn/authz/secrets/PII/network. Verdict: green, 0 findings (0 introduced).

Detailed Review Comments

Positive Feedback

• Genuinely drives the real production pipeline — a bug in copyDirectoryWithTransforms fails the guard rather than being masked by a re-implementation.
• The VIRTUAL_* deep-path setup deliberately reproduces the real sourceContentRoot/reRootTarget re-rooting branch — faithfulness beyond the minimum.
• Drift-injection proves each drift class fails and that the synthetic fixture is meaningful (asserts the transform performs all three content rewrites).
• AC2 message assertions guard the diagnostic ergonomics themselves (skill name + expected/actual dump).

Minor Issues 💡

• skill-md-mirror.ts:63-77 (datasetSkillDirs) / :30-49 (readSkillsDatasetFromDisk) — The story's approach said to reuse collectSkillDirs/buildDatasetSkillNameMap from the sibling skills-guide-mirror.ts; this PR instead adds its own dataset-dir discovery. Not strictly redundant (this path also needs the full content tree to seed the in-memory FS), but the dir-discovery concept is duplicated in the same directory. Consider deriving dirs via the shared helper or add a one-line note on why a separate reader is warranted. Non-blocking.

By-design / non-blocking notes

• skill-md-mirror.ts:34 (SKILL_COPY_OPTS) — hardcodes { flatten: true, prefix: 'pair' } rather than reading apps/pair-cli/config.json. Matches the existing convention in skills-guide-mirror.ts, and a config divergence would fail the guard loudly (fails safe). By convention.
• Coverage scope — the guard asserts only SKILL.md, not the sibling reference files the same pipeline transforms and that can also drift (pair-process-review/degradation-levels.md, merge-and-cascade.md, pair-process-implement/post-review-merge.md, pair-process-bootstrap/assess-orchestration.md). Story tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352 is explicitly scoped to per-skill SKILL.md, so this is by design; a follow-up story could extend the same real-pipeline guard to all skill-dir files. Separate artifact set / separate story if desired.

Questions ❓

• Test output noise — the in-memory run emits ~20 logger.info lines from @pair/content-ops. Acknowledged in the PR description; not this PR's code. Optionally silence the logger in test setup. Out of scope.
• Story tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352 title still says "all 36 skills" (actual = 39). The PR flags this and the guard is data-driven, so the count is moot in code — the title is a PM artifact worth correcting. Non-code.

Risk Assessment

Carried classification risk:yellow · cost:green confirmed (not raised): isolated, test-only, no production behavior change, reuses exported @pair/content-ops transforms. No new technical decision → no ADR/ADL required (agreed).

Definition of Done

All DoD items met and independently verified (AC coverage, consolidated data-driven it.each, real-pipeline reuse, drift demonstrably fails then passes, lint/type-check green, scope confined to packages/knowledge-hub).

---

Decision: Approved with Comments. No critical or major issues; all ACs met; all quality gates independently green. Comments above are minor/by-design and non-blocking. Merge remains the human gate.

[rucka]

Code Review — PR #377

Review Information

PR Number: #377
Author: rucka (Gianluca Carucci)
Reviewer: Independent Reviewer (Claude Code, /pair-process-review)
Review Date: 2026-07-24
Story/Epic: #352 (tech-debt — extend mirror-equality guard to per-skill SKILL.md)
Review Type: Chore / tech-debt (test tooling)

Review Summary

Overall Assessment

• Approved with Comments — all acceptance criteria met and independently verified; only Minor/nit findings, none blocking.

Key Changes Summary

Adds a data-driven it.each guard (skill-md-mirror.test.ts) + co-located production helper (skill-md-mirror.ts) asserting every root .claude/skills/<prefixed>/SKILL.md is byte-for-byte the real pair update copy-pipeline transform of its dataset source. The guard drives the production copyDirectoryWithTransforms over an in-memory clone of the dataset — no parallel transform re-implementation. Two files added, +333/-0, confined to packages/knowledge-hub.

Business Value Validation

Closes the structural mirror-integrity gap that PR #350 fixed by hand: per-skill SKILL.md drift between the canonical dataset and the installed root mirror is now CI-enforced, additive, and non-blocking (R7.2).

Verification Performed (independent)

Ran in a detached worktree pinned to the PR head (cac7a54):

• vitest run src/tools/skill-md-mirror.test.ts → 47 passed (39 data-driven it.each + discovery + directional + 6 drift-injection).
• pnpm --filter @pair/knowledge-hub ts:check → clean.
• pnpm lint --filter @pair/knowledge-hub → clean.
• Empirically confirmed 39 dataset skill dirs, all with existing root mirrors; agent-browser is the only root-only skill (directional rule holds).
• Traced the real pipeline (install/handler.ts → resolveEffectiveDatasetRoot → operations.ts → copy-directory-transforms.ts): for the skills registry effectiveDatasetRoot = baseTarget (flatten/prefix), so sourceRelative is the deep packages/knowledge-hub/dataset/.skills and sourceContentRoot is non-trivial — the guard's VIRTUAL_* scheme reproduces this exactly, so the link-rewriter reRootTarget branch genuinely executes (confirmed by Link rewriter: rewrote 1 links in .../pair-demo/SKILL.md in the run log).
• Confirmed both cited ADLs exist and support the design: decision-log/2026-07-18-conformance-test-per-file-not-per-story.md (cross-cutting-invariant single-file exception) and decision-log/2026-07-13-gate-tooling-code-in-tested-modules.md (gate logic in an importable tested module, no spawnSync).

AC Verification

AC	Verdict	Evidence
AC1 — every dataset skill byte-for-byte when in sync	PASS	it.each over 39 dirs green against on-disk root mirror
AC2 — frontmatter name: drift fails, names skill + shows expected/actual	PASS	drift-injection asserts message carries skill name + --- expected/--- actual + both contents
AC3 — link-depth / /command drift fails	PASS	two drift-injection cases; fixture-meaningful test proves both rewrites fire
AC4 — missing root mirror fails loudly	PASS	throws missing … pair update; real guard also throws on undefined
AC5 — new dataset skill covered with no test edit	PASS	datasetSkillDirs() derived from disk at collection time; discovery test green
Business rule — directional (agent-browser not drift)	PASS	verified empirically + dedicated test
Business rule — faithful transform, no re-implementation	PASS	drives real copyDirectoryWithTransforms + options equivalent to buildCopyOptions

Code Review Checklist

• Functionality: all AC met (verified by running, not just claimed).
• Code quality: clear, well-documented module; rationale comments explain the VIRTUAL_* faithfulness choice and helper placement.
• Technical standards / architecture: compliant with both cited ADLs; placement in src/tools/ matches the sibling skills-guide-mirror guard.
• Dependencies: none added — reuses already-exported @pair/content-ops functions.

Security Review

Test-only guard. No authn/authz, secrets, PII, or I/O beyond reading the repo's own dataset/skill trees. No security-relevant surface. Classification risk:yellow · cost:green (carried from refinement) is appropriate; nothing in the diff raises it.

Testing Review

Excellent. Real-pipeline reuse (a production bug fails the guard rather than being masked), self-guarding "fixture is meaningful" test, and drift-injection covering all four drift classes through the same assertRootSkillMdMatches code path the real guard uses. Test independence and naming are good.

Detailed Review Comments

Positive Feedback

• Faithful real-pipeline reuse via in-memory FS — no parallel transform logic; the VIRTUAL_* path scheme deliberately reproduces the real deep-source / repo-root-datasetRoot resolution so the reRootTarget/sourceContentRoot re-rooting branch is actually exercised.
• Data-driven case list keyed off disk — genuinely no hardcoded count (AC5), robust to the stale "36→39" title drift.
• Self-guarding drift fixture: the "performs all three content rewrites" test means a future regression that stopped triggering a rewrite fails loudly instead of silently weakening the guard.
• Assertion helper in a tested production module, exercised by both the on-disk guard and drift-injection — clean ADL-compliant design.

Minor Issues

• skill-md-mirror.ts / .test.ts (whole guard) — Coverage is scoped to SKILL.md only. Other artifacts under a skill dir that flow through the same pair update transform (sub-docs such as merge-and-cascade.md, degradation-levels.md, post-review-merge.md, and any references/*) are copied/rewritten but not asserted, so a hand-edit to a root sub-doc would drift undetected. This matches tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352's explicit scope (per-skill SKILL.md) and is by-design for this PR; extending mirror-equality to all skill artifacts is a broader, distinct guard that warrants its own follow-up story. Recommendation: open a follow-up tech-debt story to extend the invariant to non-SKILL.md skill artifacts.

• skill-md-mirror.test.ts:66-68 — the real it.each guard wraps assertRootSkillMdMatches in expect(...).not.toThrow(), so a genuine on-disk drift surfaces the thrown message (skill name + regenerate hint + full expected/actual dump) under vitest's "expected not to throw" wrapper. For a large SKILL.md the message embeds both entire files, which can be very noisy in CI. AC2 only requires expected-vs-actual to be shown, so this is acceptable; a unified diff would be friendlier. Recommendation: acceptable as-is; consider a diff-style dump if failure output proves noisy in practice.

Questions / Nits

• skill-md-mirror.ts:60-93 — the story's technical approach named collectSkillDirs / buildDatasetSkillNameMap (from skills-guide-mirror.ts) for case-list derivation; the PR instead adds readSkillsDatasetFromDisk + datasetSkillDirs. This is arguably a better abstraction (enumerates only SKILL.md-bearing dirs vs any-file dirs, off an already-read in-memory tree) and is not a defect, but it leaves two conceptually parallel skill-dir enumerators across the two sibling files. Recommendation (optional): a one-line comment noting why datasetSkillDirs is preferred over reusing collectSkillDirs would aid future readers.

Risk Assessment

Risk	Impact	Probability	Mitigation
Guard diverges from real pipeline	Med	Low	Drives real copyDirectoryWithTransforms; options equivalent to buildCopyOptions; verified by running
Non-SKILL.md skill-file drift undetected	Low	Low	Out of #352 scope; recommend follow-up story

Tech Debt

This PR is itself a deliberate tech-debt closure (R7.2). One residual item surfaced (non-SKILL.md skill-artifact mirror coverage) — surfaced, not blocking; recommend deliberate promotion to a follow-up story.

Adoption Compliance

Level assessed against repo ADLs: compliant. No new technical decision → no ADR required (confirmed against the two cited decision-log entries). Scope confined to packages/knowledge-hub per DoD (verified: only the 2 added files).

Decision

Approved with Comments — all five ACs met and independently verified by executing the suite; lint + type-check clean; ADL-compliant; no critical/major issues. Findings are Minor/nit and non-blocking. Merge is the human gate.

[rucka]

In reply to the code review.

Remediation — final summary (review clean, converged)

Full map of every finding raised across the review<->fix loop on this PR, and how each was resolved.

Round 1 (commit ca01f21)

• [Minor] Guard didn't exercise the reRootTarget/sourceContentRoot re-rooting branch (skill-md-mirror.ts:97-121) → Re-seeded the virtual dataset at the same deep path the real pair update uses (VIRTUAL_SOURCE_REL = 'packages/knowledge-hub/dataset/.skills', VIRTUAL_TARGET_REL = '.claude/skills'), so sourceContentRoot is non-trivial and reRootTarget actually executes under the guard. Verified via test output (Link rewriter: rewrote 1 links...). — packages/knowledge-hub/src/tools/skill-md-mirror.ts
• [Minor] Drift-injection tests only asserted .toThrow(/drifted/) — under-verified AC2 (skill-md-mirror.test.ts:117-131) → Added captureThrownMessage helper; strengthened the frontmatter-drift case to assert the message names the offending skill (/pair-demo/) and carries labelled --- expected / --- actual dumps with both the expected and drifted content. — packages/knowledge-hub/src/tools/skill-md-mirror.test.ts

Round 2 (commit cac7a54)

• [Minor] Production file not prettier-clean (redundant parens, line 89) → Removed the redundant parens around the slice arg (semantically identical; member access binds tighter than unary minus). — packages/knowledge-hub/src/tools/skill-md-mirror.ts

Round 3 (re-review, no new commit) — re-verified the round-2 prettier fix was already committed/pushed; no further change needed.

Round 4 (commits 1bea4ca, a408942)

• [Minor] Guard scope — only asserts each skill's SKILL.md, not other per-skill artifacts (sub-docs/references) → Confirmed by-design for tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352's scope; opened follow-up tech-debt story tech-debt: extend mirror-equality guard to all root skill artifacts (sub-docs + references) #384 to extend the guard to all root skill artifacts, and added a SCOPE (by design, per #352) docstring note pointing to it. — packages/knowledge-hub/src/tools/skill-md-mirror.ts
• [Minor] .not.toThrow() drift dump embedded both full files — noisy CI output → Replaced the full two-file dump with a compact LCS-based line-level diff (diffSkillMd + lineEditScript, -=expected/+=actual, collapsed unchanged runs with 2-line context); added a dedicated diffSkillMd unit-test block. — packages/knowledge-hub/src/tools/skill-md-mirror.ts, packages/knowledge-hub/src/tools/skill-md-mirror.test.ts
• [Questions] Two parallel skill-dir enumerators (datasetSkillDirs vs collectSkillDirs) without explanation → Added a doc comment on datasetSkillDirs explaining the divergence: it enumerates only SKILL.md-bearing dirs off the already-read in-memory tree (no second disk walk) and avoids mistaking a references/-only subdir for a skill's own directory. — packages/knowledge-hub/src/tools/skill-md-mirror.ts
• Housekeeping (commit a408942): prettier-wrapped the lcs[i]![j] = … line (exceeded printWidth), surfaced by the pre-push formatter.

Final independent re-review — zero actionable findings. Two items surfaced were reviewed and accepted as non-actionable:

• [Minor] SKILL_COPY_OPTS hardcodes { flatten: true, prefix: 'pair' } instead of reading it from apps/pair-cli/config.json's skills registry (skill-md-mirror.ts:38) — Accepted as-is: matches the existing convention in the sibling skills-guide-mirror.ts (same hardcoded options), and a future config divergence self-surfaces as a loud AC4 "missing mirror" failure rather than silently passing.
• [Minor] Package-level prettier:check fails on src/conformance/refine-story-draft-to-ready.test.ts — Not actionable in this PR: that file is byte-identical to origin/main, untouched by this diff, and pre-exists on main; fixing it here would be scope creep. This PR's own two files (skill-md-mirror.ts + its test) are prettier-clean, lint-clean, and type-check clean.

Quality gates: PASS — pnpm --filter @pair/knowledge-hub test (420 pass, 52 in the guard file) + check:links, pnpm --filter @pair/knowledge-hub lint, tsc --noEmit (knowledge-hub), pnpm --filter @pair/pair-cli test (846 pass).

Verdict: review clean. All actionable findings across all rounds resolved with commit refs above; the two remaining items are accepted non-actionable dispositions, not open work. Story #352 converged. Not merged — human gate.