[US-384] chore: mirror-equality guard widened to every dataset skill artifact - #406
Merged
Merged
Conversation
…artifact 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
5 tasks
Collaborator
Author
Verdict
PR: [#406] · Author: rucka · Reviewer: independent reviewer agent · Date: 2026-07-31 · Story: [US-384] · Type: chore (test tooling / refactor) Classification matrix — per dimension
Tier = max(assessed) = yellow — confirms the refinement-time tier (raise-only floor, D17; no change ⇒ no drift note). Cost = highest detected signal = green. AssessmentsSecurity — Input validationVerdict: green — no external input: the only "inputs" are repo-owned dataset paths/contents read via Details
Security — Output handlingVerdict: green — output is test-assertion text (error messages + a line diff); no rendering/serialization surface. Security — AuthenticationVerdict: green — no authentication surface touched. Security — AuthorizationVerdict: green — no access-control surface touched. Security — Introduced vulnerabilitiesVerdict: green — 0 introduced, 0 pre-existing. DetailsNo new dependency, no secret material, no privileged operation. The module runs an in-memory clone of the copy pipeline ( CostVerdict: Details
Architecture (Coupling)Verdict: green — no new integration; the intentional coupling to Details
DetailsFindings by severityCritical (must fix before merge)
Major (should fix before merge)
Minor (consider)
Questions
Positive feedback
Functionality & requirements (AC coverage)
Testing & quality gatesRun independently in a detached worktree pinned to the PR head (
Adoption compliance
Tech debt
Documentation
Performance & deployment
|
…-ops walk, #407 pointers - 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.
Formatting-only follow-up to 406f8a6 (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.
…ntradictory comments - 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 <noreply@anthropic.com>
rucka
added
the
tech-debt
Collaborator
Author
Remediation report — PR #406 · story #384Closes the loop on the first review above. 12 findings raised (1 Major · 9 Minor · 2 Questions) → 0 open actionable findings. Two fix rounds landed in Head at verification: Each item below was re-verified against the head, not against the fix commit messages. Major
Minor — all 9 resolved
Questions — both answered in the module
Beyond the findingsRound 2 added a hardening the review did not ask for: Ancora aperto, dichiarato
Verdetto invariato: TECH-DEBT — nothing blocking; the one Major is accepted with its disposition and tracked as its own story. |
rucka
added a commit
that referenced
this pull request
Jul 31, 2026
…transform `main` went red the moment #387 and #406 were both on it, and neither PR was wrong. #387 (brainstorm's new `resume.md` / `parametrization.md` siblings) was green before #406 widened the mirror-equality guard beyond `SKILL.md`; #406 was green before those siblings existed. Merging them in EITHER order produces this failure, so no single PR's CI could have caught it. The cause is mine: I hand-ported those siblings to the root mirror in #387 instead of regenerating them through the real transform. - `parametrization.md` — a missing blank line before `## $domain-placed`. - `resume.md` — SUBSTANTIVE, not cosmetic. The hand-port applied the skill-reference rewrite to a FILE NAME, shipping the handoff key as `.pair/working/pair-process-brainstorm-<root-id | theme-slug>.md` where the dataset says `.pair/working/brainstorm-<root-id | theme-slug>.md`. The real transform does not rewrite it — it is a working-file name, not a skill reference — so the shipped mirror documented a different handoff key than its authoring source. A resume reading one and a write using the other never meet. - `SKILL.md` + the dataset twin — emphasis style. Surfaced only while fixing the above, and its cause is the defect #394 fixes: the gate's `mdlint:fix` runs in WRITE mode, so my own gate run rewrote the DATASET's emphasis markers, leaving the mirror behind. The manual gate passed because it was reformatting as it ran; the pre-push hook then compared the rewritten dataset against the untouched mirror. Kept the gate's formatting (it is this repo's authority) and realigned the mirror. Third time today a hand-ported mirror drifted from the transform (#408 by one `./`, #411 likewise). The lesson is applied on the open branches: regenerate, never hand-port. `pnpm quality-gate` green — knowledge-hub 715 tests, mirror guard 82. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rucka
added a commit
that referenced
this pull request
Jul 31, 2026
…y it can prove Finishes the rebase onto the merged #406. Two fixture properties turned out to need OPPOSITE entry depths, which is why a single fixture could not carry both. - The **link-depth rewrite** is only observable with a ONE-segment entry: `demo/` sits 2 levels below the dataset root while `pair-demo/` sits 3 below the mirror root, so the link up must be recomputed. With a two-segment entry both sides are 3 levels and the rewriter correctly leaves the link untouched — a degenerate case that proves nothing. I moved this fixture to two segments earlier in the rebase and silently destroyed the assertion's purpose; it is back on one segment, with the reason stated in the test. - The **nested `references/`** case needs a TWO-segment entry, because a one-segment entry owning a sub-directory is refused outright by the shallow-entry guard — and an entry deeper than the flatten depth by the guard added in round 3. So it gets its own `catalog/nested` entry. Also inverted the two assertions that pinned the OLD flatten mapping. They said so by name — "as the pipeline does today (#407)" — and were written knowing this PR would invert them: the nested sub-dir is now expected INSIDE its skill, with the sibling shape asserted unreachable. The rebase conflict itself was in the same file: #406 added `behavior` to the registry-pin test while this PR added `flattenDepth`. Both kept. `pnpm quality-gate` green — mirror suite 78, content-ops 649. Refs #407
4 tasks
rucka
added a commit
that referenced
this pull request
Jul 31, 2026
… landing Third and fourth instance today of the same class: two PRs each green on their own branch, red once both are on main, because each PR's guard only ever saw its own branch. Neither PR was wrong either time. 1. **#388 + #389.** #389's AC4 audit forbids a skill doc from reading a PR "from the PM tool". #388's `assess-cost` report mode reads the window's merged PRs "from the PM tool / code host" — correctly routed, with a pointer to the resolution convention, so it is a FALSE POSITIVE of the guard: the pattern tempers itself against `code host` only in the 60 characters BEFORE "the PM tool", and here it comes after. Reordered to "from the code host / PM tool", which is also the more accurate reading — a PR is a code-host object, so naming that side first is right independently of the guard. Recorded for whoever tunes that pattern next: the temper is order-dependent, so `PRs … from the PM tool … (code host)` would still pass. Not widened here — the guard belongs to #236 and this is a main-fix, not a scope change. 2. **#408 + #406.** The bootstrap sibling's mirror shipped `[SKILL.md](SKILL.md)` where the real pipeline emits `[SKILL.md](./SKILL.md)` — it prepends `./` to a same-dir link when the file moves. #408's own sibling-parity test tolerates exactly that shape (it calls `neutralizeSameDirDotSlash` on both sides), while #406's widened mirror guard does not. So the escape hatch in one test is what let it reach main, where the other guard caught it. Both fixes are one line. What is not one line is the pattern: four times today a merge of two green PRs produced a red main. The common cause is that every guard runs against its own branch, so an interaction is only ever observable after the second merge — and nothing runs the full gate on the post-merge state before it is pushed. `pnpm quality-gate` green — 22 + 12 tasks.
rucka
added a commit
that referenced
this pull request
Jul 31, 2026
…ed flatten + link rebase + config wiring (#411) * [US-407] fix: bound the flatten depth so a skill's nested references/ installs inside it Test-first, per the project's bug workflow: the failing cases were written and verified RED (`process-review-references` instead of `process-review/references`) before any code changed. `flattenPath` replaced EVERY separator with a hyphen, so a skill's nested dir became a sibling pseudo-skill: `process/review/references/deep.md` installed at `pair-process-review-references/deep.md`. The first skill to use the standard Agent-Skills `references/` progressive-disclosure layout would install unusable. `flattenDepth` bounds flattening to the registry's ENTRY granularity: the first N segments are joined, deeper ones stay a real sub-path. The skills registry's entries are two segments (`process/review`), so a third is content OF that skill. Omitted ⇒ every separator is flattened, exactly as before — no other registry changes behaviour, which is why this is not a blanket fix to flattenPath. Threaded through SyncOptions -> TransformOpts -> transformPath, so the copy pipeline honours it rather than only the pure path function. - 8 unit cases on flattenPath/transformPath, including a REGRESSION WITNESS that pins the old unbounded behaviour (a bounded-depth bug must not silently become the default for everyone else) - 1 end-to-end case through copyPathOps: the nested dir lands inside the skill, NOT as a sibling, and the skill's forward link needs no rewrite because both files move together HALF THE STORY, AND IT SAYS SO. #407's AC asks for working links in BOTH directions. The sub-doc's back-link `../SKILL.md` is still mangled to `../../../source/process/review/SKILL.md`: `rebaseWithinMovedDir` only rebases targets INSIDE the file's own directory, and `../SKILL.md` points at the PARENT, so it falls through to the source-root fallback. Fixing it means anchoring the rebase at the moved ENTRY rather than at the file's directory — a signature change to the rewriter and its callers, deliberately not rushed. Recorded as an `it.todo` naming the exact cause, not left as a silent gap. `pnpm quality-gate` green. 641 tests in content-ops, 49 files. Refs #407 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [US-407] chore: drop two unrelated prettier reformats from the diff The gate's prettier:fix rewrote apps/pair-cli/src/commands/kb-info/version-check*.test.ts — pre-existing drift on main, nothing to do with this story — and `git add -A` caught them. Reverted to origin/main: that drift belongs to #394 (pre-push gate should run the formatters in write-mode), not to a flatten fix. Refs #407 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [US-407] fix: rebase a link through any directory the same copy moved (link half) Completes #407. Test-first: both cases were written and verified RED before the rewriter changed. `rebaseWithinMovedDir` only rebased targets INSIDE the file's own directory. A sub-doc's link UP to its skill (`../SKILL.md`) points at the PARENT, so it fell through to the source-root fallback and came out as `../../../source/process/review/SKILL.md` — a path back into the dataset layout, dead in an install. `movedDirs` carries every directory this copy moved, built once per batch and threaded rewriteLinksAfterTransform -> rewriteLinksInFile -> computeNewHref -> resolveAbsoluteTarget. A target outside the file's own directory is now rebased through whichever moved directory contains it, most specific first. Absent, the behaviour is unchanged. THE DEFECT WAS NOT LIMITED TO NESTED references/. The second test proves it on the UNBOUNDED flatten that ships today: a sub-doc becoming a sibling had its back-link re-rooted into `source/` too. #407 described a latent defect; it is partly a live one. Live instance found by the mirror-equality guard, which flagged a skill I never touched: `.claude/skills/pair-process-plan-epics/SKILL.md` shipped `[map-subdomains](../../../.skills/capability/map-subdomains/SKILL.md)` — a `.skills/` path that does not exist in an installed project. The corrected transform emits `../pair-capability-map-subdomains/SKILL.md`, pointing at the installed sibling. Regenerated: one line, one file. `computeNewHref` exceeded the complexity ceiling once `movedDirs` was threaded; split via `splitRewritableHref` rather than raising the limit. Caught by the gate, not by vitest — which transpiles without type-checking, and had also let an un-extended inline param type through as green. `pnpm quality-gate` green. content-ops 644 tests / 49 files; the mirror guard 54. Closes #407 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [US-407] fix: a nested content dir is content, not a skill (review round 1) Bounded flatten made a skill's `references/` a real sub-path, so it reached `buildSkillNameMap`/`buildSkillLinkPathMap`/the frontmatter sync as if it were a skill dir: `references` got registered as a SKILL NAME mapped to one arbitrary skill's sub-dir (last writer wins on dir iteration order), and the `/references` token in an UNRELATED skill's body was rewritten to it. Test-first: 4 cases verified RED first. `isRegistryEntryPath` is the one predicate — flattenDepth IS the entry granularity, so anything deeper is content. Absent depth ⇒ every dir is an entry, the pre-#407 behaviour unchanged. Also, per review: - `flattenPath` throws on a non-positive-integer depth (0/-1/1.5 used to degrade silently, in the direction that reintroduces the bug) and refuses a `.`/`..` segment in the preserved tail (`process/review/../../../../etc` joined onto the dest root resolved to `/etc`; the unbounded form was traversal-safe by construction). - one name for the concept: `maxDepth` -> `flattenDepth` everywhere. - `buildSkillNameMap`/`buildSkillLinkPathMap` take the full `TransformOpts` (the narrow declared type lied about reading `flattenDepth`); `TransformOpts` now lives next to `transformPath` and copy-types re-exports it. - `rebaseWithinMovedDirs` picks the longest match in one pass instead of re-sorting per link; stale `rebaseWithinMovedDir` scope paragraph rewritten for the two-stage resolution; misplaced `computeNewHref` JSDoc moved back onto it. Refs #407 * [US-407] fix: wire flattenDepth from config to the copy pipeline (review round 1) The fix was unreachable for the product: `normalizeRegistryConfig` never read `raw['flattenDepth']`, `RegistryConfig` never declared it, `buildCopyOptions` never set it — so update/install/package always ran unbounded and a nested `references/` still installed as the sibling `pair-process-review-references/`. `"flattenDepth": 2` in config.json was silently dropped. - resolver: `RegistryConfig.flattenDepth`, carried through uncoerced. - validation: `validateFlattenDepthField` — positive integer, and rejected without `flatten: true`. A JSON typo now fails loudly instead of degrading back into the defect. - operations: `buildCopyOptions` forwards it. - config.json: `flattenDepth: 2` on the `skills` registry (its entries are `<category>/<name>`). - skill-md-mirror: `SKILL_COPY_OPTS` + module docstring re-pointed at the corrected mapping (the story lists this as in scope); the pin test now asserts flattenDepth against config.json too. Unchanged output for every current skill — no dataset skill dir is deeper than two segments. - skills-guide-mirror: reuses the single `SKILL_COPY_OPTS` instead of re-typing the knobs, so its `collectSkillDirs` (ANY dir holding a file) cannot register a `references/` dir as a skill. CLI-level coverage through `pair update`: nested dir installs inside the skill with both links intact; without the option the same dataset still produces the pre-#407 sibling layout; a `flattenDepth: 0` config is rejected. Refs #407 * [US-407] docs: ADR-020 for the bounded-flatten decision + authoring convention The story flags this as ADR-worthy and CLAUDE.md makes it a HALT condition: the reasoning lived only in the PR body and code comments while ADR-005 still documents flatten as FULL flattening because "AI tools expect skills in flat directory structures". ADR-020 records the numeric-depth form chosen and, on the record, the three alternatives rejected: registry-wide depth-1 (wrong for a two-segment entry), `preserveNested: true` (a flag that still has to infer the entry), and "the dir containing SKILL.md is the entry" — the semantically sharpest option, rejected on COUPLING: it would leak skills-domain knowledge into a transform four non-skill registries share, and make the installed path depend on file discovery rather than declared config. Also records the opt-in default, the duplicated-number trade-off, and the entry-vs-content corollary. ADR-005 amended in place (status, Key Design Choice #2, the Flatten/prefix rationale bullet, the config listing) so it no longer reads as authority for the old semantic. DoD's documentation item: `nested-sub-documents.md` states the authoring convention — a skill may ship a nested `references/`, how it installs, that relative links work in both directions, and that a sub-doc is content (no prefix, no `name:` sync, not invocable). Written generic/portable per that directory's Scope note, so it cites no ADR or issue back; dataset source + root mirror. Closes #407 * [US-407] fix: a registry-root skill cannot ship a sub-dir — fail loudly (review round 2) flattenDepth assumes every entry sits at the same depth; `.skills/next` is a ONE-segment entry beside 39 two-segment ones, so `next/references` had the shape of a real entry and would install as the sibling `pair-next-references` — the whole defect back for that skill, blast radius including AGENTS.md via the skill-name map. Unrepresentable, so copyDirectoryWithTransforms now rejects the shape (a dir shallower than flattenDepth that holds files directly AND owns a sub-dir) before copying anything. Category vs entry told apart by "holds files directly" — no SKILL.md knowledge, per ADR-020's coupling argument. Also from the review: - link-rewriter: most-specific moved-dir match now runs BEFORE the own-dir rebase, so a forward link into a sub-dir that moved elsewhere is rewritten too (own-dir first left `./references/deep.md` dead under an unbounded flatten) - mirror cleanup descends into a bounded entry, so a `references/` removed from the source stops staying installed forever; unbounded path unchanged, gated - naming-transforms: typed IO_ERROR instead of bare Error; depth validated before the empty-path early return; `isValidFlattenDepth` shared with the CLI boundary so the two enforcement points cannot drift - resolveAbsoluteTarget JSDoc: three stages, not two - ADR-020 Trade-offs + nested-sub-documents.md (both copies): the exclusion Refs: #407, PR #411 * [US-407] fix: round 3 — reject an entry DEEPER than flattenDepth, and close the depth-1 traversal hole Two findings from the round-3 review, both real defects this PR introduced. MAJOR — the round-2 guard was asymmetric. It rejected an entry SHALLOWER than flattenDepth and let the deeper case through silently: `capability/sub/foo/SKILL.md` installed at `pair-capability-sub/foo/SKILL.md` — a pseudo-entry directory with NO SKILL.md at its root, invisible to the skill loader. Two further defects followed silently because isRegistryEntryPath reports false for it: the frontmatter `name:` stayed unsynced, and no entry reached skillNameMap, leaving a `/foo` reference in an unrelated skill dangling. Injection-verified: with the new guard disabled the test resolves instead of rejecting, and skillNameMap comes out with ONE entry instead of two. This was a REGRESSION, not an uncovered edge — full flattening produced a perfectly usable `pair-capability-sub-foo/` for the same source. Neither CI gate catches it: skills:conformance only walks <cat>/<sub>/SKILL.md so the file is never read, and the #384 mirror guard derives the installed path from the same transformPath, so it compares equal and passes. `validateNoDeepEntry` uses the shape data already collected, with no SKILL.md knowledge (ADR-020's coupling argument): a directory deeper than flattenDepth is content IFF its nearest ancestor at that depth also holds files directly — that ancestor is the entry owning it. So `process/review/references` still passes (its depth-2 ancestor holds files) while `capability/sub/foo` fails (`capability/sub` holds none). Both cases are tested. MINOR — the traversal guard checked only the preserved tail, and the argument that the head is safe ("every separator becomes a hyphen") fails at flattenDepth 1, where the head is a single un-joined segment: `flattenPath('../evil', 1)` returned `'../evil'`. The bounded form was therefore strictly LESS traversal-safe than the unbounded one it replaces, in exactly the dimension it advertises as hardened. Now every segment is validated, including in the `segments.length <= flattenDepth` branch. Not reachable through the CLI today, which is why it was a hole rather than a live bug. Reverted a cosmetic rewording of the guard's message ("keep" for "preserve") that broke two pre-existing tests — not worth the churn. `pnpm quality-gate` green. content-ops 62 naming-transform cases, 28 copy cases. Refs #407 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [US-407] test: split the mirror fixture — one entry depth per property it can prove Finishes the rebase onto the merged #406. Two fixture properties turned out to need OPPOSITE entry depths, which is why a single fixture could not carry both. - The **link-depth rewrite** is only observable with a ONE-segment entry: `demo/` sits 2 levels below the dataset root while `pair-demo/` sits 3 below the mirror root, so the link up must be recomputed. With a two-segment entry both sides are 3 levels and the rewriter correctly leaves the link untouched — a degenerate case that proves nothing. I moved this fixture to two segments earlier in the rebase and silently destroyed the assertion's purpose; it is back on one segment, with the reason stated in the test. - The **nested `references/`** case needs a TWO-segment entry, because a one-segment entry owning a sub-directory is refused outright by the shallow-entry guard — and an entry deeper than the flatten depth by the guard added in round 3. So it gets its own `catalog/nested` entry. Also inverted the two assertions that pinned the OLD flatten mapping. They said so by name — "as the pipeline does today (#407)" — and were written knowing this PR would invert them: the nested sub-dir is now expected INSIDE its skill, with the sibling shape asserted unreachable. The rebase conflict itself was in the same file: #406 added `behavior` to the registry-pin test while this PR added `flattenDepth`. Both kept. `pnpm quality-gate` green — mirror suite 78, content-ops 649. Refs #407 * [US-407] fix: round 4 — public docs for flattenDepth, broader rule stated + enforced Review round 4 on PR #411 (11 findings, all resolved, none escalated). - website reference: flattenDepth row, bounded-flatten section, skills example (+ stale behavior mirror->overwrite there and in ADR-005 #5) - guard rule documented AS ENFORCED (a category dir with a file hits it too), remediation names that way out, child depth no longer mis-interpolated - buildTransformOpts drops flattenDepth when flatten is false — one source of truth for "bounded?", no unbounded transform + bounded classification - traversal guard applied by SHAPE: a single segment ('..') is refused in BOTH forms, so bounded is neither less nor more safe than unbounded - skills:conformance fails on a SKILL.md below the entry depth (authoring rule 1 of nested-sub-documents was stated but unenforced) - .pair/llms.txt regenerated (nested-sub-documents + pre-existing coupling-balance) - nested mirror cleanup labelled forward-compat, ACCEPTED RESIDUAL in ADR-020 - layout validators extracted to copy/layout-validation.ts - misattributed test comment moved onto its real subject; stale #407 caveats in skill-md-mirror corrected Refs #407 * [US-407] docs: round 5 — say what the code enforces (remedy, mixed depth, guard asymmetry) - too-deep error: second way out qualified as content-only; an entrypoint inside content installs as content, pinned by test (consumers have no conformance gate) - website Bounded Flattening: mixed depth is workable — shallower entry aborts only once it owns a sub-dir; managing-ai-artifacts examples gain flattenDepth: 2 - flattenPath docstring/ADR-020: real invariant is 'each form checked where its own output can carry a live ..' — not equal strictness; tail check is stricter deliberately, do not 'restore symmetry' - nested-sub-documents: heading + lead reworded to the enforced rule, new rule 5 (link a sub-doc only from within its skill — cross-registry maps entrypoints only) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [US-407] fix: round-4 review — null flattenDepth reaches validation, docs/ADR wording - resolver: '!== undefined' so an explicit null is rejected by name instead of silently restoring the unbounded flatten (+ test). - adr-020: the depth lead-in claimed every deviation aborts; neither guard fires on depth alone. - skill-md-mirror docstring: drop the pre-#407 unbounded example stated in the present tense. - layout-validation docstring: name the real audience of the qualified remedy. - configuration.mdx: flattenDepth in the validation list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Title: [US-384] chore: mirror-equality guard widened to every dataset skill artifact
Story/Epic: Closes #384 (builds on #352, PR #377)
Type: Chore (tech-debt)
Labels: tech-debt, risk:yellow
Priority: P2
Summary
What Changed
Widens the #352 mirror-equality guard (
packages/knowledge-hub/src/tools/skill-md-mirror.ts+ colocated.test.ts) from asserting only each skill'sSKILL.mdto asserting EVERY markdown artifact a dataset skill dir contributes through the realpair updatecopy pipeline — sub-docs today (process/bootstrap/assess-orchestration.md,process/review/merge-and-cascade.md,process/review/degradation-levels.md,process/implement/post-review-merge.md), any futurereferences/*automatically.Why This Change
The #352 guard deliberately scoped to
SKILL.md; a hand-edit to a root sub-doc could drift silently from its dataset source until someone regenerated and lost it. This closes that gap the same way #352 closed it forSKILL.md: real pipeline, no parallel transform re-implementation, data-driven so it never needs a test edit as artifacts are added.Story Context
User Story: As a maintainer of the Pair knowledge base, I want the mirror-equality guard to assert every artifact a skill directory contributes, not only its
SKILL.md, so a hand-edit to a root sub-doc is caught in CI instead of drifting silently.Acceptance Criteria: AC1-AC5, see mapping below.
Changes Made
Implementation Details
datasetSkillArtifacts(tree)— every.mdpath the dataset contributes, recursive, markdown-only, sorted, dataset-derived — never a hardcoded list, never a count (AC1/AC3).installedArtifactPath(datasetArtifact)— root location via the REALtransformPath, correctly flattening a nested sub-directory into its own top-level prefixed dir (process/review/references/deep.md→pair-process-review-references/deep.md) exactly as the copy pipeline does.buildInstalledArtifacts(tree)— one realcopyDirectoryWithTransformsrun over an in-memory dataset clone →{ byDatasetPath, producedPaths }.producedPathsis the pipeline's ACTUAL output set, cross-checked set-for-set againstinstalledArtifactPath's derivation, so a wrong path-mapping assumption can no longer silently drop an artifact from the assertion instead of failing loudly.assertRootArtifactMatches— same contract as tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352'sassertRootSkillMdMatches(names the artifact by dataset path, compactdiffSkillMdline diff, "missing → runpair update" hint), now the single assertion helper for bothSKILL.mdand every sub-doc.SKILL.mdartifacts on a syntheticdemo/sub-doc.md+demo/references/deep.mdfixture (hermetic, not the real 4): content drift, sibling self-pointer normalisation (SKILL.md→./SKILL.md), link-depth bump,/commandskill-reference rewrite, missing root copy — each proven RED then green.Files Changed
packages/knowledge-hub/src/tools/skill-md-mirror.ts(extended production helper)packages/knowledge-hub/src/tools/skill-md-mirror.test.ts(extended guard + drift-injection suite)No other files touched — confirmed disjoint from every other currently open PR's file footprint (#386-391, #404, #405).
Testing
Test Coverage
packages/knowledge-hubfull suite: 542 tests pass, 78 inskill-md-mirror.test.ts. Baseline for the delta is currentorigin/main, measured, not tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352's merge-time count: 54 → 78 (+24) — main is 14 static cases +it.each(skillDirs)over 40 dataset skill dirs; this branch is 34 static cases +it.each(artifacts)over 44 dataset markdown artifacts (40SKILL.md+ 4 sub-docs).AC → Test Mapping
it.each(artifacts)main guard — 44 cases (main asserts 40,SKILL.md-only)/commanddrift testsdatasetSkillArtifactsderivation + nested-subdir recursion testpair update)FAILS loudly when the sub-doc root copy is missing/ nested-reference variant; plusroot-copy read distinguishes a missing copy from an unreadable one(EXISTS-but-unreadable is rethrown with its path + cause, never mislabelled "missing → runpair update")directional guard ignores root-only artifacts...—agent-browser(whole root-only skill) and a strayscratch.mdreally written into the pipeline's destination tree (InstalledMirror.root.write, then the produced set re-derived) so the case is a fixture, not an absent-by-construction assertionAC numbering in code comments is #384's throughout (AC2 = drift, AC3 = data-driven, AC4 = missing/unreadable root copy, AC5 = directional); #352's numbering is no longer referenced anywhere in the file.
Test Results
T-6 — Reconciling pre-existing drift (mandatory per the story, precedent #352 → root carried a fix the dataset lacked)
Outcome: no drift found, nothing reconciled. Ran the extended guard against the current
mainstate for all 4 real sub-docs — all pass. Tracedgit logon the one artifact a prior investigation flagged (assess-orchestration.md), per copy — the two sides have DIFFERENT last-touch commits:.claude/skills/pair-process-bootstrap/assess-orchestration.md7bc2f50c— "make pair update self-consistent for the dogfooded root install" (PR #346)packages/knowledge-hub/dataset/.skills/process/bootstrap/assess-orchestration.md66cbfc44— progressive disclosure for implement/review/bootstrap (PR #331)So the ROOT side was reconciled by #346 (which changed only the root copy) while the DATASET side is unchanged since #331 — i.e. #346 brought the root copy back in line with the then-current dataset source, and the pair has been consistent ever since. Nothing is left to reconcile in this PR. The verdict does not rest on that history: it is independently verified by the extended guard passing against the real dataset for all 4 sub-docs. Flagged explicitly because the story's DoD requires T-6 to be reported even when the finding is "nothing to reconcile," so a reviewer doesn't go looking for a reconciliation commit that isn't there.
Known Behavior & Accepted Residuals (decided, recorded in the module docblock)
references/flatten is a DEFECT, not a sanctioned layout — tracked as tech-debt: pair update mis-installs a skill's nested references/ subdir (flattened out of the skill, both links broken) #407. The pipeline installsprocess/review/references/deep.mdaspair-process-review-references/deep.md(outside its skill dir, both links broken). This guard mirrors that behavior faithfully so it cannot lie about the mirror; the docblock oninstalledArtifactPath, the test-suite docblock and the test name all point at tech-debt: pair update mis-installs a skill's nested references/ subdir (flattened out of the skill, both links broken) #407 so the next author adding areferences/sub-doc does not read the mapping as endorsed. tech-debt: pair update mis-installs a skill's nested references/ subdir (flattened out of the skill, both links broken) #407's fix flips these expectations.pair updatecopies with behavioroverwrite(no mirror-delete; the resolved options are asserted against the registry's declaredbehaviorby theSKILL_COPY_OPTSpin test, viaskillCopySyncOptions(), so a flip tomirrorfails there instead of quietly invalidating this residual), so a sub-doc deleted from the dataset but left in.claude/skills/<skill>/is neither asserted nor cleaned. Detecting it needs a non-directional check that must exempt root-only skills (agent-browser); that inversion is deliberately out of this guard's contract ("every dataset artifact is faithfully mirrored") — regenerating deletions belongs topair update. Recorded as anACCEPTED RESIDUALblock in the module docblock..DS_Store, editor backups) must NEVER be asserted; (2) legitimate non-markdown assets (thetemplates/*.shpatternagent-browserships) are real content whose equality is an accepted residual, out of tech-debt: extend mirror-equality guard to all root skill artifacts (sub-docs + references) #384's scope by explicit story decision (Edge Cases exclude non-markdown) — a byte-equality check for opaque assets is a different, simpler guard. The docblock states it as a decision, not an accident.skillCopySyncOptions+ docs), summary still coherent (below the DR-1 recognition threshold). If it grows again, the seam to extract islineEditScript/diffSkillMd.Quality Assurance
Code Quality Checklist
copyDirectoryWithTransformspipeline as tech-debt: extend mirror-equality guard to per-skill SKILL.md pairs (all 36 skills) #352content-opscopy pipeline's own informational logs during the in-memory run are unrelated to this PR)Risk Assessment
Classification carried from refinement:
risk:yellow(KB-default service criticality) ·cost:green(test-only, one module + its test, no runtime path).producedPathscross-check assertsinstalledArtifactPath's derivation against the pipeline's real output set-for-setdatasetSkillArtifactsderives the case list from the dataset tree at collection time; explicitly tested to require no count assertionReviewer Guide
Review Focus Areas
buildInstalledArtifactsdrives the real production pipeline (no parallel transform logic) and thatproducedPathsgenuinely cross-checksinstalledArtifactPath's derivation rather than trusting it.references/*file (not justSKILL.md), across every drift class.main(that is the evidence). Thegit logtrace in the T-6 table is history only — root reconciled by7bc2f50c(fix: make pair update self-consistent for the dogfooded root install #346, root side only), dataset unchanged since66cbfc44([#313] refactor: progressive disclosure for implement/review/bootstrap (task T5) #331).Testing the Changes
git checkout chore/US-384-mirror-guard-all-skill-artifacts pnpm --filter @pair/knowledge-hub testDependencies & Related Work
Related PRs
SKILL.mdguard this widens.