fix: make pair update self-consistent for the dogfooded root install

[rucka]

Summary

Makes pair update produce a root .pair/ install that equals the real content-ops transform of the dataset, is link-correct, and idempotent — so the dogfooded tree is regenerated by the tool, and the conformance tests validate the dataset (source) through that same transform.

Why

Running pair update on this repo previously broke 3 conformance tests, would have produced broken links, and deleted the third-party agent-browser skill. Causes: the copy pipeline rewrote skill /commands in .pair/knowledge/ but not markdown link PATHS; two skills-guide.md spans were hand-curated; and the .skills registry used mirror, which reaps any non-source entry from the shared .claude/skills/ dir.

What changed

• content-ops — new rewriteSkillLinkPaths + buildSkillLinkPathMap convert .skills/<cat>/<name>/SKILL.md links to .claude/skills/pair-<...> in copied non-.skills registries; threaded through the copy pipeline + CLI.
• config — the skills registry behavior is now overwrite (was mirror). Stale cleanup runs only under mirror, so overwrite updates/adds pair skills but never deletes — third-party / user-installed skills in the shared .claude/skills/ dir (e.g. agent-browser) survive. Trade-off: a renamed/removed pair skill leaves its old dir, cleaned up manually (rare, human-gated).
• dataset skills-guide.md — the 2 hand-curated spans authored so the mechanical transform reproduces the correct installed form (no manual test overrides).
• conformance — secret-scanning + skills-guide-mirror assert root == realTransform(dataset) via the content-ops functions (single source); skills-guide-mirror delegates map construction to the production builder.
• check:links — now also gates the generated root .pair/knowledge/ (read-only).
• regenerated root install from pair update (reconciles pre-existing staleness).

Verification (all green, local)

content-ops 633 · pair-cli 836 · knowledge-hub 206 + check:links on dataset AND root · skills:conformance 36 · docs:staleness · lint 6/6. Idempotent; a foreign skill (agent-browser + a synthetic one) survives pair update — empirically verified.

Reviewer notes

• Blast radius: @pair/content-ops link-path conversion (consumer install pipeline). The agent-browser preservation is config-only (no ownership code).
• CI does not gate prettier/markdownlint; CLAUDE.md is committed as pair update emits it (idempotency).

See the PR comments for the review-round history (round 2 approved; then simplified to config-only per maintainer direction).

🤖 Generated with Claude Code

[rucka]

Code Review — PR #346: fix: make pair update self-consistent for the dogfooded root install

Reviewer: Independent review (blind, adversarial) · Date: 2026-07-18 · Review Type: Bug fix / refactor
Method: Detached worktree at PR head 6962a9b. Review from diff + code only. All package suites run locally in the worktree.

Overall Assessment

• Approved with Comments — no critical/major issues; a few minor / by-design notes below.

The change is well-scoped, well-tested, and the headline claims verify independently. The link-path conversion is correctly keyed to real relative links and cannot corrupt prose. Tests genuinely compare committed root == realTransform(dataset) (not a tautology). The check:links root gate is read-only and fails closed. Consumer blast radius is bounded and consistent with pre-existing behavior.

Independently verified

• @pair/content-ops 632 tests pass; @pair/pair-cli 836 pass; @pair/knowledge-hub 206 pass.
• check:links passes on both the dataset AND the generated root .pair/knowledge/ (read-only over root — checkOnly: true; reports but never writes; exits non-zero only on LINK TARGET NOT FOUND/BAD LINK FORMAT).
• Root skills-guide.md: composition diagram carries prefixed names; the two ../../.claude/skills/pair-*/SKILL.md links resolve to real .claude/skills/pair-capability-map-* directories.
• skills-guide-mirror.test.ts whole-file test asserts transform(dataset) === committed root — catches drift in either direction. secret-scanning.test.ts root-parity asserts rootContent === realTransform(datasetContent). Neither is a self-referential tautology.
• Threading of skillLinkPathMap is consistent across copyPathOps → copy-file → CLI install/update → reconcileSkillNameRegistry → applySkillRefsToNonSkillRegistries; reconcile signature updated in both handlers.

Positive feedback

• rewriteSkillLinkPaths substring key ../.skills/<cat>/<name>/SKILL.md (single leading ../, matched as a substring of any deeper ../../…) is a clean, depth-independent design; idempotency holds because the installed .claude/skills/ form contains no source ../.skills/ substring. Tests cover depth preservation, prose bare-path, repo-relative prose, and idempotency.
• Fence-aware rewriteSkillReferences with the CommonMark info-string guard is careful; the fenced composition diagram being authored with prefixed names (transform is identity inside fences) is the right way to make the dataset transform-correct.
• The manifest-based transition map keeps /command rewriting idempotent across prefix changes — orthogonal to the new link-path work but correctly left intact.

Minor Issues / Questions

1. .pair/knowledge/guidelines/quality-assurance/manual-testing.md:108 + .claude/skills/pair-capability-execute-manual-tests/SKILL.md + dataset copy + qa/release-validation/README.md — after this PR deletes .claude/skills/agent-browser/ (11 files), KB docs still name agent-browser as the Preferred manual-testing tool. Not broken links (inline code, not ](…)), so check:links stays green. Reads as documentation pointing at a now-absent local skill. nonActionable / by-design: agent-browser is framed as an external installable Agent Skill ("works across any agent that supports Agent Skills"), the deletion is required to keep the mirror-registry root idempotent, and the PR reviewer-note flags it explicitly with a remedy (add to dataset or a reserved path in a separate change).

2. skill-reference-rewriter.ts:234-245 (buildSkillLinkPathMap) — map keyed strictly on …/SKILL.md. A KB cross-reference to a non-SKILL.md file inside a skill dir (e.g. ../../.skills/capability/foo/references/bar.md) would not be converted and would break post-install. No such links exist today. Recommendation: note the SKILL.md-only scope in the docstring's contract (it documents the ../ rationale but not the SKILL.md-only limitation), or broaden the key if reference-file cross-links are ever authored.

3. skills-guide-mirror.ts:85-93 — the test-side buildSkillLinkPathMap re-implements the map builder locally (hardcoded ../../ prefix) instead of importing the production buildSkillLinkPathMap from @pair/content-ops. It uses the real rewriteSkillLinkPaths, so the equality check is sound, but a bug in production's map construction wouldn't be caught by the mirror tests (only the rewrite function is shared). Recommendation: build the test map from the production buildSkillLinkPathMap to close the single-source-of-truth gap the module docstring claims.

4. check-broken-links.ts (root gate) — the root run reports Normalized relative links: 4 (read-only, benign, non-failing). Worth confirming these 4 are just check:links-normalizer opinions the pair update pipeline deliberately doesn't apply (so not a latent idempotency wrinkle), rather than links the generated root should already carry in normalized form.

5. Root content-parity coverage — only skills-guide.md (whole-file) + 4 named files have root == transform(dataset) equality tests. Content drift in other generated root KB files is caught only as broken links, not as content drift. nonActionable / by-design: the PR intentionally scoped this narrowly (per [#227] feat: assess-security capability + deterministic secret-scanning layer #341 round-3 review) rather than adding a whole-tree parity gate.

6. Dataset skills-guide.md "Unprefixed dataset command names (map-subdomains, map-contexts)" — dropping the leading / reads slightly less like a command token. nonActionable / by-design: required so rewriteSkillReferences (which only matches /name) is identity on these self-referential mentions, keeping root == transform(dataset).

Risk Assessment

• Consumer blast radius (content-ops install pipeline): Low. The new rewrite runs over non-.skills registries' .md files, same set already subject to /command rewriting; the link-path key cannot match prose describing dataset paths (no ../ prefix) — covered by tests.
• Idempotency: Verified via conformance tests (root == transform(dataset)) + all-green suites; residual note (4) is informational only.

Verdict: Approved with Comments

[rucka]

Remediation — review round 1 resolved

All review findings addressed (+ a correction on agent-browser). Every gate re-run locally and green; pair update idempotent.

agent-browser is now PRESERVED (correction of finding #1 / earlier PR note)

Root-cause fix instead of accepting the deletion: .claude/skills/ (and the symlinked tool dirs) is a shared target — pair's own skills are all pair-*, third-party/user skills are not.

• content-ops mirror cleanup is now scoped to the registry's own prefix: it only reaps ${prefix}-* stale entries, never foreign ones.
• CLI reconcileSkillNameRegistry adds a manifest-aware pass that removes only dirs pair owned last run and no longer installs (renamed/prefix-changed via the transition map, or removed via orphaned names) — so cross-prefix cleanup (AC4 / Skill install: flatten + prefix + cross-reference rewriting from nested dataset #238) still works, with ownership coming from the manifest, never a heuristic.
• Empirically proven: a fake acme-thirdparty-tool survives pair update while a stale pair-* dir is still removed. Regression tests added (content-ops: "preserves a foreign non-prefixed entry"; the prefix-change test documents the conservative content-ops behavior + the manifest-aware CLI removal).
• The docs referencing agent-browser stay valid — no doc changes needed.

Findings

• Collaborative Knowledge Base #2 (SKILL.md-only contract) — documented on buildSkillLinkPathMap.
• Adoption Management & Guideline Linking #3 (test re-implemented the map builder) — skills-guide-mirror.ts now delegates to the production buildSkillLinkPathMap (single source; keys now ../.skills/…, unit test updated).
• Workflow Customization #4 (Normalized relative links: N in checkOnly) — generateFinalReport skips the patch/normalize lines in read-only mode.
• Advanced Features Enablement #5 / #6 — confirmed nonActionable / by-design.

Gates (re-run)

content-ops 633 · pair-cli 836 (incl. #238) · knowledge-hub 206 + check:links valid on dataset and root · skills:conformance 36 · docs:staleness · lint 6/6 · pair update idempotent (2nd run = 0 diff) · agent-browser present after update.

[rucka]

Code Review (round 2) — PR #346: fix: make pair update self-consistent for the dogfooded root install

Reviewer: Independent review (blind, adversarial) · Date: 2026-07-19 · Review Type: Bug fix / refactor (re-review)
Method: Detached worktree pinned to PR head a39cb20. Review from diff + code + PR description + round-1 review comment + remediation comment only (no .pair/working/). All three package suites run locally in the worktree; check:links run over dataset AND generated root.

Overall Assessment

• Approved with Comments — every round-1 finding is genuinely resolved, the agent-browser deletion is reversed by a correct two-layer root-cause fix, and all gates pass. Remaining notes are Minor / by-design.

Round-1 findings — verification

• Setup & Project Management Integration #1 (agent-browser deleted) — RESOLVED via root-cause fix, not by accepting the loss. agent-browser (11 files) is restored and now provably survives pair update. Two-layer design:
  • content-ops cleanupStaleTransformedEntries (copy-directory-transforms.ts:307) skips any top-level entry not matching the registry's own ${prefix}- when a prefix is set — foreign/user skills in the shared .claude/skills/ are preserved; only pair-* stale dirs are reaped.
  • CLI removeStaleOwnedSkillDirs (skill-refs.ts:105) removes only dirs derived from the manifest (transition-map keys = old installed names on a prefix change, plus orphaned names for removed skills). Foreign skills are never in the manifest, so are never candidates — safe by construction.
• Collaborative Knowledge Base #2 (SKILL.md-only contract) — RESOLVED. buildSkillLinkPathMap docstring (skill-reference-rewriter.ts:233-236) now documents that only a skill's SKILL.md entrypoint link is converted; deep links to references/*.md are not (none exist today).
• Adoption Management & Guideline Linking #3 (test re-implemented the map builder) — RESOLVED, non-superficially. skills-guide-mirror.ts:26,86 now delegates to the production buildSkillLinkPathMap (prodBuildSkillLinkPathMap). The equality test still reads the committed root file from disk (skills-guide-mirror.test.ts:85,131) and compares it to transform(dataset) — not a tautology; a bug in production map construction now fails the conformance test.
• Workflow Customization #4 (misleading Normalized: N in checkOnly) — RESOLVED. generateFinalReport (validatePathOps.ts:130) returns early in checkOnly mode, suppressing the patch/normalize count lines. Read-only guarantee intact: runAllChecks fails only on allErrors.length > 0 (LINK TARGET NOT FOUND / BAD LINK FORMAT), never on normalized counts; the root run passes checkOnly: true (check-broken-links.ts:94) and mutated no tracked files in my run.
• Advanced Features Enablement #5 / #6 — confirmed nonActionable / by-design (unchanged).

Focus-area verification (round 2)

• Cleanup correctness across scenarios (all locked in by tests):
  • same-prefix removed skill IS reaped — content-ops test "removes a stale flattened directory when its source skill is gone" + handler test 1127 (end-to-end).
  • prefix-change old dir: content-ops deliberately PRESERVES it (conservative, ownership-unaware — test at copy-directory-transforms.test.ts:412), CLI manifest pass removes it (handler test "AC4 edge case: prefix change removes the old flattened dir…" at handler.test.ts:1052, end-to-end incl. rewriting an add-behavior doc via the recorded map).
  • foreign skill preserved — content-ops test "preserves a foreign non-prefixed entry (third-party skill)" at :390.
• Symlinked tool dirs: cleanup + CLI removal operate only on non-symlink targets (getNonSymlinkTargets); symlinked tool dirs resolve to canonical .claude/skills/, so they inherit its preserved/reaped state. No path-resolution gap.
• Regression from scoping: only the skills registry uses a prefix (config/adr-005/website docs); the non-prefix mirror path (handleMirrorCleanup and the !transformOpts.prefix fallback) is unchanged. Nothing else relied on reap-everything for a prefixed target.
• Manifest handling: .pair/.skill-name-map.json is a local runtime marker, correctly gitignored (not committed); errors-root.txt gitignored too.

Gates (re-run locally in worktree)

• @pair/content-ops 633 pass · @pair/pair-cli 836 pass · @pair/knowledge-hub 206 pass.
• check:links valid on dataset AND generated root .pair/knowledge/ (read-only; suppresses count lines; no tracked-file mutation observed post-run).

Minor Issues / Notes

1. apps/pair-cli/src/commands/update/handler.test.ts (coverage) — No end-to-end (handler-level) test asserts a foreign (non-pair-*) skill dir SURVIVES a full pair update. Foreign preservation is tested at the content-ops unit layer (where the original bug lived) and the CLI pass is structurally safe, but the headline fix ("agent-browser survives") is only "empirically proven" in the PR body, not locked in end-to-end — ideally combined with a prefix change so both cleanup layers run in one test. Not blocking — the specific regression is guarded at its origin layer. Recommendation: add such a handler test.

2. copy-directory-transforms.ts:307 (by-design edge) — A foreign skill whose directory name coincidentally starts with pair- WOULD be reaped by content-ops mirror cleanup. nonActionable / by-design: the documented contract (apps/website/content/docs/integrations/index.mdx:30, adr-005) is that the pair prefix namespaces all pair skills and third-party skills use a different/no prefix; a foreign pair-* dir is namespace squatting and indistinguishable from a pair skill by design. Reported for transparency.

Verdict: Approved with Comments

[rucka]

Simplified: agent-browser preserved via config, not code

Per maintainer direction, the two-layer ownership code from round 1 (prefix-scoped mirror cleanup + manifest-aware CLI deletion) is reverted. The shared-skills-dir preservation is now handled purely by configuration:

• apps/pair-cli/config.json — the skills registry behavior is now overwrite (was mirror). Stale cleanup runs only under mirror, so overwrite updates/adds pair skills and never deletes — third-party / user-installed skills in the shared .claude/skills/ dir (e.g. agent-browser) survive pair update.
• Trade-off (accepted): a renamed/removed pair skill leaves its old dir behind — cleaned up manually. Rare and human-gated (skill renames go through a PR anyway).
• No production code for ownership; content-ops mirror cleanup and the CLI reconcile pass are back to their original behavior.

Tests: content-ops now has "preserves a foreign (non-source) entry under overwrite behavior"; the prefix-change mirror test is restored; config loader asserts behavior: overwrite. Empirically re-verified: a fake third-party skill AND agent-browser both survive pair update; regenerated root is unchanged and idempotent.

Gates (re-run): content-ops 633 · pair-cli 836 · knowledge-hub 206 + check:links (dataset+root) · skills:conformance 36 · docs:staleness · lint 6/6.

The self-consistency core (link-path conversion, root==realTransform(dataset), check:links over the generated root, findings #2/#3/#4) is unchanged from the round-2 approval.