From 5873c1a8e633d4a5f4e9efa16f1b10a4b326ef73 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 14:17:02 +0200 Subject: [PATCH 01/12] [US-407] fix: bound the flatten depth so a skill's nested references/ installs inside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../kb-info/version-check-formatter.test.ts | 2 +- .../commands/kb-info/version-check.test.ts | 4 +- packages/content-ops/src/ops/SyncOptions.ts | 9 +++ .../copy/copy-directory-transforms.test.ts | 42 +++++++++++++ .../src/ops/copy/copy-directory-transforms.ts | 4 +- .../content-ops/src/ops/copy/copy-types.ts | 7 ++- .../src/ops/naming-transforms.test.ts | 59 +++++++++++++++++++ .../content-ops/src/ops/naming-transforms.ts | 32 ++++++++-- 8 files changed, 149 insertions(+), 10 deletions(-) diff --git a/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts b/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts index 5909c13e4..59b56adba 100644 --- a/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts +++ b/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts @@ -82,7 +82,7 @@ describe('formatVersionCheckHuman', () => { ) const output = formatVersionCheckHuman(result) - const installedLine = output.split('\n').find((l) => l.includes('Installed:')) ?? '' + const installedLine = output.split('\n').find(l => l.includes('Installed:')) ?? '' expect(installedLine).toContain('non-stable') }) }) diff --git a/apps/pair-cli/src/commands/kb-info/version-check.test.ts b/apps/pair-cli/src/commands/kb-info/version-check.test.ts index 090a9d678..b71a6b683 100644 --- a/apps/pair-cli/src/commands/kb-info/version-check.test.ts +++ b/apps/pair-cli/src/commands/kb-info/version-check.test.ts @@ -28,9 +28,7 @@ describe('compareVersions', () => { const result = compareVersions(installed({ version: '1.1.0' }), current({ version: '1.2.0' })) expect(result.status).toBe('drift') - expect(result.migrationUrl).toBe( - 'https://pair.foomakers.com/docs/migrations/v1.1.0-to-v1.2.0', - ) + expect(result.migrationUrl).toBe('https://pair.foomakers.com/docs/migrations/v1.1.0-to-v1.2.0') }) it('reports drift WITHOUT a migration URL on downgrade (installed newer than current)', () => { diff --git a/packages/content-ops/src/ops/SyncOptions.ts b/packages/content-ops/src/ops/SyncOptions.ts index 4848f0835..97bf9fa53 100644 --- a/packages/content-ops/src/ops/SyncOptions.ts +++ b/packages/content-ops/src/ops/SyncOptions.ts @@ -19,6 +19,15 @@ export type SyncOptions = { include: string[] /** Flatten directory hierarchy into hyphen-separated names */ flatten: boolean + /** + * Bound flattening to the registry's ENTRY granularity: only the first + * `flattenDepth` 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 — without this it installed as the sibling + * `pair-process-review-references/` with both relative links dead (#407). + * Omitted ⇒ every separator is flattened, exactly as before. + */ + flattenDepth?: number /** Prefix to prepend to top-level directory names */ prefix?: string /** Target configurations for multi-target distribution (empty array = no targets) */ diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts index e166960f6..2a140aff6 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts @@ -361,6 +361,48 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => expect(skill).toContain('[edge cases](./edge-cases.md)') expect(edgeCases).toContain('[SKILL](./SKILL.md)') }) + // #407 (placement half — landed). The standard Agent-Skills + // progressive-disclosure layout. Before `flattenDepth`, this installed as the + // SIBLING dir `pair-process-review-references/`: not a sub-doc of the skill at + // all, so the skill's own `./references/deep.md` pointed at nothing. + it('installs a nested references/ dir INSIDE the skill, not as a sibling (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/review/SKILL.md': + '---\nname: review\n---\n# /review\nDetail in [deep dive](./references/deep.md).', + '/dataset/source/process/review/references/deep.md': + '# Deep dive\nBack to [SKILL](../SKILL.md).', + }) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }) + + expect( + await fileService.exists('/dataset/target/pair-process-review/references/deep.md'), + ).toBe(true) + // ...and NOT as a sibling pseudo-skill, which was the defect. + expect( + await fileService.exists('/dataset/target/pair-process-review-references/deep.md'), + ).toBe(false) + + // The skill's FORWARD link is correct with no rewrite: both files moved + // together, so the same-dir-relative href still resolves. + const skill = await fileService.readFile('/dataset/target/pair-process-review/SKILL.md') + expect(skill).toContain('[deep dive](./references/deep.md)') + }) + + // The remaining half of #407, deliberately NOT asserted as passing. + // `rebaseWithinMovedDir` only rebases targets INSIDE the file's own directory; + // `../SKILL.md` points at the PARENT, so it falls through to the source-root + // fallback and becomes `../../../source/process/review/SKILL.md` — a path back + // into the dataset layout. Fixing it means anchoring the rebase at the moved + // ENTRY (`process/review` → `pair-process-review`) rather than at the file's + // directory, which changes the rewriter's signature and its callers. + it.todo('rewrites a nested sub-doc back-link to stay ../SKILL.md (#407, link half)') }) describe('mirror behavior — idempotent updates (AC4)', () => { diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts index 0a630ddc5..d92f662b2 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts @@ -213,7 +213,9 @@ async function copyAllFilesWithTransform(params: { function buildTransformOpts(options?: SyncOptions): TransformOpts { const flatten = options?.flatten ?? false const prefix = options?.prefix - return prefix ? { flatten, prefix } : { flatten } + const flattenDepth = options?.flattenDepth + const base: TransformOpts = flattenDepth === undefined ? { flatten } : { flatten, flattenDepth } + return prefix ? { ...base, prefix } : base } export async function copyDirectoryWithTransforms(params: { diff --git a/packages/content-ops/src/ops/copy/copy-types.ts b/packages/content-ops/src/ops/copy/copy-types.ts index 158f8f896..359fd6af7 100644 --- a/packages/content-ops/src/ops/copy/copy-types.ts +++ b/packages/content-ops/src/ops/copy/copy-types.ts @@ -11,4 +11,9 @@ export type CopyPathOpsResult = { } /** Naming transform options (flatten and/or prefix) applied during a copy. */ -export type TransformOpts = { flatten: boolean; prefix?: string } +/** + * `flattenDepth` bounds flattening to the registry's entry granularity, so a + * deeper segment is preserved as a real sub-path instead of becoming a sibling + * entry. Omitted ⇒ every separator is flattened, as before (#407). + */ +export type TransformOpts = { flatten: boolean; prefix?: string; flattenDepth?: number } diff --git a/packages/content-ops/src/ops/naming-transforms.test.ts b/packages/content-ops/src/ops/naming-transforms.test.ts index 1e27aa28f..106cce44a 100644 --- a/packages/content-ops/src/ops/naming-transforms.test.ts +++ b/packages/content-ops/src/ops/naming-transforms.test.ts @@ -101,3 +101,62 @@ describe('detectCollisions', () => { expect(detectCollisions(['a-b'])).toEqual([]) }) }) + +// --------------------------------------------------------------------------- +// #407 — a skill's nested `references/` sub-dir must install INSIDE the skill +// --------------------------------------------------------------------------- +// +// The skills registry's entry granularity is TWO segments (`process/review`, +// `capability/write-issue`), so a deeper segment is content *of* that entry, not +// a separate one. Flattening every slash turned `process/review/references` into +// the sibling pseudo-skill `pair-process-review-references`, which breaks the +// skill's link to `./references/deep.md` and the sub-doc's link back up. +// +// `flattenDepth` bounds the flattening to the entry: segments beyond it are +// preserved. Absent, behaviour is unchanged — every other registry keeps +// flattening everything, so this cannot regress them. +describe('flattenPath with a bounded depth (#407)', () => { + it('flattens exactly the entry segments and preserves what is below', () => { + expect(flattenPath('process/review/references', 2)).toBe('process-review/references') + }) + + it('preserves more than one level below the entry', () => { + expect(flattenPath('process/review/references/deep', 2)).toBe('process-review/references/deep') + }) + + it('leaves an entry-depth path exactly as the unbounded form does', () => { + expect(flattenPath('process/review', 2)).toBe(flattenPath('process/review')) + }) + + it('is a no-op beyond the available segments (a shallower entry is not padded)', () => { + expect(flattenPath('review', 2)).toBe('review') + }) + + it('without a depth, flattens everything — unchanged for every other registry', () => { + expect(flattenPath('process/review/references')).toBe('process-review-references') + }) +}) + +describe('transformPath with a bounded flatten depth (#407)', () => { + it('prefixes the entry and keeps the nested dir underneath it', () => { + expect( + transformPath('process/review/references', { + flatten: true, + prefix: 'pair', + flattenDepth: 2, + }), + ).toBe('pair-process-review/references') + }) + + it('is identical to today for a path at entry depth', () => { + expect( + transformPath('process/review', { flatten: true, prefix: 'pair', flattenDepth: 2 }), + ).toBe('pair-process-review') + }) + + it('reproduces the #407 defect when the depth is absent (regression witness)', () => { + expect(transformPath('process/review/references', { flatten: true, prefix: 'pair' })).toBe( + 'pair-process-review-references', + ) + }) +}) diff --git a/packages/content-ops/src/ops/naming-transforms.ts b/packages/content-ops/src/ops/naming-transforms.ts index dd76221e1..c2f7a0252 100644 --- a/packages/content-ops/src/ops/naming-transforms.ts +++ b/packages/content-ops/src/ops/naming-transforms.ts @@ -8,11 +8,32 @@ /** * Flatten a path by replacing directory separators with hyphens. * Example: 'catalog/next' → 'catalog-next' + * + * `maxDepth` bounds the flattening to a registry's **entry granularity**: only + * the first `maxDepth` segments are joined, and anything deeper is preserved as + * a real sub-path. + * Example: 'process/review/references' with maxDepth 2 → 'process-review/references' + * + * Why it exists (#407): the skills registry's entries are two segments deep + * (`process/review`), so a third segment is content *of* that skill, not a + * separate skill. Flattening every slash installed it as the SIBLING pseudo-skill + * `pair-process-review-references`, which breaks the skill's own link to + * `./references/deep.md` and the sub-doc's link back up — the first skill using + * the standard `references/` progressive-disclosure layout would install unusable. + * + * Omitted ⇒ every separator is flattened, exactly as before. Registries whose + * entries are single-segment are unaffected either way. */ -export function flattenPath(dirName: string): string { +export function flattenPath(dirName: string, maxDepth?: number): string { const trimmed = dirName.replace(/^\/+/, '').replace(/\/+$/, '') if (trimmed === '') return '' - return trimmed.replace(/\//g, '-') + if (maxDepth === undefined || maxDepth < 1) return trimmed.replace(/\//g, '-') + + const segments = trimmed.split('/') + // Fewer segments than the entry depth: nothing below the entry to preserve, so + // this is the unbounded result — a shallower entry is never padded. + if (segments.length <= maxDepth) return segments.join('-') + return [segments.slice(0, maxDepth).join('-'), ...segments.slice(maxDepth)].join('/') } /** @@ -38,13 +59,16 @@ export function prefixPath(dirName: string, prefix: string): string { */ export function transformPath( dirName: string, - options: { flatten?: boolean; prefix?: string }, + options: { flatten?: boolean; prefix?: string; flattenDepth?: number }, ): string { let result = dirName if (options.flatten) { - result = flattenPath(result) + result = flattenPath(result, options.flattenDepth) } if (options.prefix) { + // prefixPath already prefixes only the top-level segment, so a preserved + // sub-path stays under the prefixed entry: 'process-review/references' → + // 'pair-process-review/references'. result = prefixPath(result, options.prefix) } return result From f0e09451d0095ddf289d56110a3971c39a9bdd53 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 14:19:03 +0200 Subject: [PATCH 02/12] [US-407] chore: drop two unrelated prettier reformats from the diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/commands/kb-info/version-check-formatter.test.ts | 2 +- apps/pair-cli/src/commands/kb-info/version-check.test.ts | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts b/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts index 59b56adba..5909c13e4 100644 --- a/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts +++ b/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts @@ -82,7 +82,7 @@ describe('formatVersionCheckHuman', () => { ) const output = formatVersionCheckHuman(result) - const installedLine = output.split('\n').find(l => l.includes('Installed:')) ?? '' + const installedLine = output.split('\n').find((l) => l.includes('Installed:')) ?? '' expect(installedLine).toContain('non-stable') }) }) diff --git a/apps/pair-cli/src/commands/kb-info/version-check.test.ts b/apps/pair-cli/src/commands/kb-info/version-check.test.ts index b71a6b683..090a9d678 100644 --- a/apps/pair-cli/src/commands/kb-info/version-check.test.ts +++ b/apps/pair-cli/src/commands/kb-info/version-check.test.ts @@ -28,7 +28,9 @@ describe('compareVersions', () => { const result = compareVersions(installed({ version: '1.1.0' }), current({ version: '1.2.0' })) expect(result.status).toBe('drift') - expect(result.migrationUrl).toBe('https://pair.foomakers.com/docs/migrations/v1.1.0-to-v1.2.0') + expect(result.migrationUrl).toBe( + 'https://pair.foomakers.com/docs/migrations/v1.1.0-to-v1.2.0', + ) }) it('reports drift WITHOUT a migration URL on downgrade (installed newer than current)', () => { From 168cb404e76f7245de78a8cd61a0b564d0d89784 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 14:56:22 +0200 Subject: [PATCH 03/12] [US-407] fix: rebase a link through any directory the same copy moved (link half) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../skills/pair-process-plan-epics/SKILL.md | 2 +- .../copy/copy-directory-transforms.test.ts | 62 ++++++++++++++--- packages/content-ops/src/ops/link-rewriter.ts | 67 +++++++++++++++++-- 3 files changed, 116 insertions(+), 15 deletions(-) diff --git a/.claude/skills/pair-process-plan-epics/SKILL.md b/.claude/skills/pair-process-plan-epics/SKILL.md index acc6df308..1c280922d 100644 --- a/.claude/skills/pair-process-plan-epics/SKILL.md +++ b/.claude/skills/pair-process-plan-epics/SKILL.md @@ -152,4 +152,4 @@ See [graceful degradation](../../../.pair/knowledge/guidelines/technical-standar - This skill **modifies PM tool state** — creates and extends epic issues linked to initiatives. - **Idempotent** — see [idempotency convention](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md) and [to-issues-triage.md](../../../.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/to-issues-triage.md). This skill's check: exact idempotency-key match is proposed `ALREADY EXISTS #ID` (skip) at triage time, before any write (Step 3) — Step 4 only executes the confirmed proposal; substantial-overlap match proposes EXTEND instead of a duplicate CREATE (Step 3) — re-running the same candidate tree never duplicates. - Epic 0 rule: for new projects, always assess if a bootstrap/foundation epic is needed before functional epics. -- Domain mapping (Step 3.5) is scoped to this run's epic breakdown — see [map-subdomains](../../../.skills/capability/map-subdomains/SKILL.md). +- Domain mapping (Step 3.5) is scoped to this run's epic breakdown — see [map-subdomains](../pair-capability-map-subdomains/SKILL.md). diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts index 2a140aff6..226c8ba90 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts @@ -395,14 +395,60 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => expect(skill).toContain('[deep dive](./references/deep.md)') }) - // The remaining half of #407, deliberately NOT asserted as passing. - // `rebaseWithinMovedDir` only rebases targets INSIDE the file's own directory; - // `../SKILL.md` points at the PARENT, so it falls through to the source-root - // fallback and becomes `../../../source/process/review/SKILL.md` — a path back - // into the dataset layout. Fixing it means anchoring the rebase at the moved - // ENTRY (`process/review` → `pair-process-review`) rather than at the file's - // directory, which changes the rewriter's signature and its callers. - it.todo('rewrites a nested sub-doc back-link to stay ../SKILL.md (#407, link half)') + // The link half of #407. A sub-doc's link UP to its skill points at the + // PARENT directory, which the file's own dir-mapping cannot rebase — it only + // covers targets inside itself. Before the fix this fell through to the + // source-root fallback and became `../../../source/process/review/SKILL.md`: + // a path back into the dataset layout, dead in the install. + it('keeps a nested sub-doc back-link pointing at its own skill (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/review/SKILL.md': + '---\nname: review\n---\n# /review\nDetail in [deep dive](./references/deep.md).', + '/dataset/source/process/review/references/deep.md': + '# Deep dive\nBack to [SKILL](../SKILL.md).', + }) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }) + + const deep = await fileService.readFile( + '/dataset/target/pair-process-review/references/deep.md', + ) + // Both files moved together, so the relative path is still correct and the + // rewriter must leave it alone rather than re-root it. + expect(deep).toContain('[SKILL](../SKILL.md)') + expect(deep).not.toContain('source/process/review') + }) + + // The sibling case, which the same mechanism has to get right: when the + // nested dir does NOT move with its parent (unbounded flatten), the link up + // must be REWRITTEN to wherever the parent landed — not left alone. + it('rewrites the back-link when the nested dir becomes a sibling (unbounded)', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + '/dataset/source/process/review/references/deep.md': + '# Deep dive\nBack to [SKILL](../SKILL.md).', + }) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', targets: [] }, + }) + + const deep = await fileService.readFile( + '/dataset/target/pair-process-review-references/deep.md', + ) + expect(deep).toContain('[SKILL](../pair-process-review/SKILL.md)') + expect(deep).not.toContain('source/process/review') + }) }) describe('mirror behavior — idempotent updates (AC4)', () => { diff --git a/packages/content-ops/src/ops/link-rewriter.ts b/packages/content-ops/src/ops/link-rewriter.ts index 6d8c1620c..d7e659f4a 100644 --- a/packages/content-ops/src/ops/link-rewriter.ts +++ b/packages/content-ops/src/ops/link-rewriter.ts @@ -18,6 +18,14 @@ export type RewriteLinksInFileParams = { * `packages/kb/dataset`), links resolving under that subtree are re-rooted * to datasetRoot so they point to the installed copy, not the source. */ sourceContentRoot?: string + /** + * Every directory this copy moved, as absolute paths. A link can point OUTSIDE + * the file's own directory — a sub-doc's `../SKILL.md` points at its parent — + * and that parent may have moved too. Without this the target fell through to + * the source-root fallback and came out as a path back into the dataset layout, + * dead in the install (#407). Optional: absent, behaviour is unchanged. + */ + movedDirs?: Array<{ originalDir: string; newDir: string }> } /** @@ -93,6 +101,7 @@ type ComputeNewHrefParams = { newFileDir: string datasetRoot?: string sourceContentRoot?: string + movedDirs?: Array<{ originalDir: string; newDir: string }> } /** @@ -106,31 +115,67 @@ function resolveAbsoluteTarget(params: { pathPart: string datasetRoot?: string sourceContentRoot?: string + movedDirs?: Array<{ originalDir: string; newDir: string }> }): string { - const { originalFileDir, newFileDir, pathPart, datasetRoot, sourceContentRoot } = params + const { originalFileDir, newFileDir, pathPart, datasetRoot, sourceContentRoot, movedDirs } = + params const absoluteTarget = posix.resolve(originalFileDir, pathPart) const rebased = rebaseWithinMovedDir(absoluteTarget, originalFileDir, newFileDir) if (rebased) return rebased + // The target is outside the file's own directory. It may still live in another + // directory this same copy moved — the commonest case being a sub-doc linking + // UP to its skill. Most specific (longest originalDir) wins, so a nested entry + // is preferred over its parent. + const viaSibling = rebaseWithinMovedDirs(absoluteTarget, movedDirs) + if (viaSibling) return viaSibling if (datasetRoot && sourceContentRoot) { return reRootTarget(absoluteTarget, datasetRoot, sourceContentRoot) } return absoluteTarget } +/** + * Rebase a target through whichever moved directory contains it, preferring the + * most specific match. Returns null when no moved directory covers it, so the + * caller keeps its existing fallbacks. + */ +function rebaseWithinMovedDirs( + absoluteTarget: string, + movedDirs?: Array<{ originalDir: string; newDir: string }>, +): string | null { + if (!movedDirs || movedDirs.length === 0) return null + const candidates = [...movedDirs].sort((a, b) => b.originalDir.length - a.originalDir.length) + for (const { originalDir, newDir } of candidates) { + const rebased = rebaseWithinMovedDir(absoluteTarget, originalDir, newDir) + if (rebased) return rebased + } + return null +} + /** * Computes the new href for a relative link after the file has moved. * Returns null if the link should not be rewritten (external, anchor, unchanged). */ -function computeNewHref(params: ComputeNewHrefParams): string | null { - const { href, originalFileDir, newFileDir, datasetRoot, sourceContentRoot } = params +/** + * Splits a rewritable href into its path and anchor halves, or null when the + * link must be left alone: external, a pure anchor, or anchor-only after the + * split. Extracted from `computeNewHref` to keep it under the complexity ceiling. + */ +function splitRewritableHref(href: string): { pathPart: string; anchorPart: string } | null { if (isExternalLink(href) || href.startsWith('#')) return null - const anchorIdx = href.indexOf('#') const pathPart = anchorIdx >= 0 ? href.slice(0, anchorIdx) : href const anchorPart = anchorIdx >= 0 ? href.slice(anchorIdx) : '' - if (pathPart === '') return null // pure anchor link + return { pathPart, anchorPart } +} + +function computeNewHref(params: ComputeNewHrefParams): string | null { + const { href, originalFileDir, newFileDir, datasetRoot, sourceContentRoot, movedDirs } = params + const split = splitRewritableHref(href) + if (!split) return null + const { pathPart, anchorPart } = split const absoluteTarget = resolveAbsoluteTarget({ originalFileDir, @@ -138,6 +183,7 @@ function computeNewHref(params: ComputeNewHrefParams): string | null { pathPart, ...(datasetRoot && { datasetRoot }), ...(sourceContentRoot && { sourceContentRoot }), + ...(movedDirs && { movedDirs }), }) let newRelativePath = posix.relative(newFileDir, absoluteTarget) @@ -205,7 +251,8 @@ function replaceHrefInNode( * Unresolvable links produce a warning but do not fail. */ export async function rewriteLinksInFile(params: RewriteLinksInFileParams): Promise { - const { fileService, filePath, originalDir, newDir, datasetRoot, sourceContentRoot } = params + const { fileService, filePath, originalDir, newDir, datasetRoot, sourceContentRoot, movedDirs } = + params const content = await fileService.readFile(filePath) const links = await extractLinks(content) @@ -230,6 +277,7 @@ export async function rewriteLinksInFile(params: RewriteLinksInFileParams): Prom newFileDir, datasetRoot, ...(sourceContentRoot && { sourceContentRoot }), + ...(movedDirs && { movedDirs }), }) if (!newHref) continue @@ -254,6 +302,12 @@ export async function rewriteLinksAfterTransform( ): Promise { const { fileService, pathMapping, datasetRoot, sourceContentRoot } = params + // Built once for the whole batch: every directory this copy moved, absolute. + const movedDirs = pathMapping.map(e => ({ + originalDir: posix.join(datasetRoot, e.originalDir), + newDir: posix.join(datasetRoot, e.newDir), + })) + for (const entry of pathMapping) { for (const filePath of entry.files) { if (!filePath.endsWith('.md')) continue @@ -264,6 +318,7 @@ export async function rewriteLinksAfterTransform( newDir: entry.newDir, datasetRoot, ...(sourceContentRoot && { sourceContentRoot }), + movedDirs, }) } } From 65264c707da2a750957325dd0857c0e5bbf4eea9 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 15:38:14 +0200 Subject: [PATCH 04/12] [US-407] fix: a nested content dir is content, not a skill (review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/content-ops/src/index.ts | 9 +- .../copy/copy-directory-transforms.test.ts | 58 ++++++++++++ .../src/ops/copy/copy-directory-transforms.ts | 13 ++- .../content-ops/src/ops/copy/copy-types.ts | 9 +- packages/content-ops/src/ops/link-rewriter.ts | 37 ++++---- .../src/ops/naming-transforms.test.ts | 51 ++++++++++- .../content-ops/src/ops/naming-transforms.ts | 88 ++++++++++++++++--- .../src/ops/skill-reference-rewriter.test.ts | 38 +++++++- .../src/ops/skill-reference-rewriter.ts | 20 ++++- 9 files changed, 283 insertions(+), 40 deletions(-) diff --git a/packages/content-ops/src/index.ts b/packages/content-ops/src/index.ts index 4119260b4..a37699d66 100644 --- a/packages/content-ops/src/index.ts +++ b/packages/content-ops/src/index.ts @@ -55,7 +55,14 @@ export { type TransformConfig, } from './ops/behavior' export { copyPathOps, copyDirectoryWithTransforms, type CopyPathOpsResult } from './ops/copy' -export { flattenPath, prefixPath, transformPath, detectCollisions } from './ops/naming-transforms' +export { + flattenPath, + prefixPath, + transformPath, + detectCollisions, + isRegistryEntryPath, + type TransformOpts, +} from './ops/naming-transforms' export { rewriteLinksInFile, rewriteLinksAfterTransform, diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts index 226c8ba90..83f368f2f 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts @@ -449,6 +449,64 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => expect(deep).toContain('[SKILL](../pair-process-review/SKILL.md)') expect(deep).not.toContain('source/process/review') }) + + // #407 review finding: with a bounded flatten, `process/review/references` + // is a real sub-path and so appears in the dir mapping next to the skill + // dirs. Treating it as a SKILL registered `references` as a skill name + // pointing at ONE arbitrary skill's sub-dir, and the `/references` token in + // an unrelated skill's body was rewritten to it — a cross-skill corruption + // decided by directory iteration order. + it('never rewrites a /references token across skills when both own a references/ dir (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/review/SKILL.md': + '---\nname: review\n---\n# /review\nProgressive disclosure lives under /references.', + '/dataset/source/process/review/references/deep.md': '# Review deep dive', + '/dataset/source/capability/grill/SKILL.md': '---\nname: grill\n---\n# /grill', + '/dataset/source/capability/grill/references/deep.md': '# Grill deep dive', + }) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }) + + const skill = await fileService.readFile('/dataset/target/pair-process-review/SKILL.md') + expect(skill).toContain('under /references.') + expect(skill).not.toContain('pair-capability-grill/references') + // Both skills' own renames still happen — the fix scopes the map, it does + // not disable it. + expect(skill).toContain('# /pair-process-review') + const grill = await fileService.readFile('/dataset/target/pair-capability-grill/SKILL.md') + expect(grill).toContain('# /pair-capability-grill') + }) + + // Same root cause on the frontmatter side: the sub-doc's dir is not a + // renamed skill dir, so its `name:` must be left alone instead of being set + // to a PATH (`pair-process-review/references`). + it('does not rewrite a nested sub-doc frontmatter name to a path (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + '/dataset/source/process/review/references/deep.md': + '---\nname: references\n---\n# Deep dive', + }) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }) + + const deep = await fileService.readFile( + '/dataset/target/pair-process-review/references/deep.md', + ) + expect(deep).toContain('name: references') + expect(deep).not.toContain('pair-process-review/references') + }) }) describe('mirror behavior — idempotent updates (AC4)', () => { diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts index d92f662b2..2f8098ce4 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts @@ -3,7 +3,7 @@ import { logger, createError } from '../../observability' import { copyFileHelper } from '../../file-system' import { FileSystemService } from '../../file-system' import { SyncOptions } from '../SyncOptions' -import { transformPath, detectCollisions } from '../naming-transforms' +import { transformPath, detectCollisions, isRegistryEntryPath } from '../naming-transforms' import { rewriteLinksAfterTransform, PathMappingEntry } from '../link-rewriter' import { syncFrontmatter } from '../frontmatter-transform' import { @@ -104,6 +104,7 @@ async function copyFileWithTransform(ctx: { targetFilePath, dirMappingFiles, topLevelFiles, + isEntryDir: isRegistryEntryPath(dir, transformOpts.flattenDepth), }) } @@ -112,6 +113,12 @@ async function copyFileWithTransform(ctx: { * when a subdirectory was renamed, and records the file in either * `dirMappingFiles` (files under a subdirectory) or `topLevelFiles` (root-level * source files) so link rewriting and mirror cleanup can see it. + * + * `isEntryDir` is false for a directory BELOW the registry's entry granularity + * (a skill's `references/` sub-dir under a bounded flatten, #407). Such a file is + * still tracked for link rewriting, but its frontmatter `name` is left alone: + * the dir it lives in was not renamed into a new entry, so syncing would write a + * PATH (`pair-process-review/references`) as the doc's name. */ async function trackTransformedFile(ctx: { fileService: FileSystemService @@ -121,6 +128,7 @@ async function trackTransformedFile(ctx: { targetFilePath: string dirMappingFiles: Map topLevelFiles: Set + isEntryDir: boolean }): Promise { const { fileService, @@ -130,6 +138,7 @@ async function trackTransformedFile(ctx: { targetFilePath, dirMappingFiles, topLevelFiles, + isEntryDir, } = ctx if (dir === '.') { @@ -142,7 +151,7 @@ async function trackTransformedFile(ctx: { if (!transformedDir) return const leafName = dir.split('/').pop()! - if (leafName !== transformedDir) { + if (isEntryDir && leafName !== transformedDir) { const content = await fileService.readFile(targetFilePath) const synced = syncFrontmatter(content, { from: leafName, to: transformedDir }) if (synced !== content) { diff --git a/packages/content-ops/src/ops/copy/copy-types.ts b/packages/content-ops/src/ops/copy/copy-types.ts index 359fd6af7..81ea4454b 100644 --- a/packages/content-ops/src/ops/copy/copy-types.ts +++ b/packages/content-ops/src/ops/copy/copy-types.ts @@ -10,10 +10,9 @@ export type CopyPathOpsResult = { skillLinkPathMap?: SkillLinkPathMap } -/** Naming transform options (flatten and/or prefix) applied during a copy. */ /** - * `flattenDepth` bounds flattening to the registry's entry granularity, so a - * deeper segment is preserved as a real sub-path instead of becoming a sibling - * entry. Omitted ⇒ every separator is flattened, as before (#407). + * Naming transform options (flatten and/or prefix, optionally depth-bounded). + * Declared next to `transformPath`, which consumes it — re-exported here so the + * copy modules keep importing their types from one place. */ -export type TransformOpts = { flatten: boolean; prefix?: string; flattenDepth?: number } +export type { TransformOpts } from '../naming-transforms' diff --git a/packages/content-ops/src/ops/link-rewriter.ts b/packages/content-ops/src/ops/link-rewriter.ts index d7e659f4a..e8d7652d8 100644 --- a/packages/content-ops/src/ops/link-rewriter.ts +++ b/packages/content-ops/src/ops/link-rewriter.ts @@ -77,11 +77,12 @@ function reRootTarget( * which only knows about the *overall* content-root move and would otherwise * misplace a same-directory sibling link (see #313 T5 fixture regression). * - * Known scope boundary: only rebases links resolving inside the SAME - * directory the current file lives in. A link that escapes to a *different* - * transformed sibling directory (e.g. one skill linking into another skill's - * sibling file) still falls through to `reRootTarget` and is not corrected by - * this function — not needed by any skill in the corpus today. + * Scope: this function alone only rebases links resolving inside the SAME + * directory the current file lives in. Resolution is two-stage (see + * `resolveAbsoluteTarget`): own-dir rebase first, then `rebaseWithinMovedDirs` + * for a target in ANY other directory the same copy moved — a sub-doc's + * `../SKILL.md`, or one skill linking into another (e.g. plan-epics → + * map-subdomains) — and only then the coarser `reRootTarget` fallback. */ function rebaseWithinMovedDir( absoluteTarget: string, @@ -137,26 +138,28 @@ function resolveAbsoluteTarget(params: { /** * Rebase a target through whichever moved directory contains it, preferring the - * most specific match. Returns null when no moved directory covers it, so the - * caller keeps its existing fallbacks. + * most specific match (longest `originalDir`), so a nested entry wins over its + * parent. Returns null when no moved directory covers it, so the caller keeps + * its existing fallbacks. + * + * Picks the longest match in a single pass instead of sorting: this runs once per + * link of every file, and a per-link `[...movedDirs].sort()` would be O(F·L·D + * log D) for a result that does not depend on the input order at all. */ function rebaseWithinMovedDirs( absoluteTarget: string, movedDirs?: Array<{ originalDir: string; newDir: string }>, ): string | null { if (!movedDirs || movedDirs.length === 0) return null - const candidates = [...movedDirs].sort((a, b) => b.originalDir.length - a.originalDir.length) - for (const { originalDir, newDir } of candidates) { + let best: { length: number; rebased: string } | null = null + for (const { originalDir, newDir } of movedDirs) { + if (best !== null && originalDir.length <= best.length) continue const rebased = rebaseWithinMovedDir(absoluteTarget, originalDir, newDir) - if (rebased) return rebased + if (rebased) best = { length: originalDir.length, rebased } } - return null + return best?.rebased ?? null } -/** - * Computes the new href for a relative link after the file has moved. - * Returns null if the link should not be rewritten (external, anchor, unchanged). - */ /** * Splits a rewritable href into its path and anchor halves, or null when the * link must be left alone: external, a pure anchor, or anchor-only after the @@ -171,6 +174,10 @@ function splitRewritableHref(href: string): { pathPart: string; anchorPart: stri return { pathPart, anchorPart } } +/** + * Computes the new href for a relative link after the file has moved. + * Returns null if the link should not be rewritten (external, anchor, unchanged). + */ function computeNewHref(params: ComputeNewHrefParams): string | null { const { href, originalFileDir, newFileDir, datasetRoot, sourceContentRoot, movedDirs } = params const split = splitRewritableHref(href) diff --git a/packages/content-ops/src/ops/naming-transforms.test.ts b/packages/content-ops/src/ops/naming-transforms.test.ts index 106cce44a..ecc38e8fa 100644 --- a/packages/content-ops/src/ops/naming-transforms.test.ts +++ b/packages/content-ops/src/ops/naming-transforms.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect } from 'vitest' -import { flattenPath, prefixPath, transformPath, detectCollisions } from './naming-transforms' +import { + flattenPath, + prefixPath, + transformPath, + detectCollisions, + isRegistryEntryPath, +} from './naming-transforms' describe('flattenPath', () => { it('converts nested path separators to hyphens', () => { @@ -135,6 +141,49 @@ describe('flattenPath with a bounded depth (#407)', () => { it('without a depth, flattens everything — unchanged for every other registry', () => { expect(flattenPath('process/review/references')).toBe('process-review-references') }) + + // The value comes from JSON config, so a typo must fail loudly. Degrading + // silently would degrade in the direction that REINTRODUCES #407 (0 and -1 + // used to full-flatten; a fractional depth used to flatten nothing). + it.each([0, -1, 1.5, NaN])('throws on the non-positive-integer depth %s', depth => { + expect(() => flattenPath('a/b/c/d', depth)).toThrow(/positive integer/) + }) + + // The unbounded form is traversal-safe by construction (every separator + // becomes a hyphen). A preserved tail is not: the copy pipeline joins it onto + // the destination root, so `process/review/../../../../etc` would resolve to + // `/etc`. + it('refuses to preserve a .. segment that would escape the destination root', () => { + expect(() => flattenPath('process/review/../../../../etc', 2)).toThrow(/refusing to preserve/) + }) + + it('refuses to preserve a . segment in the tail', () => { + expect(() => flattenPath('process/review/./deep', 2)).toThrow(/refusing to preserve/) + }) + + it('still flattens a .. into a hyphen when it sits within the entry segments', () => { + expect(flattenPath('process/../review', 3)).toBe('process-..-review') + }) +}) + +describe('isRegistryEntryPath (#407)', () => { + it('treats every dir as an entry when the flatten is unbounded', () => { + expect(isRegistryEntryPath('process/review/references')).toBe(true) + }) + + it('treats a dir at or above the entry depth as an entry', () => { + expect(isRegistryEntryPath('process/review', 2)).toBe(true) + expect(isRegistryEntryPath('next', 2)).toBe(true) + }) + + it('treats a dir below the entry depth as content, not an entry', () => { + expect(isRegistryEntryPath('process/review/references', 2)).toBe(false) + expect(isRegistryEntryPath('process/review/references/deep', 2)).toBe(false) + }) + + it('rejects an invalid depth for the same reason flattenPath does', () => { + expect(() => isRegistryEntryPath('process/review', 0)).toThrow(/positive integer/) + }) }) describe('transformPath with a bounded flatten depth (#407)', () => { diff --git a/packages/content-ops/src/ops/naming-transforms.ts b/packages/content-ops/src/ops/naming-transforms.ts index c2f7a0252..41a4c010b 100644 --- a/packages/content-ops/src/ops/naming-transforms.ts +++ b/packages/content-ops/src/ops/naming-transforms.ts @@ -5,14 +5,52 @@ * Both operations are independent and composable. */ +/** + * Naming transform options applied during a copy. One name for the concept + * everywhere it travels: `SyncOptions.flattenDepth` → + * `TransformOpts.flattenDepth` → `transformPath({ flattenDepth })` → + * `flattenPath(dir, flattenDepth)`. + * + * `flattenDepth` bounds flattening to the registry's ENTRY granularity, so a + * deeper segment is preserved as a real sub-path instead of becoming a sibling + * entry. Omitted ⇒ every separator is flattened, as before (#407). + */ +export type TransformOpts = { flatten: boolean; prefix?: string; flattenDepth?: number } + +/** A `flattenDepth` given as a JSON-config typo must fail loudly, not silently full-flatten. */ +function assertValidFlattenDepth(flattenDepth: number): void { + if (!Number.isInteger(flattenDepth) || flattenDepth < 1) { + throw new Error( + `flattenPath: flattenDepth must be a positive integer, got ${flattenDepth}. ` + + 'Omit it to flatten every separator.', + ) + } +} + +/** + * A preserved tail is joined onto a destination root by the copy pipeline, so a + * `.`/`..` segment in it could escape that root. The unbounded form is + * traversal-safe by construction (every separator becomes a hyphen); the + * bounded form has to say no explicitly. + */ +function assertNoTraversalInTail(tail: string[], dirName: string): void { + const offender = tail.find(segment => segment === '.' || segment === '..') + if (offender !== undefined) { + throw new Error( + `flattenPath: refusing to preserve the relative segment '${offender}' of '${dirName}' — ` + + 'a preserved sub-path is joined onto the destination root and could escape it.', + ) + } +} + /** * Flatten a path by replacing directory separators with hyphens. * Example: 'catalog/next' → 'catalog-next' * - * `maxDepth` bounds the flattening to a registry's **entry granularity**: only - * the first `maxDepth` segments are joined, and anything deeper is preserved as - * a real sub-path. - * Example: 'process/review/references' with maxDepth 2 → 'process-review/references' + * `flattenDepth` bounds the flattening to a registry's **entry granularity**: + * only the first `flattenDepth` segments are joined, and anything deeper is + * preserved as a real sub-path. + * Example: 'process/review/references' with flattenDepth 2 → 'process-review/references' * * Why it exists (#407): the skills registry's entries are two segments deep * (`process/review`), so a third segment is content *of* that skill, not a @@ -23,17 +61,46 @@ * * Omitted ⇒ every separator is flattened, exactly as before. Registries whose * entries are single-segment are unaffected either way. + * + * Throws when `flattenDepth` is present but not a positive integer, and when the + * preserved tail contains a `.`/`..` segment (see the two assertions above). */ -export function flattenPath(dirName: string, maxDepth?: number): string { +export function flattenPath(dirName: string, flattenDepth?: number): string { const trimmed = dirName.replace(/^\/+/, '').replace(/\/+$/, '') if (trimmed === '') return '' - if (maxDepth === undefined || maxDepth < 1) return trimmed.replace(/\//g, '-') + if (flattenDepth === undefined) return trimmed.replace(/\//g, '-') + assertValidFlattenDepth(flattenDepth) const segments = trimmed.split('/') // Fewer segments than the entry depth: nothing below the entry to preserve, so // this is the unbounded result — a shallower entry is never padded. - if (segments.length <= maxDepth) return segments.join('-') - return [segments.slice(0, maxDepth).join('-'), ...segments.slice(maxDepth)].join('/') + if (segments.length <= flattenDepth) return segments.join('-') + const tail = segments.slice(flattenDepth) + assertNoTraversalInTail(tail, dirName) + return [segments.slice(0, flattenDepth).join('-'), ...tail].join('/') +} + +/** + * Whether a source sub-path is one of the registry's own ENTRIES (a skill's own + * directory) rather than CONTENT inside one (a skill's `references/` sub-dir). + * + * Only a bounded flatten can tell the two apart: `flattenDepth` IS the entry + * granularity, so anything deeper is content. Without it every sub-dir becomes + * its own top-level target dir, hence every sub-dir *is* an entry — the + * pre-#407 behaviour, preserved exactly. + * + * Why it matters (#407): the skill-name / skill-link-path maps and the + * frontmatter `name:` sync are keyed on entries. Feeding them a content dir + * registers `references` as a SKILL NAME mapped to `/references`, + * and the `/references` token in an unrelated skill's body then gets rewritten + * to another skill's sub-dir — last writer wins on directory iteration order. + */ +export function isRegistryEntryPath(dirName: string, flattenDepth?: number): boolean { + if (flattenDepth === undefined) return true + const trimmed = dirName.replace(/^\/+/, '').replace(/\/+$/, '') + if (trimmed === '') return true + assertValidFlattenDepth(flattenDepth) + return trimmed.split('/').length <= flattenDepth } /** @@ -57,10 +124,7 @@ export function prefixPath(dirName: string, prefix: string): string { * Apply flatten and/or prefix transformations to a path. * Order: flatten first (if enabled), then prefix (if provided). */ -export function transformPath( - dirName: string, - options: { flatten?: boolean; prefix?: string; flattenDepth?: number }, -): string { +export function transformPath(dirName: string, options: Partial): string { let result = dirName if (options.flatten) { result = flattenPath(result, options.flattenDepth) diff --git a/packages/content-ops/src/ops/skill-reference-rewriter.test.ts b/packages/content-ops/src/ops/skill-reference-rewriter.test.ts index e518bb03b..e8f63c716 100644 --- a/packages/content-ops/src/ops/skill-reference-rewriter.test.ts +++ b/packages/content-ops/src/ops/skill-reference-rewriter.test.ts @@ -246,10 +246,34 @@ describe('buildSkillNameMap', () => { it('skips entries where leaf equals transformed (no rename)', () => { const dirMappingFiles = new Map([['myskill', ['/target/myskill/SKILL.md']]]) - const result = buildSkillNameMap(dirMappingFiles, {}) + const result = buildSkillNameMap(dirMappingFiles, { flatten: false }) expect(result.size).toBe(0) }) + // #407: a bounded flatten makes a skill's `references/` sub-dir a real + // sub-path, so it now shows up in dirMappingFiles alongside the skill dirs. + // It is CONTENT, not a skill: registering `references` as a skill name maps + // the `/references` token to ONE arbitrary skill's sub-dir (last writer wins + // on iteration order) and rewrites it inside every OTHER skill's body too. + it('does not register a content sub-dir as a skill name (#407)', () => { + const dirMappingFiles = new Map([ + ['process/review', ['/t/pair-process-review/SKILL.md']], + ['process/review/references', ['/t/pair-process-review/references/deep.md']], + ['capability/grill', ['/t/pair-capability-grill/SKILL.md']], + ['capability/grill/references', ['/t/pair-capability-grill/references/deep.md']], + ]) + const result = buildSkillNameMap(dirMappingFiles, { + flatten: true, + prefix: 'pair', + flattenDepth: 2, + }) + expect(result.has('references')).toBe(false) + expect([...result.entries()].sort()).toEqual([ + ['grill', 'pair-capability-grill'], + ['review', 'pair-process-review'], + ]) + }) + it('handles multiple entries', () => { const dirMappingFiles = new Map([ ['catalog/next', ['/t/pair-catalog-next/SKILL.md']], @@ -363,6 +387,18 @@ describe('buildSkillLinkPathMap', () => { it('returns an empty map for an empty input', () => { expect(buildSkillLinkPathMap(new Map(), opts).size).toBe(0) }) + + // #407, same reason as buildSkillNameMap: a content sub-dir is not a skill, so + // it must not get a `.skills///SKILL.md` link mapping of its own — + // no such SKILL.md exists inside a `references/` dir. + it('skips a content sub-dir below the entry depth (#407)', () => { + const dirMappingFiles = new Map([ + ['process/review', []], + ['process/review/references', []], + ]) + const map = buildSkillLinkPathMap(dirMappingFiles, { ...opts, flattenDepth: 2 }) + expect([...map.keys()]).toEqual(['../.skills/process/review/SKILL.md']) + }) }) describe('rewriteSkillLinkPaths', () => { diff --git a/packages/content-ops/src/ops/skill-reference-rewriter.ts b/packages/content-ops/src/ops/skill-reference-rewriter.ts index e8f241b37..2ceff8775 100644 --- a/packages/content-ops/src/ops/skill-reference-rewriter.ts +++ b/packages/content-ops/src/ops/skill-reference-rewriter.ts @@ -22,7 +22,7 @@ import { logger } from '../observability' import { FileSystemService } from '../file-system' -import { transformPath } from './naming-transforms' +import { transformPath, isRegistryEntryPath, type TransformOpts } from './naming-transforms' /** Maps original (short) skill name → new (prefixed) skill name */ export type SkillNameMap = Map @@ -175,13 +175,23 @@ export function findSkillReferences(content: string, names: Iterable): s /** * Builds a skill name map from the directory mapping collected during copy. * For each transformed directory: leafName (original) → transformedName (new). + * + * Directories BELOW the registry's entry granularity are skipped: with a bounded + * flatten (`flattenDepth`, #407) a skill's `references/` sub-dir is a real + * sub-path and therefore appears in the mapping, but it is content, not a skill + * — see `isRegistryEntryPath`. + * + * Typed as the full `TransformOpts` on purpose: it is forwarded verbatim to + * `transformPath`, which reads `flattenDepth` too, so a narrower declared type + * would invite a refactor to rebuild the object and silently drop the depth. */ export function buildSkillNameMap( dirMappingFiles: Map, - transformOpts: { flatten?: boolean; prefix?: string }, + transformOpts: TransformOpts, ): SkillNameMap { const map: SkillNameMap = new Map() for (const originalSubDir of dirMappingFiles.keys()) { + if (!isRegistryEntryPath(originalSubDir, transformOpts.flattenDepth)) continue const leafName = originalSubDir.split('/').pop()! const transformedName = transformPath(originalSubDir, transformOpts) if (leafName !== transformedName) { @@ -234,14 +244,18 @@ export async function rewriteSkillReferencesInFiles(params: RewriteSkillRefsPara * Contract: converts only a skill's `SKILL.md` entrypoint link. Deep links to * other files inside a skill dir (e.g. `references/*.md`) are NOT converted — * none exist in the KB today; add per-file mappings here if that changes. + * For the same reason a directory below the entry granularity is skipped (#407): + * a `references/` sub-dir holds no `SKILL.md` of its own. `TransformOpts` (not a + * narrower shape) for the reason given on `buildSkillNameMap`. */ export function buildSkillLinkPathMap( dirMappingFiles: Map, - transformOpts: { flatten?: boolean; prefix?: string }, + transformOpts: TransformOpts, ): SkillLinkPathMap { const map: SkillLinkPathMap = new Map() for (const originalSubDir of dirMappingFiles.keys()) { if (!originalSubDir.includes('/')) continue + if (!isRegistryEntryPath(originalSubDir, transformOpts.flattenDepth)) continue const transformed = transformPath(originalSubDir, transformOpts) map.set(`../.skills/${originalSubDir}/SKILL.md`, `../.claude/skills/${transformed}/SKILL.md`) } From 10674e68576cbe04cc1c1ec2f7afbf0b4f5e6837 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 15:38:29 +0200 Subject: [PATCH 05/12] [US-407] fix: wire flattenDepth from config to the copy pipeline (review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `/`). - 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 --- apps/pair-cli/config.json | 1 + .../src/commands/update/handler.test.ts | 82 +++++++++++++++++++ apps/pair-cli/src/registry/operations.test.ts | 17 ++++ apps/pair-cli/src/registry/operations.ts | 5 ++ apps/pair-cli/src/registry/resolver.test.ts | 20 +++++ apps/pair-cli/src/registry/resolver.ts | 15 ++++ apps/pair-cli/src/registry/validation.test.ts | 45 ++++++++++ apps/pair-cli/src/registry/validation.ts | 19 +++++ .../src/tools/skill-md-mirror.test.ts | 14 +++- .../src/tools/skill-md-mirror.ts | 16 +++- .../src/tools/skills-guide-mirror.ts | 14 ++-- 11 files changed, 237 insertions(+), 11 deletions(-) diff --git a/apps/pair-cli/config.json b/apps/pair-cli/config.json index f9d6a0727..96e3591ca 100644 --- a/apps/pair-cli/config.json +++ b/apps/pair-cli/config.json @@ -32,6 +32,7 @@ "source": ".skills", "behavior": "overwrite", "flatten": true, + "flattenDepth": 2, "prefix": "pair", "description": "Agent skills distributed to AI tool directories", "targets": [ diff --git a/apps/pair-cli/src/commands/update/handler.test.ts b/apps/pair-cli/src/commands/update/handler.test.ts index e0f6a4676..d732823fd 100644 --- a/apps/pair-cli/src/commands/update/handler.test.ts +++ b/apps/pair-cli/src/commands/update/handler.test.ts @@ -1620,3 +1620,85 @@ describe('update — local source path styles (#199 reorg)', () => { expect(await fs.readFile(`${cwd}/.pair/test-registry/file1.md`)).toBe('# New Content') }) }) + +/** + * #407 — the wiring, at the layer the story names: `pair update` reads + * `flattenDepth` from the registry config and the copy pipeline honours it. + * Without the resolver/validation/buildCopyOptions chain the option is dropped + * silently and this installs the sibling `pair-process-review-references/`. + */ +describe('#407: a skill nested references/ dir installs inside the skill via pair update', () => { + const moduleDir = '/project' + const datasetSrc = `${moduleDir}/packages/knowledge-hub/dataset` + + const configWith = (flattenDepth?: number) => ({ + asset_registries: { + skills: { + source: '.skills', + behavior: 'mirror', + flatten: true, + ...(flattenDepth !== undefined && { flattenDepth }), + prefix: 'pair', + description: 'Agent skills', + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + }, + }) + + const seed = (flattenDepth?: number) => + new InMemoryFileSystemService( + { + [`${moduleDir}/package.json`]: JSON.stringify({ name: 'test', version: '0.1.0' }), + [`${moduleDir}/packages/knowledge-hub/package.json`]: JSON.stringify({ + name: '@pair/knowledge-hub', + }), + [`${moduleDir}/config.json`]: JSON.stringify(configWith(flattenDepth)), + [`${datasetSrc}/.skills/process/review/SKILL.md`]: + '---\nname: review\n---\n# /review\nDetail in [deep dive](./references/deep.md).', + [`${datasetSrc}/.skills/process/review/references/deep.md`]: + '# Deep dive\nBack to [SKILL](../SKILL.md).', + // Pre-existing target — `update` requires the project to be installed. + [`${moduleDir}/.claude/skills/pair-process-review/SKILL.md`]: '# stale', + }, + moduleDir, + moduleDir, + ) + + const updateConfig: UpdateCommandConfig = { + command: 'update', + resolution: 'default', + kb: true, + offline: false, + } + + test('installs it inside the skill, with both links intact', async () => { + const fs = seed(2) + await handleUpdateCommand(updateConfig, fs, { httpClient: new MockHttpClientService() }) + + const installed = `${moduleDir}/.claude/skills/pair-process-review` + expect(await fs.exists(`${installed}/references/deep.md`)).toBe(true) + expect( + await fs.exists(`${moduleDir}/.claude/skills/pair-process-review-references/deep.md`), + ).toBe(false) + expect(await fs.readFile(`${installed}/SKILL.md`)).toContain( + '[deep dive](./references/deep.md)', + ) + expect(await fs.readFile(`${installed}/references/deep.md`)).toContain('[SKILL](../SKILL.md)') + }) + + test('without flattenDepth the same dataset still installs the pre-#407 sibling layout', async () => { + const fs = seed() + await handleUpdateCommand(updateConfig, fs, { httpClient: new MockHttpClientService() }) + + expect( + await fs.exists(`${moduleDir}/.claude/skills/pair-process-review-references/deep.md`), + ).toBe(true) + }) + + test('rejects a config typo instead of silently flattening everything', async () => { + const fs = seed(0) + await expect( + handleUpdateCommand(updateConfig, fs, { httpClient: new MockHttpClientService() }), + ).rejects.toThrow(/flattenDepth must be a positive integer/) + }) +}) diff --git a/apps/pair-cli/src/registry/operations.test.ts b/apps/pair-cli/src/registry/operations.test.ts index e5d09ae18..f628de4bf 100644 --- a/apps/pair-cli/src/registry/operations.test.ts +++ b/apps/pair-cli/src/registry/operations.test.ts @@ -101,6 +101,23 @@ describe('buildCopyOptions', () => { const options = buildCopyOptions(config) expect(options.flatten).toBe(true) expect(options.prefix).toBe('pair') + expect(options.flattenDepth).toBeUndefined() + }) + + // #407: dropped here, the copy pipeline always ran an unbounded flatten and a + // skill's nested `references/` installed as a sibling pseudo-skill. + it('forwards flattenDepth to the copy pipeline when the registry declares it', () => { + const config: RegistryConfig = { + source: '.skills', + behavior: 'mirror', + description: 'Skills', + include: [], + flatten: true, + flattenDepth: 2, + prefix: 'pair', + targets: [{ path: '.skills', mode: 'canonical' }], + } + expect(buildCopyOptions(config).flattenDepth).toBe(2) }) it('includes targets when set', () => { diff --git a/apps/pair-cli/src/registry/operations.ts b/apps/pair-cli/src/registry/operations.ts index 9f73650a1..4e0fc9263 100644 --- a/apps/pair-cli/src/registry/operations.ts +++ b/apps/pair-cli/src/registry/operations.ts @@ -158,6 +158,11 @@ export function buildCopyOptions(registryConfig: RegistryConfig): SyncOptions { include, flatten: registryConfig.flatten, targets: registryConfig.targets, + // #407: without this the copy pipeline always ran unbounded, so a skill's + // nested `references/` installed as a sibling pseudo-skill. + ...(registryConfig.flattenDepth !== undefined && { + flattenDepth: registryConfig.flattenDepth, + }), ...(registryConfig.prefix && { prefix: registryConfig.prefix }), } diff --git a/apps/pair-cli/src/registry/resolver.test.ts b/apps/pair-cli/src/registry/resolver.test.ts index e4a1292bd..a1d7ef298 100644 --- a/apps/pair-cli/src/registry/resolver.test.ts +++ b/apps/pair-cli/src/registry/resolver.test.ts @@ -13,6 +13,26 @@ describe('registry resolver', () => { expect(extractRegistries(config2)).toHaveProperty('b') }) + // #407: normalizeRegistryConfig used to ignore the key entirely, so + // `"flattenDepth": 2` in config.json was silently dropped and the copy pipeline + // always ran an unbounded flatten. + it('extractRegistries reads flattenDepth, and omits it when absent', () => { + const config = { + asset_registries: { + skills: { + source: '.skills', + flatten: true, + flattenDepth: 2, + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + knowledge: { source: '.pair', targets: [{ path: '.pair', mode: 'canonical' }] }, + }, + } + const registries = extractRegistries(config) + expect(registries['skills']?.flattenDepth).toBe(2) + expect(registries['knowledge']?.flattenDepth).toBeUndefined() + }) + it('resolveTarget correctly resolves relative and absolute targets', () => { const fs = new InMemoryFileSystemService({}, cwd, cwd) const config = { diff --git a/apps/pair-cli/src/registry/resolver.ts b/apps/pair-cli/src/registry/resolver.ts index bcc667472..11ea7be51 100644 --- a/apps/pair-cli/src/registry/resolver.ts +++ b/apps/pair-cli/src/registry/resolver.ts @@ -11,6 +11,17 @@ export interface RegistryConfig { description: string include: string[] flatten: boolean + /** + * Bounds `flatten` to this registry's ENTRY granularity (#407): only the first + * `flattenDepth` source segments are joined into the installed directory name, + * anything deeper stays a real sub-path. The `skills` registry's entries are + * two segments (`process/review`), so a third segment (`references/`) is + * content *of* that skill and must install inside it. + * + * Omitted ⇒ every separator is flattened, as before. Validated as a positive + * integer by `validateFlattenDepthField`. + */ + flattenDepth?: number prefix?: string targets: TargetConfig[] } @@ -66,6 +77,10 @@ function normalizeRegistryConfig(name: string, raw: Record): Re description: String(raw['description'] || ''), include: Array.isArray(raw['include']) ? (raw['include'] as string[]) : [], flatten: typeof raw['flatten'] === 'boolean' ? raw['flatten'] : false, + // Carried through as-is (no coercion): validateFlattenDepthField rejects a + // non-positive-integer loudly rather than letting a config typo degrade into + // an unbounded flatten, which is exactly the #407 defect. + ...(raw['flattenDepth'] != null && { flattenDepth: raw['flattenDepth'] as number }), ...(raw['prefix'] != null && { prefix: String(raw['prefix']) }), targets, } diff --git a/apps/pair-cli/src/registry/validation.test.ts b/apps/pair-cli/src/registry/validation.test.ts index 32003d934..c27583847 100644 --- a/apps/pair-cli/src/registry/validation.test.ts +++ b/apps/pair-cli/src/registry/validation.test.ts @@ -338,6 +338,51 @@ describe('registry validation - targets', () => { expect(errors).toHaveLength(0) }) + // #407: flattenDepth bounds flatten to the registry's entry granularity. It is + // read from JSON, so an invalid value must be REJECTED — degrading silently + // would degrade back into the unbounded flatten this option exists to fix. + it('accepts a positive-integer flattenDepth alongside flatten: true', () => { + const config: RegistryConfig = { + source: '.skills', + behavior: 'mirror', + description: 'Skills', + include: [], + flatten: true, + flattenDepth: 2, + prefix: 'pair', + targets: [{ path: '.skills', mode: 'canonical' }], + } + expect(validateRegistry('skills', config)).toHaveLength(0) + }) + + it.each([0, -1, 1.5, '2', null])('rejects the invalid flattenDepth %p', flattenDepth => { + const config = { + source: '.skills', + behavior: 'mirror', + description: 'Skills', + include: [], + flatten: true, + flattenDepth, + targets: [{ path: '.skills', mode: 'canonical' }], + } + const errors = validateRegistry('skills', config) + expect(errors.some(e => e.includes('flattenDepth must be a positive integer'))).toBe(true) + }) + + it('rejects flattenDepth without flatten: true (it would do nothing)', () => { + const config = { + source: '.skills', + behavior: 'mirror', + description: 'Skills', + include: [], + flatten: false, + flattenDepth: 2, + targets: [{ path: '.skills', mode: 'canonical' }], + } + const errors = validateRegistry('skills', config) + expect(errors.some(e => e.includes('flattenDepth requires flatten: true'))).toBe(true) + }) + it('rejects non-boolean flatten', () => { const config = { behavior: 'mirror', diff --git a/apps/pair-cli/src/registry/validation.ts b/apps/pair-cli/src/registry/validation.ts index f642adef1..a6c413564 100644 --- a/apps/pair-cli/src/registry/validation.ts +++ b/apps/pair-cli/src/registry/validation.ts @@ -81,6 +81,7 @@ export function validateRegistry(name: string, config: unknown): string[] { errors.push(...validateDescription(name, reg)) errors.push(...validateIncludes(name, reg)) errors.push(...validateFlattenField(name, reg)) + errors.push(...validateFlattenDepthField(name, reg)) errors.push(...validatePrefixField(name, reg)) errors.push(...validateTargetConfigs(name, reg)) @@ -138,6 +139,24 @@ function validateFlattenField(name: string, reg: Record): strin return [] } +/** + * `flattenDepth` bounds `flatten` to the registry's entry granularity (#407). + * Rejected loudly rather than coerced: a typo (`0`, `-1`, `"2"`, `1.5`) would + * otherwise degrade into an unbounded flatten — reintroducing the very defect + * the option exists to fix — and it is meaningless without `flatten: true`. + */ +function validateFlattenDepthField(name: string, reg: Record): string[] { + const flattenDepth = reg['flattenDepth'] + if (flattenDepth === undefined) return [] + if (typeof flattenDepth !== 'number' || !Number.isInteger(flattenDepth) || flattenDepth < 1) { + return [`Registry '${name}' flattenDepth must be a positive integer`] + } + if (reg['flatten'] !== true) { + return [`Registry '${name}' flattenDepth requires flatten: true`] + } + return [] +} + function validatePrefixField(name: string, reg: Record): string[] { const prefix = reg['prefix'] if (prefix === undefined) return [] diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts index 4a45dd947..8901c01c7 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts @@ -218,17 +218,27 @@ describe('root-copy read distinguishes a missing copy from an unreadable one', ( * no mirror-delete" — would become quietly false. */ describe('SKILL_COPY_OPTS stays pinned to the pair-cli skills registry', () => { - it('flatten/prefix/source/behavior match apps/pair-cli/config.json asset_registries.skills', () => { + it('flatten/flattenDepth/prefix/source/behavior match apps/pair-cli/config.json asset_registries.skills', () => { const config = JSON.parse( readFileSync(join(REPO_ROOT, 'apps/pair-cli/config.json'), 'utf-8'), ) as { asset_registries: { - skills: { source: string; flatten: boolean; prefix: string; behavior: string } + skills: { + source: string + flatten: boolean + flattenDepth?: number + prefix: string + behavior: string + } } } const registry = config.asset_registries.skills expect(SKILL_COPY_OPTS.flatten).toBe(registry.flatten) expect(SKILL_COPY_OPTS.prefix).toBe(registry.prefix) + // #407: the entry granularity is part of the transform, so it is pinned too — + // otherwise the guard would derive sibling `-references` paths the real + // `pair update` no longer produces. + expect(SKILL_COPY_OPTS.flattenDepth).toBe(registry.flattenDepth) // the guard reads the dataset from the registry's declared source dir (.skills) expect(DATASET_SKILLS.endsWith(registry.source)).toBe(true) // ...and runs the pipeline with the registry's declared copy behavior diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts index 39f143473..11c61e70f 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.ts @@ -6,9 +6,17 @@ * canonical dataset source under `packages/knowledge-hub/dataset/.skills/` * by `pair update`. Rather than re-implement that transform, these helpers run * the REAL copy pipeline — `copyDirectoryWithTransforms` with the exact - * `{ flatten: true, prefix: 'pair' }` options `apps/pair-cli/config.json` - * declares for the `skills` registry — over an in-memory clone of the dataset, - * so a bug in the real pipeline FAILS the guard instead of being masked. + * `{ flatten: true, flattenDepth: 2, prefix: 'pair' }` options + * `apps/pair-cli/config.json` declares for the `skills` registry — over an + * in-memory clone of the dataset, so a bug in the real pipeline FAILS the guard + * instead of being masked. + * + * `flattenDepth: 2` is the registry's ENTRY granularity (#407): a dataset skill + * dir is `/` (or the bare `next`), so a THIRD segment + * (`references/`) is content of that skill and installs inside it rather than as + * a sibling `pair---references/`. The mapping asserted here is + * unchanged for every current skill — no dataset skill dir is deeper than two + * segments — but the derivation now matches the corrected pipeline. * * The composed transform covers all four drift classes the guard must catch: * dir-rename (`transformPath`), frontmatter `name:` sync (`syncFrontmatter`), @@ -56,7 +64,7 @@ import { } from '@pair/content-ops' /** The exact naming-transform options the `skills` registry uses in config.json. */ -export const SKILL_COPY_OPTS = { flatten: true, prefix: 'pair' } as const +export const SKILL_COPY_OPTS = { flatten: true, flattenDepth: 2, prefix: 'pair' } as const /** * The FULL `SyncOptions` the guard runs the pipeline with: content-ops' defaults diff --git a/packages/knowledge-hub/src/tools/skills-guide-mirror.ts b/packages/knowledge-hub/src/tools/skills-guide-mirror.ts index 82fda5038..090662515 100644 --- a/packages/knowledge-hub/src/tools/skills-guide-mirror.ts +++ b/packages/knowledge-hub/src/tools/skills-guide-mirror.ts @@ -29,6 +29,7 @@ import { type SkillNameMap, type SkillLinkPathMap, } from '@pair/content-ops' +import { SKILL_COPY_OPTS } from './skill-md-mirror' /** * Recursively finds directories under `root` that directly contain at least @@ -65,13 +66,16 @@ export function collectSkillDirs(root: string): string[] { /** * Builds the real `skillNameMap` (short name -> installed prefixed name) * from the dataset's `.skills/` tree, using the same canonical - * `buildSkillNameMap` + `{ flatten: true, prefix: 'pair' }` options the real - * transfer pipeline uses (see `copy-directory-transforms.ts`'s - * `applySkillReferenceRewrites`). + * `buildSkillNameMap` + the single `SKILL_COPY_OPTS` pinned to the `skills` + * registry (see `copy-directory-transforms.ts`'s `applySkillReferenceRewrites`). + * Reusing that constant — rather than re-typing the knobs — is what keeps the + * entry granularity (`flattenDepth`, #407) in step: `collectSkillDirs` returns + * ANY dir holding a file, so a skill's `references/` sub-dir reaches this map and + * must be recognized as content, not registered as a skill named `references`. */ export function buildDatasetSkillNameMap(skillsDir: string): SkillNameMap { const dirMappingFiles = new Map(collectSkillDirs(skillsDir).map(d => [d, []])) - return buildSkillNameMap(dirMappingFiles, { flatten: true, prefix: 'pair' }) + return buildSkillNameMap(dirMappingFiles, SKILL_COPY_OPTS) } /** @@ -83,7 +87,7 @@ export function buildDatasetSkillNameMap(skillsDir: string): SkillNameMap { */ export function buildSkillLinkPathMap(skillsDir: string): SkillLinkPathMap { const dirMappingFiles = new Map(collectSkillDirs(skillsDir).map(d => [d, []])) - return prodBuildSkillLinkPathMap(dirMappingFiles, { flatten: true, prefix: 'pair' }) + return prodBuildSkillLinkPathMap(dirMappingFiles, SKILL_COPY_OPTS) } /** From 0257d3a0c6b80dc113953a52e4b3f6c4bec220b4 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 15:38:48 +0200 Subject: [PATCH 06/12] [US-407] docs: ADR-020 for the bounded-flatten decision + authoring convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tech/adr/adr-005-skills-infrastructure.md | 8 +- ...bounded-flatten-depth-entry-granularity.md | 81 +++++++++++++++++++ .../skill-conventions/README.md | 1 + .../skill-conventions/nested-sub-documents.md | 37 +++++++++ .../skill-conventions/README.md | 1 + .../skill-conventions/nested-sub-documents.md | 37 +++++++++ 6 files changed, 161 insertions(+), 4 deletions(-) create mode 100644 .pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md create mode 100644 .pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md create mode 100644 packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md diff --git a/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md b/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md index 3ded67edb..5009200b8 100644 --- a/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md +++ b/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md @@ -1,6 +1,6 @@ # ADR-005: Skills Infrastructure for Agent-Assisted Development -**Status:** Accepted +**Status:** Accepted — **amended by [ADR-020](adr-020-bounded-flatten-depth-entry-granularity.md)** (flatten is bounded to the registry's entry granularity for the `skills` registry; see Key Design Choice #2 and the Flatten/prefix rationale below) **Date:** 2026-02-10 **Context:** Epic #97 - Agent Skills in KB, Story #98 - Skill Infrastructure and Navigator @@ -11,16 +11,16 @@ Introduce a `.skills/` directory within the KB dataset as the source of truth fo ### Key Design Choices 1. **Directory structure**: `.skills/{category}/{skill-name}/SKILL.md` — skills organized by category (navigator, process, capability). -2. **Naming transforms**: Flatten (`navigator/next` → `navigator-next`) + prefix (`pair-navigator-next`) to produce unique, tool-compatible directory names. +2. **Naming transforms**: Flatten (`navigator/next` → `navigator-next`) + prefix (`pair-navigator-next`) to produce unique, tool-compatible directory names. **Amended by ADR-020**: flatten joins only the registry's ENTRY segments (`flattenDepth: 2` for `skills`); a deeper segment is content *of* the skill and installs inside it (`process/review/references` → `pair-process-review/references`, not the sibling `pair-process-review-references`). 3. **Multi-target distribution**: One canonical target (`.claude/skills/`) receives the physical copy; 5 secondary targets (`.github/skills/`, `.cursor/skills/`, `.agent/skills/`, `.agents/skills/`, `.windsurf/skills/`) receive symlinks pointing to canonical. 4. **Link rewriting**: Relative markdown links inside skill files are rewritten after flatten/prefix copy to maintain correctness. -5. **Configuration**: Skills registry defined in `config.json` with `flatten: true`, `prefix: "pair"`, `behavior: "mirror"`, and explicit `targets[]` array. +5. **Configuration**: Skills registry defined in `config.json` with `flatten: true`, `flattenDepth: 2` (ADR-020), `prefix: "pair"`, `behavior: "mirror"`, and explicit `targets[]` array. ## Rationale - **Open standard**: Agent Skills (agentskills.io) is supported by Claude Code, Cursor, VS Code Copilot, OpenAI Codex, and Windsurf. Adopting this standard maximizes tool compatibility. - **Symlinks over copies**: Avoids content duplication across 6 targets, reducing disk usage and ensuring consistency. Windows environments fall back to copy mode (validated at config time). -- **Flatten/prefix**: AI tools expect skills in flat directory structures. Prefixing with `pair-` prevents naming collisions with skills from other sources. +- **Flatten/prefix**: AI tools expect skills in flat directory structures. Prefixing with `pair-` prevents naming collisions with skills from other sources. **Amended by ADR-020**: "flat" means the skill *directory names* are flat — a skill's own sub-directories (e.g. `references/`) stay nested inside it, which is what the Agent Skills progressive-disclosure layout requires. - **KB-native**: Skills live in the KB dataset alongside existing `.pair/` content, leveraging the existing distribution pipeline (`pair install`, `pair update`). ## Consequences diff --git a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md new file mode 100644 index 000000000..522c298a8 --- /dev/null +++ b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md @@ -0,0 +1,81 @@ +# ADR-020: Bounded flatten depth — a registry's entry granularity, opt-in per registry + +## Status + +Accepted — **amends [ADR-005](adr-005-skills-infrastructure.md)** §Key Design Choices #2 (Naming transforms) and its "Flatten/prefix" rationale bullet. ADR-005 otherwise stands; only the flatten *semantics for the `skills` registry* change here. + +> Numbering note: `adr-018` is claimed by two open branches (#234, #236); `019` is left free for whichever of them renumbers. + +## Date + +2026-07-30 + +## Context + +- ADR-005 introduced `flatten` as "replace every directory separator with a hyphen", because "AI tools expect skills in flat directory structures". At the time every source path was exactly `/`, so *every separator* and *the entry boundary* were the same thing. #238 later generalized flatten/prefix across asset registries and likewise never considered a path deeper than the entry. +- Story #407 is the case where the two stop coinciding. A skill using the standard Agent-Skills progressive-disclosure layout ships `process/review/references/deep.md`. Unbounded flatten installs that as `pair-process-review-references/deep.md` — a **sibling pseudo-skill directory**, outside the skill it belongs to, with the skill's own `./references/deep.md` pointing at nothing and the sub-doc's `../SKILL.md` re-rooted back into the dataset layout. The layout is not hypothetical: the third-party `agent-browser` skill already ships `references/*.md`, and #313's direction is progressive disclosure for the pair corpus. +- The transform is **shared by every asset registry** (`knowledge`, `adoption`, `github`, `agents`, `skills`). A registry whose entries are single-segment is served correctly by full flattening, so redefining flatten globally would silently change behaviour for registries that have no defect. +- Constraint from CLAUDE.md / `/pair-process-review` Step 2.3: a technical decision must be recorded, not left in a PR body. Story #407 explicitly flags this question as ADR-worthy. + +## Options Considered + +### Option 1: Redefine `flatten` registry-wide as depth-1 + +- **Description**: flatten only the first separator everywhere; every registry preserves deeper structure. +- **Pros**: no new option; one semantic for the whole pipeline. +- **Cons**: wrong answer for the `skills` registry, whose entry is TWO segments (`process/review`) — depth-1 would install `pair-process/review/`, not `pair-process-review/`. Also a silent behaviour change for every other registry, with no defect to justify it. + +### Option 2: `preserveNested: true` (boolean — "flatten the entry, keep the rest") + +- **Description**: a boolean meaning "stop flattening below the entry", with the entry inferred. +- **Pros**: no layout number in the caller; reads declaratively. +- **Cons**: the boolean is not self-contained — it still has to *infer* where the entry ends, so it is Option 3 in disguise plus a flag. And "the entry" is exactly what a generic path transform cannot know. + +### Option 3: "the directory containing `SKILL.md` is the entry" + +- **Description**: detect the entry structurally, from the presence of a marker file. +- **Pros**: semantically the sharpest — it is literally what "a skill" means, and it survives a category level being added or removed. +- **Cons**: leaks **skills-domain knowledge into a generic content transform** shared by four non-skill registries (`naming-transforms.ts` would have to know what `SKILL.md` is). It also makes the installed path depend on file *content discovery* rather than on declared configuration, so the same source tree can install differently depending on which files happen to be present — harder to validate ahead of a copy, and it would have to be re-derived in the mirror guards that reproduce the transform. + +### Option 4 (chosen): `flattenDepth: `, opt-in per registry + +- **Description**: an explicit numeric depth on the registry config. The first `flattenDepth` segments are joined with hyphens; anything deeper is preserved as a real sub-path. Absent ⇒ unchanged, unbounded flatten. +- **Pros**: the generic transform stays generic (pure path math, no marker-file knowledge); the layout fact lives in the config that already declares that layout (`source`, `prefix`, `targets`); opt-in means no other registry changes behaviour; trivially validatable before any file is copied. +- **Cons**: it encodes a **registry-layout fact as a number in the caller** — if a category level is ever added to or removed from `.skills/`, `flattenDepth` must be updated in step or the transform mis-flattens. Mitigated by (a) validating it as a positive integer at the CLI boundary and throwing in `flattenPath` on an invalid value, so a typo fails loudly instead of degrading back into the defect, and (b) pinning the value in `SKILL_COPY_OPTS` against `config.json` with a test, so guard and pipeline cannot drift apart. + +## Decision + +**Adopt `flattenDepth` (Option 4): a positive-integer depth, declared per registry, bounding `flatten` to that registry's entry granularity. `skills` declares `flattenDepth: 2`. Every other registry omits it and keeps the pre-existing unbounded behaviour.** + +Option 3 is the semantically better model and was rejected on **coupling**, not on correctness: the flatten transform serves four registries that have no notion of `SKILL.md`, and paying for entry detection there means every consumer of the transform (including the two mirror guards that reproduce it) inherits skills-domain knowledge. A declared number keeps the knowledge where the layout is already declared. Option 1 was rejected as wrong for the very registry that has the defect; Option 2 collapses into Option 3. + +The **opt-in default** (absent ⇒ unbounded) is deliberate: it makes this a scoped fix rather than a pipeline-wide semantic change, and it is pinned by a regression-witness test asserting the unbounded result so the old behaviour cannot drift unnoticed. + +Corollary, recorded because it is not obvious: once flatten is bounded, a sub-directory below the entry appears in the copy's directory mapping alongside real entries. It is **content, not an entry** — so the skill-name map, the skill link-path map, and the frontmatter `name:` sync all skip it (`isRegistryEntryPath`). Without that, `references` gets registered as a *skill name* mapped to one arbitrary skill's sub-directory, and the `/references` token in an unrelated skill's body is rewritten to it. + +## Consequences + +### Benefits + +- A skill may ship a nested `references/` (or any sub-dir) and it installs **inside** the skill, with both link directions intact — the standard progressive-disclosure layout becomes usable in the pair corpus. +- Non-skill registries are byte-for-byte unaffected; the change is provably scoped by the absence of the option. +- An invalid depth fails loudly at config validation (`pair update`/`install`/`package`) and in `flattenPath` itself, instead of silently reverting to the defect. +- A bounded flatten cannot produce the traversal-unsafe tail an unbounded one made impossible: `.`/`..` in the preserved tail is rejected rather than joined onto the destination root. + +### Trade-offs and Limitations + +- **The number is a duplicated fact.** `.skills/` layout depth is asserted in `apps/pair-cli/config.json` and mirrored in `SKILL_COPY_OPTS`. A pin test ties the two together, but a `.skills/` reorganization must still update the number deliberately. +- **Two flatten semantics coexist** in one codebase (bounded for `skills`, unbounded elsewhere). Accepted as the price of not changing registries that have no defect; the regression-witness test documents the unbounded branch as intentional, not vestigial. +- Collision detection (`detectCollisions`) now compares sub-paths, not only top-level names. No behaviour change for the current corpus (no dataset skill dir is deeper than two segments) but a future nested-name clash surfaces as a sub-path collision message. + +## Adoption Impact + +- **`.pair/adoption/tech/adr/adr-005-skills-infrastructure.md`** — amended: Key Design Choice #2 and the "Flatten/prefix" rationale bullet now point here for the bounded form; `config.json`'s registry entry gains `flattenDepth: 2`. +- **`.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md`** (new, plus its index row in that directory's `README.md`, and the `packages/knowledge-hub/dataset/` source of both) — the authoring convention this enables: a skill may ship a nested `references/` sub-dir, it installs inside the skill, and relative links work in both directions. Written generic/portable per that directory's Scope note, so it does not cite this ADR back. +- No change to `architecture.md`, `tech-stack.md`, or `infrastructure.md` — this is a transform semantic, not a stack or boundary decision. + +## References + +- Story: #407 · PR: #411 +- Amends: ADR-005 (#98, PR #106) · introduced-by context: #238 (flatten/prefix across registries) +- Implementation: `packages/content-ops/src/ops/naming-transforms.ts` (`flattenPath`, `isRegistryEntryPath`), `apps/pair-cli/src/registry/{resolver,validation,operations}.ts`, `apps/pair-cli/config.json`, `packages/knowledge-hub/src/tools/skill-md-mirror.ts` diff --git a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md index 167e3dd10..0a7606d41 100644 --- a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md +++ b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md @@ -13,6 +13,7 @@ Single-source explanations for patterns that recur across many `SKILL.md` files. | Output Format shapes (Decision / Report) | [output-shapes.md](output-shapes.md) | 10 skills (documentation-only — no logic change) | | To-issues triage (extend vs create) | [to-issues-triage.md](to-issues-triage.md) | 2 composing skills (new convention — no prior duplication to extract) | | Guided / Quick setup duality (guided asks; quick accepts defaults) | [guided-quick-setup.md](guided-quick-setup.md) | 2 precedents (`pair package` `--interactive`; the `assess-*` cascade) — documented, not retrofitted | +| Nested sub-documents / progressive disclosure (`references/` inside a skill dir) | [nested-sub-documents.md](nested-sub-documents.md) | 0 skills (authoring convention — describes the installed layout, not a skill step) | ## How a skill uses these files diff --git a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md new file mode 100644 index 000000000..115e558d6 --- /dev/null +++ b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -0,0 +1,37 @@ +# Nested Sub-Documents (Progressive Disclosure) + +A skill is a **directory**, not a single file. `SKILL.md` is its entrypoint; anything else the skill owns lives beside it in that directory — including a nested sub-directory. + +## The layout + +```text +// +├── SKILL.md # entrypoint — always loaded +├── merge-and-cascade.md # a sibling sub-document +└── references/ # a nested sub-directory + ├── deep-dive.md + └── catalog.md +``` + +`references/` is the Agent Skills convention for **progressive disclosure**: material the executor should read only when it reaches the step that needs it, kept out of the entrypoint so `SKILL.md` stays small. The name is a convention, not a requirement — any sub-directory works the same way. + +## How it installs + +The distribution pipeline flattens only the **entry** part of the source path and preserves everything below it, so a skill's directory keeps its shape: + +```text +//references/deep-dive.md + → .claude/skills/--/references/deep-dive.md +``` + +The sub-directory installs **inside** the skill, not as a sibling directory of its own. Two consequences for the author: + +- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. +- **A sub-document is content, not a skill.** It is not a separate installed entry, gets no name prefix, and its frontmatter (if any) is left alone. Do not give a sub-document a `name:` matching a skill name, and do not expect a sub-directory name to be invocable. + +## Authoring rules + +1. **Only the entry directory holds `SKILL.md`.** A `SKILL.md` inside a sub-directory would read as a second skill; put the entrypoint at the skill's root and nowhere else. +2. **Point at sub-documents with relative links** from `SKILL.md`, at the step that needs them, so the executor follows the pointer when it gets there — the same way it follows pointers to guideline files. +3. **Keep the entrypoint self-sufficient for the happy path.** A sub-document is for depth (long catalogs, worked examples, edge-case tables), not for material the skill needs on every run. +4. **One level is usually enough.** Deeper nesting is preserved too, but it costs the reader a hop for no gain in most cases. diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md index 881434f52..93b72c755 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md @@ -13,6 +13,7 @@ Single-source explanations for patterns that recur across many `SKILL.md` files. | Output Format shapes (Decision / Report) | [output-shapes.md](output-shapes.md) | 10 skills (documentation-only — no logic change) | | To-issues triage (extend vs create) | [to-issues-triage.md](to-issues-triage.md) | 2 composing skills (new convention — no prior duplication to extract) | | Guided / Quick setup duality (guided asks; quick accepts defaults) | [guided-quick-setup.md](guided-quick-setup.md) | 2 precedents (`pair package` `--interactive`; the `assess-*` cascade) — documented, not retrofitted | +| Nested sub-documents / progressive disclosure (`references/` inside a skill dir) | [nested-sub-documents.md](nested-sub-documents.md) | 0 skills (authoring convention — describes the installed layout, not a skill step) | ## How a skill uses these files diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md new file mode 100644 index 000000000..115e558d6 --- /dev/null +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -0,0 +1,37 @@ +# Nested Sub-Documents (Progressive Disclosure) + +A skill is a **directory**, not a single file. `SKILL.md` is its entrypoint; anything else the skill owns lives beside it in that directory — including a nested sub-directory. + +## The layout + +```text +// +├── SKILL.md # entrypoint — always loaded +├── merge-and-cascade.md # a sibling sub-document +└── references/ # a nested sub-directory + ├── deep-dive.md + └── catalog.md +``` + +`references/` is the Agent Skills convention for **progressive disclosure**: material the executor should read only when it reaches the step that needs it, kept out of the entrypoint so `SKILL.md` stays small. The name is a convention, not a requirement — any sub-directory works the same way. + +## How it installs + +The distribution pipeline flattens only the **entry** part of the source path and preserves everything below it, so a skill's directory keeps its shape: + +```text +//references/deep-dive.md + → .claude/skills/--/references/deep-dive.md +``` + +The sub-directory installs **inside** the skill, not as a sibling directory of its own. Two consequences for the author: + +- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. +- **A sub-document is content, not a skill.** It is not a separate installed entry, gets no name prefix, and its frontmatter (if any) is left alone. Do not give a sub-document a `name:` matching a skill name, and do not expect a sub-directory name to be invocable. + +## Authoring rules + +1. **Only the entry directory holds `SKILL.md`.** A `SKILL.md` inside a sub-directory would read as a second skill; put the entrypoint at the skill's root and nowhere else. +2. **Point at sub-documents with relative links** from `SKILL.md`, at the step that needs them, so the executor follows the pointer when it gets there — the same way it follows pointers to guideline files. +3. **Keep the entrypoint self-sufficient for the happy path.** A sub-document is for depth (long catalogs, worked examples, edge-case tables), not for material the skill needs on every run. +4. **One level is usually enough.** Deeper nesting is preserved too, but it costs the reader a hop for no gain in most cases. From 520b20c82b69f708b77e807000cd30e17c3aba88 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 16:05:42 +0200 Subject: [PATCH 07/12] =?UTF-8?q?[US-407]=20fix:=20a=20registry-root=20ski?= =?UTF-8?q?ll=20cannot=20ship=20a=20sub-dir=20=E2=80=94=20fail=20loudly=20?= =?UTF-8?q?(review=20round=202)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...bounded-flatten-depth-entry-granularity.md | 4 +- .../skill-conventions/nested-sub-documents.md | 13 +- apps/pair-cli/src/registry/validation.ts | 14 +- packages/content-ops/src/index.ts | 1 + .../copy/copy-directory-transforms.test.ts | 112 +++++++++++++- .../src/ops/copy/copy-directory-transforms.ts | 144 +++++++++++++++++- packages/content-ops/src/ops/link-rewriter.ts | 42 +++-- .../src/ops/naming-transforms.test.ts | 33 ++++ .../content-ops/src/ops/naming-transforms.ts | 52 +++++-- .../skill-conventions/nested-sub-documents.md | 13 +- 10 files changed, 390 insertions(+), 38 deletions(-) diff --git a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md index 522c298a8..3ed4383ed 100644 --- a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md +++ b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md @@ -41,7 +41,7 @@ Accepted — **amends [ADR-005](adr-005-skills-infrastructure.md)** §Key Design - **Description**: an explicit numeric depth on the registry config. The first `flattenDepth` segments are joined with hyphens; anything deeper is preserved as a real sub-path. Absent ⇒ unchanged, unbounded flatten. - **Pros**: the generic transform stays generic (pure path math, no marker-file knowledge); the layout fact lives in the config that already declares that layout (`source`, `prefix`, `targets`); opt-in means no other registry changes behaviour; trivially validatable before any file is copied. -- **Cons**: it encodes a **registry-layout fact as a number in the caller** — if a category level is ever added to or removed from `.skills/`, `flattenDepth` must be updated in step or the transform mis-flattens. Mitigated by (a) validating it as a positive integer at the CLI boundary and throwing in `flattenPath` on an invalid value, so a typo fails loudly instead of degrading back into the defect, and (b) pinning the value in `SKILL_COPY_OPTS` against `config.json` with a test, so guard and pipeline cannot drift apart. +- **Cons**: it encodes a **registry-layout fact as a number in the caller** — if a category level is ever added to or removed from `.skills/`, `flattenDepth` must be updated in step or the transform mis-flattens. It also assumes every entry sits at the SAME depth, and `.skills/` already breaks that (`next/` is one segment, the other 39 are two), so a sub-directory under a registry-root skill is excluded by a loud failure — see Trade-offs. Mitigated by (a) validating it as a positive integer at the CLI boundary and throwing in `flattenPath` on an invalid value, so a typo fails loudly instead of degrading back into the defect, and (b) pinning the value in `SKILL_COPY_OPTS` against `config.json` with a test, so guard and pipeline cannot drift apart. ## Decision @@ -67,6 +67,8 @@ Corollary, recorded because it is not obvious: once flatten is bounded, a sub-di - **The number is a duplicated fact.** `.skills/` layout depth is asserted in `apps/pair-cli/config.json` and mirrored in `SKILL_COPY_OPTS`. A pin test ties the two together, but a `.skills/` reorganization must still update the number deliberately. - **Two flatten semantics coexist** in one codebase (bounded for `skills`, unbounded elsewhere). Accepted as the price of not changing registries that have no defect; the regression-witness test documents the unbounded branch as intentional, not vestigial. - Collision detection (`detectCollisions`) now compares sub-paths, not only top-level names. No behaviour change for the current corpus (no dataset skill dir is deeper than two segments) but a future nested-name clash surfaces as a sub-path collision message. +- **A registry-root entry cannot ship a sub-directory while `flattenDepth` is 2.** The depth assumes every entry sits at the same depth, and `.skills/` does not: `next/` is a ONE-segment entry (installed as `pair-next`) beside 39 two-segment ones. `next/references` would therefore have the same shape as the entry `process/review` and install as the sibling `pair-next-references/` — the whole defect back, for that one skill. The copy **fails loudly** on that shape (a directory shallower than `flattenDepth` that holds files directly AND owns a sub-directory) rather than mis-installing; a category directory is told apart from an entry by whether it holds files directly, so no marker-file knowledge enters the transform. Consequence for authors: giving `next` a sub-directory requires moving it under a category first, which **renames the installed skill users invoke**. Stated in `nested-sub-documents.md` as the one exclusion to the convention. +- Mirror cleanup descends one level per preserved sub-path under a bounded flatten (it was top-level-only, which a sub-path move made insufficient — a `references/` removed from the source would have stayed installed). Unbounded mirrors keep the exact previous top-level-only behaviour, gated on the option's presence. ## Adoption Impact diff --git a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md index 115e558d6..a3887dfc1 100644 --- a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md +++ b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -26,9 +26,20 @@ The distribution pipeline flattens only the **entry** part of the source path an The sub-directory installs **inside** the skill, not as a sibling directory of its own. Two consequences for the author: -- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. +- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. (True for a skill at the standard `/` depth — see the exclusion below.) - **A sub-document is content, not a skill.** It is not a separate installed entry, gets no name prefix, and its frontmatter (if any) is left alone. Do not give a sub-document a `name:` matching a skill name, and do not expect a sub-directory name to be invocable. +## The one exclusion: a skill that is not at the standard depth + +The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`). A skill that sits **shallower** than that — directly at the registry root, with no category directory — cannot ship a sub-directory: its `/references/` has exactly the shape of a real `/` entry, so it would install as a **sibling** `--references/` rather than inside the skill. The pipeline **fails the copy loudly** in that case instead of installing something broken: + +```text +Ambiguous layout for a bounded flatten (flattenDepth=2): 'next' is 1 segment(s) deep, +holds files directly AND owns the sub-directory 'next/references'. … +``` + +To give such a skill a sub-directory, first move it under a category directory (that renames the installed skill, so it changes the name users invoke — decide deliberately). + ## Authoring rules 1. **Only the entry directory holds `SKILL.md`.** A `SKILL.md` inside a sub-directory would read as a second skill; put the entrypoint at the skill's root and nowhere else. diff --git a/apps/pair-cli/src/registry/validation.ts b/apps/pair-cli/src/registry/validation.ts index a6c413564..4c97316b5 100644 --- a/apps/pair-cli/src/registry/validation.ts +++ b/apps/pair-cli/src/registry/validation.ts @@ -1,4 +1,10 @@ -import { Behavior, FileSystemService, validateTargets, type TargetConfig } from '@pair/content-ops' +import { + Behavior, + FileSystemService, + isValidFlattenDepth, + validateTargets, + type TargetConfig, +} from '@pair/content-ops' import type { RegistryConfig } from './resolver' import { getCanonicalTarget } from './layout' import { DEFAULT_WORKING_PATH, validateWorkingPath } from './working-area' @@ -144,11 +150,15 @@ function validateFlattenField(name: string, reg: Record): strin * Rejected loudly rather than coerced: a typo (`0`, `-1`, `"2"`, `1.5`) would * otherwise degrade into an unbounded flatten — reintroducing the very defect * the option exists to fix — and it is meaningless without `flatten: true`. + * + * The predicate itself comes from `@pair/content-ops` (`isValidFlattenDepth`), + * the same one `flattenPath` asserts on, so the deliberate double boundary + * cannot drift into two different rules. Only the message is this layer's. */ function validateFlattenDepthField(name: string, reg: Record): string[] { const flattenDepth = reg['flattenDepth'] if (flattenDepth === undefined) return [] - if (typeof flattenDepth !== 'number' || !Number.isInteger(flattenDepth) || flattenDepth < 1) { + if (!isValidFlattenDepth(flattenDepth)) { return [`Registry '${name}' flattenDepth must be a positive integer`] } if (reg['flatten'] !== true) { diff --git a/packages/content-ops/src/index.ts b/packages/content-ops/src/index.ts index a37699d66..91f394450 100644 --- a/packages/content-ops/src/index.ts +++ b/packages/content-ops/src/index.ts @@ -61,6 +61,7 @@ export { transformPath, detectCollisions, isRegistryEntryPath, + isValidFlattenDepth, type TransformOpts, } from './ops/naming-transforms' export { diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts index 83f368f2f..14133bb94 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts @@ -428,9 +428,15 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => // The sibling case, which the same mechanism has to get right: when the // nested dir does NOT move with its parent (unbounded flatten), the link up // must be REWRITTEN to wherever the parent landed — not left alone. + // Both directions, because they are the same mechanism: the FORWARD link + // must be rewritten too. Its own directory moved AND the sub-directory it + // points into moved elsewhere, so the most specific move has to win — with + // the own-directory rebase asked first, the forward link was left as + // `./references/deep.md`, pointing at nothing (#407 review). it('rewrites the back-link when the nested dir becomes a sibling (unbounded)', async () => { const fileService = createTestFileService({ - '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + '/dataset/source/process/review/SKILL.md': + '---\nname: review\n---\n# /review\nDetail in [deep dive](./references/deep.md).', '/dataset/source/process/review/references/deep.md': '# Deep dive\nBack to [SKILL](../SKILL.md).', }) @@ -448,6 +454,59 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => ) expect(deep).toContain('[SKILL](../pair-process-review/SKILL.md)') expect(deep).not.toContain('source/process/review') + + const skill = await fileService.readFile('/dataset/target/pair-process-review/SKILL.md') + expect(skill).toContain('[deep dive](../pair-process-review-references/deep.md)') + expect(skill).not.toContain('](./references/deep.md)') + }) + + // #407 review (Major). `flattenDepth` states "an entry is N segments deep", + // and `.skills/next` is a ONE-segment entry while `process/review` is two. + // `next/references` therefore has the same shape as a real entry and would + // install as the sibling `pair-next-references/` — every defect this option + // removes, back for `next`. Unrepresentable, so it must fail loudly. + it('refuses a source entry shallower than flattenDepth that owns a sub-dir (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/next/SKILL.md': '---\nname: next\n---\n# /next', + '/dataset/source/next/references/deep.md': '# Deep dive\nBack to [SKILL](../SKILL.md).', + '/dataset/source/process/review/SKILL.md': + '---\nname: review\n---\n# /review\nProgressive disclosure lives under /references.', + }) + + await expect( + copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }), + ).rejects.toThrow(/Ambiguous layout for a bounded flatten/) + + // Fails BEFORE copying anything, so no half-installed sibling is left behind + // and no unrelated skill body was rewritten. + expect(await fileService.exists('/dataset/target/pair-next-references/deep.md')).toBe(false) + expect(await fileService.exists('/dataset/target/pair-process-review/SKILL.md')).toBe(false) + }) + + it('accepts a shallower entry that owns no sub-dir, alongside deeper entries (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/next/SKILL.md': '---\nname: next\n---\n# /next', + '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + }) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }) + + // The real `.skills/` shape today: a one-segment entry and two-segment + // entries side by side, both installing at the top level. + expect(await fileService.exists('/dataset/target/pair-next/SKILL.md')).toBe(true) + expect(await fileService.exists('/dataset/target/pair-process-review/SKILL.md')).toBe(true) }) // #407 review finding: with a bounded flatten, `process/review/references` @@ -610,6 +669,57 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => ) }) + // #407 review: under a bounded flatten a source sub-directory maps to a + // target SUB-PATH, so top-level-only cleanup could no longer see it — a + // `references/` deleted from the source stayed installed forever (and its + // docs kept being loaded). Cleanup descends into a transformed entry. + it('removes a nested sub-dir that is gone from the source (bounded + mirror, #407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + '/dataset/source/process/review/references/deep.md': '# Deep dive', + }) + + const runOnce = () => + copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { + flatten: true, + prefix: 'pair', + flattenDepth: 2, + defaultBehavior: 'mirror', + targets: [], + }, + }) + + await runOnce() + await expect( + fileService.exists('/dataset/target/pair-process-review/references/deep.md'), + ).resolves.toBe(true) + + // Idempotent while the source is unchanged... + await runOnce() + await expect( + fileService.exists('/dataset/target/pair-process-review/references/deep.md'), + ).resolves.toBe(true) + + // ...and the nested target goes away with its source. + await fileService.rm('/dataset/source/process/review/references', { + recursive: true, + force: true, + }) + await runOnce() + await expect( + fileService.exists('/dataset/target/pair-process-review/references/deep.md'), + ).resolves.toBe(false) + // The entry itself and its own files are untouched. + await expect( + fileService.exists('/dataset/target/pair-process-review/SKILL.md'), + ).resolves.toBe(true) + }) + it('does not clean up stale entries when behavior is not mirror', async () => { const fileService = createTestFileService({ '/dataset/source/catalog/next/SKILL.md': '---\nname: next\n---\n# /next', diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts index 2f8098ce4..77369e6d5 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts @@ -65,6 +65,84 @@ function validateNoCollisions( } } +/** + * Two facts about the source tree's shape, from the flat file list: which + * directories hold files DIRECTLY, and for each directory one example + * sub-directory (ancestors included, since a file list only names leaf dirs). + * Consumed by `validateNoShallowEntryWithSubdir`. + */ +function collectDirShapes(files: string[]): { + dirsWithOwnFiles: Set + firstChildDirOf: Map +} { + const dirsWithOwnFiles = new Set() + const firstChildDirOf = new Map() + for (const filePath of files) { + const dir = dirname(filePath) + // The source ROOT is never an entry: its files are copied straight to the + // destination root, untransformed. + if (dir === '.') continue + dirsWithOwnFiles.add(dir) + const segments = dir.split('/') + for (let i = 1; i < segments.length; i++) { + const parent = segments.slice(0, i).join('/') + if (!firstChildDirOf.has(parent)) { + firstChildDirOf.set(parent, segments.slice(0, i + 1).join('/')) + } + } + } + return { dirsWithOwnFiles, firstChildDirOf } +} + +/** + * Rejects the one source shape a bounded flatten cannot represent: an entry that + * sits SHALLOWER than `flattenDepth` and owns a sub-directory (#407 review). + * + * `flattenDepth` is a positional statement about the source layout — "an entry is + * N segments deep" (ADR-020). A registry whose entries are not all at that depth + * breaks it: in `.skills/` today `next/` is a ONE-segment entry while + * `process/review/` is two, so `next/references` — content of `next` — has the + * same shape as the entry `process/review`. It would install as the sibling + * `pair-next-references/`, reintroducing for `next` all four defects this option + * removes for a two-segment entry: misplacement outside the skill, a dead + * `./references/…` forward link, a bogus `references` skill-name mapping that + * leaks into unrelated files, and a path written as the sub-doc's `name:`. + * + * That shape is unrepresentable, not merely unhandled, so it fails loudly here — + * before any file is copied — rather than being guessed at. A category directory + * (only sub-directories) and an entry (files of its own) are told apart by + * whether the directory holds files DIRECTLY: no `SKILL.md` knowledge, per + * ADR-020's coupling argument. Files at the source ROOT are exempt: they are + * copied straight to the destination root and are never entries. + */ +function validateNoShallowEntryWithSubdir( + files: string[], + transformOpts: TransformOpts, + srcPath: string, +): void { + const { flattenDepth } = transformOpts + if (!transformOpts.flatten || flattenDepth === undefined || flattenDepth < 2) return + + const { dirsWithOwnFiles, firstChildDirOf } = collectDirShapes(files) + + for (const dir of dirsWithOwnFiles) { + const depth = dir.split('/').length + if (depth >= flattenDepth) continue + const child = firstChildDirOf.get(dir) + if (child === undefined) continue + throw createError({ + type: 'IO_ERROR', + message: + `Ambiguous layout for a bounded flatten (flattenDepth=${flattenDepth}): '${dir}' is ${depth} segment(s) deep, ` + + `holds files directly AND owns the sub-directory '${child}'. '${child}' is ${flattenDepth} segment(s) deep, so it cannot be told apart ` + + `from a real entry and would install as a sibling entry instead of inside '${dir}'. ` + + `Move '${dir}' ${flattenDepth - depth} level(s) deeper (e.g. under a category directory), or drop the sub-directory.`, + operation: 'copyDir', + path: join(srcPath, dir), + }) + } +} + /** * Copies a single file to its transformed location and tracks the * directory mapping for later link rewriting. @@ -241,6 +319,7 @@ export async function copyDirectoryWithTransforms(params: { const files = await collectFiles(fileService, srcPath, srcPath) validateNoCollisions(files, transformOpts, srcPath) + validateNoShallowEntryWithSubdir(files, transformOpts, srcPath) await fileService.mkdir(destPath, { recursive: true }) @@ -282,9 +361,15 @@ export async function copyDirectoryWithTransforms(params: { * skill's leftover flattened directory is cleaned up, and a prefix change no * longer leaves the old prefixed directory orphaned alongside the new one. * - * Only top-level entries are considered — matches the granularity of the - * non-transform `handleMirrorCleanup` and the flatten use case (one source - * subdirectory maps to exactly one top-level target directory). + * Under an UNBOUNDED flatten only top-level entries are considered — matching + * the granularity of the non-transform `handleMirrorCleanup`, and sufficient + * because one source subdirectory then maps to exactly one top-level target + * directory. Under a BOUNDED flatten (`flattenDepth`, #407) a source + * subdirectory maps to a target SUB-PATH instead, so cleanup descends into a + * transformed entry as well: without that, a `references/` removed from the + * source would stay installed forever and its progressive-disclosure docs would + * keep being loaded. Descent is gated on `flattenDepth` being present, so the + * unbounded path stays byte-for-byte as before. * * `topLevelFiles` (file names copied directly from the source root, with no * subdirectory of their own) must be included in `expected` alongside the @@ -302,17 +387,60 @@ async function cleanupStaleTransformedEntries(params: { const { fileService, destPath, dirMappingFiles, topLevelFiles, transformOpts } = params const expected = new Set(topLevelFiles) + // Every transformed sub-path a bounded flatten produces, plus each of its + // ancestors: a directory holding only deeper directories has no file of its + // own in the mapping, and must not read as stale. + const expectedNested = new Set() for (const originalSubDir of dirMappingFiles.keys()) { const transformedDir = transformPath(originalSubDir, transformOpts) - expected.add(transformedDir.split('/')[0]!) + const segments = transformedDir.split('/') + expected.add(segments[0]!) + for (let i = 2; i <= segments.length; i++) { + expectedNested.add(segments.slice(0, i).join('/')) + } } const entries = await fileService.readdir(destPath).catch(() => []) for (const entry of entries) { - if (expected.has(entry.name)) continue - const toRemove = join(destPath, entry.name) - await fileService.rm(toRemove, { recursive: true, force: true }) - logger.info(`Mirror: removed stale transformed entry ${toRemove}`) + if (!expected.has(entry.name)) { + const toRemove = join(destPath, entry.name) + await fileService.rm(toRemove, { recursive: true, force: true }) + logger.info(`Mirror: removed stale transformed entry ${toRemove}`) + continue + } + if (transformOpts.flattenDepth !== undefined && entry.isDirectory()) { + await cleanupStaleNestedDirs( + fileService, + join(destPath, entry.name), + entry.name, + expectedNested, + ) + } + } +} + +/** + * Removes sub-directories of a transformed entry that no longer correspond to a + * preserved source sub-path. Only directories are considered: a file inside an + * entry belongs to that entry and is handled by the entry's own copy/overwrite. + */ +async function cleanupStaleNestedDirs( + fileService: FileSystemService, + dirPath: string, + relativeDir: string, + expectedNested: Set, +): Promise { + const entries = await fileService.readdir(dirPath).catch(() => []) + for (const entry of entries) { + if (!entry.isDirectory()) continue + const relPath = `${relativeDir}/${entry.name}` + const childPath = join(dirPath, entry.name) + if (!expectedNested.has(relPath)) { + await fileService.rm(childPath, { recursive: true, force: true }) + logger.info(`Mirror: removed stale nested entry ${childPath}`) + continue + } + await cleanupStaleNestedDirs(fileService, childPath, relPath, expectedNested) } } diff --git a/packages/content-ops/src/ops/link-rewriter.ts b/packages/content-ops/src/ops/link-rewriter.ts index e8d7652d8..f312b7b79 100644 --- a/packages/content-ops/src/ops/link-rewriter.ts +++ b/packages/content-ops/src/ops/link-rewriter.ts @@ -78,11 +78,15 @@ function reRootTarget( * misplace a same-directory sibling link (see #313 T5 fixture regression). * * Scope: this function alone only rebases links resolving inside the SAME - * directory the current file lives in. Resolution is two-stage (see - * `resolveAbsoluteTarget`): own-dir rebase first, then `rebaseWithinMovedDirs` - * for a target in ANY other directory the same copy moved — a sub-doc's - * `../SKILL.md`, or one skill linking into another (e.g. plan-epics → - * map-subdomains) — and only then the coarser `reRootTarget` fallback. + * directory the current file lives in, which is why it is NOT the first stage of + * resolution (see `resolveAbsoluteTarget`): `movedDirs` already contains the + * file's own directory, so `rebaseWithinMovedDirs` subsumes this case AND picks + * the most specific move — necessary when a sub-directory of the file's own + * directory moved somewhere else (an unbounded flatten turns `./references/x.md` + * into a sibling entry). Asking this function first would let the less specific + * own-directory match win and leave the link pointing at a path that does not + * exist. It stays as the fallback for a caller that passes no `movedDirs` + * (`rewriteLinksInFile` is public API). */ function rebaseWithinMovedDir( absoluteTarget: string, @@ -107,8 +111,17 @@ type ComputeNewHrefParams = { /** * Resolves the absolute target a link points at, after accounting for the - * current copy operation: a same-directory rebase takes priority over the - * coarser dataset-root re-root (see `rebaseWithinMovedDir` docs). + * current copy operation. Three stages, most specific first (see the + * `rebaseWithinMovedDir` docs for why that order): + * + * 1. `rebaseWithinMovedDirs` — through whichever directory THIS copy moved + * contains the target, longest `originalDir` winning. Covers the file's own + * directory (it is in `movedDirs`), a sub-directory of it that moved + * elsewhere, a sub-doc's `../SKILL.md`, and one skill linking into another. + * 2. `rebaseWithinMovedDir` — the own-directory rebase, as the fallback for a + * caller that passes no `movedDirs`. + * 3. `reRootTarget` — the coarser dataset-root re-root, which only knows about + * the overall content-root move. */ function resolveAbsoluteTarget(params: { originalFileDir: string @@ -122,14 +135,17 @@ function resolveAbsoluteTarget(params: { params const absoluteTarget = posix.resolve(originalFileDir, pathPart) + // Every directory this copy moved, most specific (longest originalDir) first — + // including the file's own. A sub-doc linking UP to its skill resolves here, + // and so does a link DOWN into a sub-directory that moved somewhere other than + // under this file (the unbounded-flatten sibling case): the nested move wins + // over the file's own, which is why this runs before the own-dir rebase. + const viaMovedDir = rebaseWithinMovedDirs(absoluteTarget, movedDirs) + if (viaMovedDir) return viaMovedDir + // Fallback for a caller that passes no movedDirs (`rewriteLinksInFile` is + // public API): rebase a target inside the file's own directory. const rebased = rebaseWithinMovedDir(absoluteTarget, originalFileDir, newFileDir) if (rebased) return rebased - // The target is outside the file's own directory. It may still live in another - // directory this same copy moved — the commonest case being a sub-doc linking - // UP to its skill. Most specific (longest originalDir) wins, so a nested entry - // is preferred over its parent. - const viaSibling = rebaseWithinMovedDirs(absoluteTarget, movedDirs) - if (viaSibling) return viaSibling if (datasetRoot && sourceContentRoot) { return reRootTarget(absoluteTarget, datasetRoot, sourceContentRoot) } diff --git a/packages/content-ops/src/ops/naming-transforms.test.ts b/packages/content-ops/src/ops/naming-transforms.test.ts index ecc38e8fa..917f966df 100644 --- a/packages/content-ops/src/ops/naming-transforms.test.ts +++ b/packages/content-ops/src/ops/naming-transforms.test.ts @@ -5,6 +5,7 @@ import { transformPath, detectCollisions, isRegistryEntryPath, + isValidFlattenDepth, } from './naming-transforms' describe('flattenPath', () => { @@ -149,6 +150,13 @@ describe('flattenPath with a bounded depth (#407)', () => { expect(() => flattenPath('a/b/c/d', depth)).toThrow(/positive integer/) }) + // The depth is rejected whatever path it is paired with: an early return for a + // trivial path must not decide whether the value is validated at all. + it.each([0, -1, 1.5, NaN])('throws on the depth %s even for an empty path', depth => { + expect(() => flattenPath('', depth)).toThrow(/positive integer/) + expect(() => isRegistryEntryPath('', depth)).toThrow(/positive integer/) + }) + // The unbounded form is traversal-safe by construction (every separator // becomes a hyphen). A preserved tail is not: the copy pipeline joins it onto // the destination root, so `process/review/../../../../etc` would resolve to @@ -166,6 +174,31 @@ describe('flattenPath with a bounded depth (#407)', () => { }) }) +// The copy-time failure convention in this layer is the typed IO_ERROR, so a +// programmatic consumer that skipped CLI validation still gets +// `operation`/`path` context through the CLI error formatter. +describe('flattenPath failure typing (#407 review)', () => { + it('throws the ops layer IO_ERROR for an invalid depth', () => { + expect(() => flattenPath('a/b/c', 0)).toThrow(expect.objectContaining({ name: 'IO_ERROR' })) + }) + + it('throws the ops layer IO_ERROR for a traversal segment in the tail', () => { + expect(() => flattenPath('a/b/../c', 2)).toThrow(expect.objectContaining({ name: 'IO_ERROR' })) + }) +}) + +// One predicate, shared with the CLI config boundary so the two enforcement +// points cannot drift into two different rules. +describe('isValidFlattenDepth (#407 review)', () => { + it.each([1, 2, 10])('accepts the positive integer %s', value => { + expect(isValidFlattenDepth(value)).toBe(true) + }) + + it.each([0, -1, 1.5, NaN, '2', null, undefined, {}])('rejects %s', value => { + expect(isValidFlattenDepth(value)).toBe(false) + }) +}) + describe('isRegistryEntryPath (#407)', () => { it('treats every dir as an entry when the flatten is unbounded', () => { expect(isRegistryEntryPath('process/review/references')).toBe(true) diff --git a/packages/content-ops/src/ops/naming-transforms.ts b/packages/content-ops/src/ops/naming-transforms.ts index 41a4c010b..7413ce54f 100644 --- a/packages/content-ops/src/ops/naming-transforms.ts +++ b/packages/content-ops/src/ops/naming-transforms.ts @@ -4,6 +4,7 @@ * Prefix: prepends a prefix to the top-level directory name. * Both operations are independent and composable. */ +import { createError } from '../observability' /** * Naming transform options applied during a copy. One name for the concept @@ -17,13 +18,33 @@ */ export type TransformOpts = { flatten: boolean; prefix?: string; flattenDepth?: number } -/** A `flattenDepth` given as a JSON-config typo must fail loudly, not silently full-flatten. */ -function assertValidFlattenDepth(flattenDepth: number): void { - if (!Number.isInteger(flattenDepth) || flattenDepth < 1) { - throw new Error( - `flattenPath: flattenDepth must be a positive integer, got ${flattenDepth}. ` + +/** + * The one predicate for a valid `flattenDepth`, shared with the CLI config + * boundary (`validateFlattenDepthField`) so the rule cannot drift between the + * two places that enforce it. Each boundary keeps its own message; only the + * rule is shared. + */ +export function isValidFlattenDepth(value: unknown): boolean { + return typeof value === 'number' && Number.isInteger(value) && value >= 1 +} + +/** + * A `flattenDepth` given as a JSON-config typo must fail loudly, not silently + * full-flatten. Thrown as the ops layer's typed `IO_ERROR` (like the sibling + * collision error in `copy-directory-transforms.ts`) so a programmatic + * `@pair/content-ops` consumer that skipped CLI validation still gets + * `operation`/`path` context through the CLI error formatter. + */ +function assertValidFlattenDepth(flattenDepth: number, operation: string, path: string): void { + if (!isValidFlattenDepth(flattenDepth)) { + throw createError({ + type: 'IO_ERROR', + message: + `${operation}: flattenDepth must be a positive integer, got ${flattenDepth}. ` + 'Omit it to flatten every separator.', - ) + operation, + path, + }) } } @@ -36,10 +57,14 @@ function assertValidFlattenDepth(flattenDepth: number): void { function assertNoTraversalInTail(tail: string[], dirName: string): void { const offender = tail.find(segment => segment === '.' || segment === '..') if (offender !== undefined) { - throw new Error( - `flattenPath: refusing to preserve the relative segment '${offender}' of '${dirName}' — ` + + throw createError({ + type: 'IO_ERROR', + message: + `flattenPath: refusing to preserve the relative segment '${offender}' of '${dirName}' — ` + 'a preserved sub-path is joined onto the destination root and could escape it.', - ) + operation: 'flattenPath', + path: dirName, + }) } } @@ -66,10 +91,13 @@ function assertNoTraversalInTail(tail: string[], dirName: string): void { * preserved tail contains a `.`/`..` segment (see the two assertions above). */ export function flattenPath(dirName: string, flattenDepth?: number): string { + // Validated before ANY early return: an invalid depth must be rejected + // regardless of the path it happens to be paired with, or the fail-loudly + // guarantee becomes dependent on the other argument. + if (flattenDepth !== undefined) assertValidFlattenDepth(flattenDepth, 'flattenPath', dirName) const trimmed = dirName.replace(/^\/+/, '').replace(/\/+$/, '') if (trimmed === '') return '' if (flattenDepth === undefined) return trimmed.replace(/\//g, '-') - assertValidFlattenDepth(flattenDepth) const segments = trimmed.split('/') // Fewer segments than the entry depth: nothing below the entry to preserve, so @@ -97,9 +125,11 @@ export function flattenPath(dirName: string, flattenDepth?: number): string { */ export function isRegistryEntryPath(dirName: string, flattenDepth?: number): boolean { if (flattenDepth === undefined) return true + // Same ordering rationale as `flattenPath`: validate the depth before the + // empty-path early return. + assertValidFlattenDepth(flattenDepth, 'isRegistryEntryPath', dirName) const trimmed = dirName.replace(/^\/+/, '').replace(/\/+$/, '') if (trimmed === '') return true - assertValidFlattenDepth(flattenDepth) return trimmed.split('/').length <= flattenDepth } diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md index 115e558d6..a3887dfc1 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -26,9 +26,20 @@ The distribution pipeline flattens only the **entry** part of the source path an The sub-directory installs **inside** the skill, not as a sibling directory of its own. Two consequences for the author: -- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. +- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. (True for a skill at the standard `/` depth — see the exclusion below.) - **A sub-document is content, not a skill.** It is not a separate installed entry, gets no name prefix, and its frontmatter (if any) is left alone. Do not give a sub-document a `name:` matching a skill name, and do not expect a sub-directory name to be invocable. +## The one exclusion: a skill that is not at the standard depth + +The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`). A skill that sits **shallower** than that — directly at the registry root, with no category directory — cannot ship a sub-directory: its `/references/` has exactly the shape of a real `/` entry, so it would install as a **sibling** `--references/` rather than inside the skill. The pipeline **fails the copy loudly** in that case instead of installing something broken: + +```text +Ambiguous layout for a bounded flatten (flattenDepth=2): 'next' is 1 segment(s) deep, +holds files directly AND owns the sub-directory 'next/references'. … +``` + +To give such a skill a sub-directory, first move it under a category directory (that renames the installed skill, so it changes the name users invoke — decide deliberately). + ## Authoring rules 1. **Only the entry directory holds `SKILL.md`.** A `SKILL.md` inside a sub-directory would read as a second skill; put the entrypoint at the skill's root and nowhere else. From 018fb2879afa8c538baa11fc9f9b80d9b6a8e1a9 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 16:31:29 +0200 Subject: [PATCH 08/12] =?UTF-8?q?[US-407]=20fix:=20round=203=20=E2=80=94?= =?UTF-8?q?=20reject=20an=20entry=20DEEPER=20than=20flattenDepth,=20and=20?= =?UTF-8?q?close=20the=20depth-1=20traversal=20hole?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 //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 --- .../copy/copy-directory-transforms.test.ts | 40 +++++++++++++++ .../src/ops/copy/copy-directory-transforms.ts | 49 +++++++++++++++++++ .../src/ops/naming-transforms.test.ts | 20 ++++++++ .../content-ops/src/ops/naming-transforms.ts | 31 ++++++++---- 4 files changed, 131 insertions(+), 9 deletions(-) diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts index 14133bb94..e003998b4 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts @@ -400,6 +400,46 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => // covers targets inside itself. Before the fix this fell through to the // source-root fallback and became `../../../source/process/review/SKILL.md`: // a path back into the dataset layout, dead in the install. + // Round-3 Major on PR #411: the shallow guard's symmetric twin. An entry + // DEEPER than flattenDepth silently mis-installed — a REGRESSION the bounded + // flatten introduced, since full flattening handled it fine. + it('rejects an entry deeper than flattenDepth, instead of mis-installing it (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/capability/sub/foo/SKILL.md': '---\nname: foo\n---\n# /foo', + '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + }) + + await expect( + copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }), + ).rejects.toThrow(/entry too deep|holds none/) + }) + + // The rule must NOT fire on legitimate content, which is the whole point of + // the story: `references` is deeper than flattenDepth too, but its depth-2 + // ancestor holds files, so something owns it. + it('still accepts a nested references/ dir, whose ancestor is a real entry', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + '/dataset/source/process/review/references/deep.md': '# Deep', + }) + + await expect( + copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }), + ).resolves.toBeDefined() + }) + it('keeps a nested sub-doc back-link pointing at its own skill (#407)', async () => { const fileService = createTestFileService({ '/dataset/source/process/review/SKILL.md': diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts index 77369e6d5..a238dbf28 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts @@ -143,6 +143,54 @@ function validateNoShallowEntryWithSubdir( } } +/** + * The DEEPER half of the same layout mismatch, and the reason it needs its own + * rule: `validateNoShallowEntryWithSubdir` above rejects an entry SHALLOWER than + * `flattenDepth`. An entry DEEPER than it is equally unrepresentable and was + * silently mis-installed — a regression introduced by the bounded flatten itself. + * + * `capability/sub/foo/SKILL.md` (three segments, `flattenDepth` 2) installed at + * `pair-capability-sub/foo/SKILL.md`: a pseudo-entry directory with NO `SKILL.md` + * at its root, so the skill loader never sees the skill. Two further defects + * followed silently, because `isRegistryEntryPath` reports false for it: the + * frontmatter `name:` was left unsynced, and no entry reached `skillNameMap`, so + * a `/foo` reference in an unrelated skill stayed dangling. Before the bounded + * flatten the same source produced a perfectly usable `pair-capability-sub-foo/`. + * + * Telling a too-deep ENTRY from legitimate CONTENT uses the shape data already + * collected, with no `SKILL.md` knowledge (ADR-020's coupling argument): a + * directory deeper than `flattenDepth` that holds files directly is content **iff** + * its nearest ancestor at depth <= `flattenDepth` also holds files directly — that + * ancestor is the entry the content belongs to. `process/review/references` passes + * (its depth-2 ancestor `process/review` holds files); `capability/sub/foo` fails + * (`capability/sub` holds none, so nothing owns it). + */ +function validateNoDeepEntry(files: string[], transformOpts: TransformOpts, srcPath: string): void { + const { flattenDepth } = transformOpts + if (!transformOpts.flatten || flattenDepth === undefined || flattenDepth < 1) return + + const { dirsWithOwnFiles } = collectDirShapes(files) + + for (const dir of dirsWithOwnFiles) { + const segments = dir.split('/') + if (segments.length <= flattenDepth) continue + const ancestor = segments.slice(0, flattenDepth).join('/') + if (dirsWithOwnFiles.has(ancestor)) continue // content of a real entry + throw createError({ + type: 'IO_ERROR', + message: + `Ambiguous layout for a bounded flatten (flattenDepth=${flattenDepth}): '${dir}' is ` + + `${segments.length} segment(s) deep and holds files directly, but its ancestor at depth ` + + `${flattenDepth} ('${ancestor}') holds none — so nothing owns it as content and it is an ` + + `entry too deep. It would install at a path with no entry root, invisible to the skill ` + + `loader, with an unsynced frontmatter name and no skill-name mapping. ` + + `Move it to depth ${flattenDepth}, or give '${ancestor}' files of its own.`, + operation: 'copyDir', + path: join(srcPath, dir), + }) + } +} + /** * Copies a single file to its transformed location and tracks the * directory mapping for later link rewriting. @@ -320,6 +368,7 @@ export async function copyDirectoryWithTransforms(params: { const files = await collectFiles(fileService, srcPath, srcPath) validateNoCollisions(files, transformOpts, srcPath) validateNoShallowEntryWithSubdir(files, transformOpts, srcPath) + validateNoDeepEntry(files, transformOpts, srcPath) await fileService.mkdir(destPath, { recursive: true }) diff --git a/packages/content-ops/src/ops/naming-transforms.test.ts b/packages/content-ops/src/ops/naming-transforms.test.ts index 917f966df..bdb0c941f 100644 --- a/packages/content-ops/src/ops/naming-transforms.test.ts +++ b/packages/content-ops/src/ops/naming-transforms.test.ts @@ -242,3 +242,23 @@ describe('transformPath with a bounded flatten depth (#407)', () => { ) }) }) + +describe('bounded flatten is not less traversal-safe than the unbounded form (#411 round 3)', () => { + it('refuses a `..` head when flattenDepth is 1', () => { + // Returned '../evil' before the fix — escaping the destination root, in the + // very dimension the guard claims to harden. + expect(() => flattenPath('../evil', 1)).toThrow(/refusing to preserve/) + }) + + it('refuses a `.` head when flattenDepth is 1', () => { + expect(() => flattenPath('./evil', 1)).toThrow(/refusing to preserve/) + }) + + it('the unbounded form stays safe by construction, as before', () => { + expect(flattenPath('../evil')).toBe('..-evil') + }) + + it('a `..` in the head of a deeper bound is refused too', () => { + expect(() => flattenPath('../a/b', 2)).toThrow(/refusing to preserve/) + }) +}) diff --git a/packages/content-ops/src/ops/naming-transforms.ts b/packages/content-ops/src/ops/naming-transforms.ts index 7413ce54f..c0ababed3 100644 --- a/packages/content-ops/src/ops/naming-transforms.ts +++ b/packages/content-ops/src/ops/naming-transforms.ts @@ -49,19 +49,27 @@ function assertValidFlattenDepth(flattenDepth: number, operation: string, path: } /** - * A preserved tail is joined onto a destination root by the copy pipeline, so a - * `.`/`..` segment in it could escape that root. The unbounded form is - * traversal-safe by construction (every separator becomes a hyphen); the - * bounded form has to say no explicitly. + * A bounded result is joined onto a destination root by the copy pipeline, so a + * `.`/`..` segment surviving in it could escape that root. The unbounded form is + * traversal-safe by construction (every separator becomes a hyphen); the bounded + * form has to say no explicitly. + * + * Checks EVERY segment, not only the preserved tail. Round-3 review of PR #411: + * with `flattenDepth === 1` the head is a single un-joined segment, so + * `flattenPath('../evil', 1)` returned `'../evil'` — leaving the bounded form + * strictly LESS traversal-safe than the unbounded one it replaces, in exactly the + * dimension this guard advertises. Not reachable through the CLI today (dir names + * come from `dirname(relative(...))`, and no registry declares depth 1), which is + * why it was a hole rather than a live bug. */ -function assertNoTraversalInTail(tail: string[], dirName: string): void { - const offender = tail.find(segment => segment === '.' || segment === '..') +function assertNoTraversal(segments: string[], dirName: string): void { + const offender = segments.find(segment => segment === '.' || segment === '..') if (offender !== undefined) { throw createError({ type: 'IO_ERROR', message: `flattenPath: refusing to preserve the relative segment '${offender}' of '${dirName}' — ` + - 'a preserved sub-path is joined onto the destination root and could escape it.', + 'a bounded result is joined onto the destination root and could escape it.', operation: 'flattenPath', path: dirName, }) @@ -102,9 +110,14 @@ export function flattenPath(dirName: string, flattenDepth?: number): string { const segments = trimmed.split('/') // Fewer segments than the entry depth: nothing below the entry to preserve, so // this is the unbounded result — a shallower entry is never padded. - if (segments.length <= flattenDepth) return segments.join('-') + if (segments.length <= flattenDepth) { + // With flattenDepth 1 a single segment is returned UN-joined, so hyphen-joining + // cannot neutralise a `..` here either. + if (segments.length === 1) assertNoTraversal(segments, dirName) + return segments.join('-') + } + assertNoTraversal(segments, dirName) const tail = segments.slice(flattenDepth) - assertNoTraversalInTail(tail, dirName) return [segments.slice(0, flattenDepth).join('-'), ...tail].join('/') } From bf76aa603ed1ff51e63702b3d359bd2191e8cc41 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 18:29:50 +0200 Subject: [PATCH 09/12] =?UTF-8?q?[US-407]=20test:=20split=20the=20mirror?= =?UTF-8?q?=20fixture=20=E2=80=94=20one=20entry=20depth=20per=20property?= =?UTF-8?q?=20it=20can=20prove?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/tools/skill-md-mirror.test.ts | 46 +++++++++++++++---- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts index 8901c01c7..b78b714fd 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts @@ -130,6 +130,9 @@ describe('directional guard ignores root-only artifacts with no dataset source', it('enumerates ONLY the dataset-derived expected set, never a root-only skill', async () => { // Synthetic dataset that deliberately does NOT contain `agent-browser` (a real root-only skill). const tree: DatasetTree = { + // The nested reference needs a TWO-segment entry: a one-segment entry owning + // a sub-directory is refused outright by the shallow-entry guard. + 'catalog/nested/SKILL.md': '---\nname: nested\ndescription: "n"\n---\n\n# nested\n', 'capability/verify-quality/SKILL.md': '---\nname: verify-quality\n---\n\n# vq\n', 'next/SKILL.md': '---\nname: next\n---\n\n# next\n', } @@ -315,9 +318,19 @@ describe('installedArtifactPath — root location via the real naming transform' expect(installedArtifactPath('next/SKILL.md')).toBe('pair-next/SKILL.md') }) - it('flattens a nested sub-dir into its own prefixed dir, as the pipeline does today (#407)', () => { + // Inverted when #407 landed. This asserted the DEFECT — the sub-dir flattened + // OUT of its skill into a sibling pseudo-skill — deliberately, because the + // guard's job is to mirror whatever the real pipeline does, and pinning the + // then-current behaviour is what kept the guard honest until the pipeline was + // fixed. The bounded flatten (`flattenDepth`) now keeps it inside the skill, + // so the expectation flips with it. + it('keeps a nested sub-dir INSIDE its skill, as the bounded flatten does (#407)', () => { expect(installedArtifactPath('process/review/references/deep.md')).toBe( - 'pair-process-review-references/deep.md', + 'pair-process-review/references/deep.md', + ) + // The sibling shape the pipeline used to produce is now unreachable. + expect(installedArtifactPath('process/review/references/deep.md')).not.toContain( + 'pair-process-review-references', ) }) @@ -457,7 +470,7 @@ describe('drift-injection: guard fails on each drift class, passes when reconcil */ describe('drift-injection on sub-docs and nested references (non-SKILL.md artifacts)', () => { const SUB = 'demo/sub-doc.md' - const NESTED = 'demo/references/deep.md' + const NESTED = 'catalog/nested/references/deep.md' const SUB_ROOT = '.claude/skills/pair-demo/sub-doc.md' const MINI: DatasetTree = { 'demo/SKILL.md': '---\nname: demo\ndescription: "d"\n---\n\n# demo\n', @@ -467,6 +480,10 @@ describe('drift-injection on sub-docs and nested references (non-SKILL.md artifa // `/command` -> `/pair-*` skill-reference rewrite. [SUB]: '# sub\n\nDisclosed from [SKILL.md](SKILL.md).\nSee [kb](../../.pair/kb.md).\nCompose /verify-quality first.\nTail line kept unchanged.\nAnother tail line.\nAnd a third.\n', + // The nested reference needs a TWO-segment entry that holds files of its own: + // a one-segment entry owning a sub-directory, and an entry deeper than the + // flatten depth, are both refused by the layout guards. + 'catalog/nested/SKILL.md': '---\nname: nested\ndescription: "n"\n---\n\n# nested\n', [NESTED]: '# deep\n\nCompose /verify-quality here.\n', 'capability/verify-quality/SKILL.md': '---\nname: verify-quality\n---\n\n# vq\n', // non-markdown noise: copied by the pipeline, never asserted as an artifact @@ -481,14 +498,18 @@ describe('drift-injection on sub-docs and nested references (non-SKILL.md artifa it('asserts the sub-doc and the nested reference, and NOT the non-markdown file', () => { expect([...mirror.byDatasetPath.keys()].sort()).toEqual([ 'capability/verify-quality/SKILL.md', - 'demo/SKILL.md', + 'catalog/nested/SKILL.md', NESTED, + 'demo/SKILL.md', SUB, ]) expect(mirror.producedPaths).toContain('pair-demo/sub-doc.md') - // a nested subdir is FLATTENED into its own top-level prefixed dir (current - // pipeline behavior — a defect tracked in #407, mirrored here, not endorsed) - expect(mirror.producedPaths).toContain('pair-demo-references/deep.md') + // The nested subdir is PRESERVED inside its skill (#407). Its fixture entry is + // `catalog/nested` — two segments — because a one-segment entry owning a + // sub-directory is now refused outright by the shallow-entry layout guard, + // while the sub-doc case above stays on the one-segment `demo/` precisely + // because that is where the link-depth rewrite is observable. + expect(mirror.producedPaths).toContain('pair-catalog-nested/references/deep.md') // The pipeline REALLY copied the non-markdown file (asserted on the // destination tree, so this cannot pass vacuously) and the guard's asserted // set above still excludes it. @@ -499,13 +520,20 @@ describe('drift-injection on sub-docs and nested references (non-SKILL.md artifa const sub = mirror.byDatasetPath.get(SUB)! expect(sub).toContain('](./SKILL.md)') // sibling self-pointer normalised expect(sub).not.toContain('[SKILL.md](SKILL.md)') + // A ONE-segment entry is deliberate here: `demo/` sits 2 levels below the + // dataset root and `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 alone — a degenerate case that + // cannot demonstrate the bump. The nested-reference case needs the opposite + // (two segments, else the shallow-entry guard refuses it), so it has its own + // fixture below. expect(sub).toContain('](../../../.pair/kb.md)') // link-depth bump expect(sub).not.toContain('](../../.pair/kb.md)') expect(sub).toContain('/pair-capability-verify-quality') // skill-reference rewrite expect(sub).not.toContain(' /verify-quality ') }) - it('rewrites a nested reference file too (flattened into its own prefixed dir)', () => { + it('rewrites a nested reference file too (now preserved inside the skill, #407)', () => { expect(mirror.byDatasetPath.get(NESTED)).toContain('/pair-capability-verify-quality') }) @@ -558,6 +586,6 @@ describe('drift-injection on sub-docs and nested references (non-SKILL.md artifa const drifted = `${deep}stray\n` const message = captureThrownMessage(() => assertRootArtifactMatches(NESTED, deep, drifted)) expect(message).toContain(NESTED) - expect(message).toContain('.claude/skills/pair-demo-references/deep.md') + expect(message).toContain('.claude/skills/pair-catalog-nested/references/deep.md') }) }) From 8b3571764783ca3cb8c99b31bea930b74c4dc3e1 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 19:05:07 +0200 Subject: [PATCH 10/12] =?UTF-8?q?[US-407]=20fix:=20round=204=20=E2=80=94?= =?UTF-8?q?=20public=20docs=20for=20flattenDepth,=20broader=20rule=20state?= =?UTF-8?q?d=20+=20enforced?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tech/adr/adr-005-skills-infrastructure.md | 2 +- ...bounded-flatten-depth-entry-granularity.md | 9 +- .../skill-conventions/nested-sub-documents.md | 21 +- .pair/llms.txt | 2 + .../content/docs/reference/configuration.mdx | 22 +- .../docs/reference/skill-management.mdx | 18 +- .../copy/copy-directory-transforms.test.ts | 66 +++++- .../src/ops/copy/copy-directory-transforms.ts | 204 ++++-------------- .../src/ops/copy/layout-validation.ts | 188 ++++++++++++++++ .../src/ops/naming-transforms.test.ts | 14 +- .../content-ops/src/ops/naming-transforms.ts | 44 ++-- .../skill-conventions/nested-sub-documents.md | 21 +- .../src/tools/skill-md-mirror.ts | 31 +-- .../tools/skills-conformance-check.test.ts | 81 +++++++ .../src/tools/skills-conformance-check.ts | 63 +++++- 15 files changed, 561 insertions(+), 225 deletions(-) create mode 100644 packages/content-ops/src/ops/copy/layout-validation.ts diff --git a/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md b/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md index 5009200b8..d00169a21 100644 --- a/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md +++ b/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md @@ -14,7 +14,7 @@ Introduce a `.skills/` directory within the KB dataset as the source of truth fo 2. **Naming transforms**: Flatten (`navigator/next` → `navigator-next`) + prefix (`pair-navigator-next`) to produce unique, tool-compatible directory names. **Amended by ADR-020**: flatten joins only the registry's ENTRY segments (`flattenDepth: 2` for `skills`); a deeper segment is content *of* the skill and installs inside it (`process/review/references` → `pair-process-review/references`, not the sibling `pair-process-review-references`). 3. **Multi-target distribution**: One canonical target (`.claude/skills/`) receives the physical copy; 5 secondary targets (`.github/skills/`, `.cursor/skills/`, `.agent/skills/`, `.agents/skills/`, `.windsurf/skills/`) receive symlinks pointing to canonical. 4. **Link rewriting**: Relative markdown links inside skill files are rewritten after flatten/prefix copy to maintain correctness. -5. **Configuration**: Skills registry defined in `config.json` with `flatten: true`, `flattenDepth: 2` (ADR-020), `prefix: "pair"`, `behavior: "mirror"`, and explicit `targets[]` array. +5. **Configuration**: Skills registry defined in `config.json` with `flatten: true`, `flattenDepth: 2` (ADR-020), `prefix: "pair"`, `behavior: "overwrite"`, and explicit `targets[]` array. ## Rationale diff --git a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md index 3ed4383ed..e3370c1f1 100644 --- a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md +++ b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md @@ -60,15 +60,18 @@ Corollary, recorded because it is not obvious: once flatten is bounded, a sub-di - A skill may ship a nested `references/` (or any sub-dir) and it installs **inside** the skill, with both link directions intact — the standard progressive-disclosure layout becomes usable in the pair corpus. - Non-skill registries are byte-for-byte unaffected; the change is provably scoped by the absence of the option. - An invalid depth fails loudly at config validation (`pair update`/`install`/`package`) and in `flattenPath` itself, instead of silently reverting to the defect. -- A bounded flatten cannot produce the traversal-unsafe tail an unbounded one made impossible: `.`/`..` in the preserved tail is rejected rather than joined onto the destination root. +- A bounded flatten cannot produce a traversal-unsafe result: a `.`/`..` segment that would survive into the result is rejected rather than joined onto the destination root. Applied by SHAPE, not by branch, so the two forms stay symmetric — the unbounded form's "safe by construction" argument (every separator becomes a hyphen) does not cover a single-segment path, which has no separator, so that case is rejected in both forms. ### Trade-offs and Limitations - **The number is a duplicated fact.** `.skills/` layout depth is asserted in `apps/pair-cli/config.json` and mirrored in `SKILL_COPY_OPTS`. A pin test ties the two together, but a `.skills/` reorganization must still update the number deliberately. - **Two flatten semantics coexist** in one codebase (bounded for `skills`, unbounded elsewhere). Accepted as the price of not changing registries that have no defect; the regression-witness test documents the unbounded branch as intentional, not vestigial. - Collision detection (`detectCollisions`) now compares sub-paths, not only top-level names. No behaviour change for the current corpus (no dataset skill dir is deeper than two segments) but a future nested-name clash surfaces as a sub-path collision message. -- **A registry-root entry cannot ship a sub-directory while `flattenDepth` is 2.** The depth assumes every entry sits at the same depth, and `.skills/` does not: `next/` is a ONE-segment entry (installed as `pair-next`) beside 39 two-segment ones. `next/references` would therefore have the same shape as the entry `process/review` and install as the sibling `pair-next-references/` — the whole defect back, for that one skill. The copy **fails loudly** on that shape (a directory shallower than `flattenDepth` that holds files directly AND owns a sub-directory) rather than mis-installing; a category directory is told apart from an entry by whether it holds files directly, so no marker-file knowledge enters the transform. Consequence for authors: giving `next` a sub-directory requires moving it under a category first, which **renames the installed skill users invoke**. Stated in `nested-sub-documents.md` as the one exclusion to the convention. -- Mirror cleanup descends one level per preserved sub-path under a bounded flatten (it was top-level-only, which a sub-path move made insufficient — a `references/` removed from the source would have stayed installed). Unbounded mirrors keep the exact previous top-level-only behaviour, gated on the option's presence. +- **Every entry must sit at the declared depth; both deviations abort the copy.** The depth assumes a uniform layout, and `.skills/` already bends it: `next/` is a ONE-segment entry (installed as `pair-next`) beside 39 two-segment ones. The enforced rules, stated as the code enforces them (not only for the case that motivated them): + - **Too shallow** — *any* directory shallower than `flattenDepth` that holds files directly AND owns a sub-directory. That is broader than "a registry-root skill cannot ship a sub-directory": it equally rejects a **category directory with a file of its own** (a `process/README.md` beside `process/review/`), because entry-vs-category is decided by "holds files directly" — the price of keeping marker-file knowledge (`SKILL.md`) out of a transform four non-skill registries share. The shape is genuinely unrepresentable either way: `next/references` has the same shape as the entry `process/review` and would install as the sibling `pair-next-references/` — the whole defect back. Consequence for authors: giving `next` a sub-directory requires moving it under a category first, which **renames the installed skill users invoke**; a category-level file must move into a skill directory instead. + - **Too deep** — a directory deeper than `flattenDepth` holding files whose nearest ancestor at the entry depth holds none, i.e. an entry too deep with nothing owning it as content. It would install under a directory with no entrypoint at its root, invisible to the skill loader. +- Both deviations are stated in `nested-sub-documents.md` as the convention's two exclusions, with the way out per shape. The too-deep case is guarded twice: at copy time (above) for a directory nothing owns, and statically over the dataset corpus by the skills conformance gate (`skills:conformance` fails on any `SKILL.md` below the entry depth) — because a `SKILL.md` inside a real skill's `references/` IS legitimately-shaped content for the copy-time guard, and would install silently non-invocable. +- Mirror cleanup descends one level per preserved sub-path under a bounded flatten (it was top-level-only, which a sub-path move made insufficient — a `references/` removed from the source would have stayed installed). Unbounded mirrors keep the exact previous top-level-only behaviour, gated on the option's presence. **ACCEPTED RESIDUAL:** the descent is reachable only under `behavior: 'mirror'`, and the only registry declaring `flattenDepth` today (`skills`) declares `behavior: 'overwrite'`, so on the live `pair update` path a `references/` deleted from the dataset stays installed — consistent with the pre-existing `overwrite` semantics for a whole skill directory, which never deleted either. The descent is kept as forward-compatibility for a `skills` flip to `mirror` (and for any other registry adopting `flattenDepth`), is covered by unit tests, and is NOT claimed as a live fix. ## Adoption Impact diff --git a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md index a3887dfc1..61c80a469 100644 --- a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md +++ b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -26,23 +26,34 @@ The distribution pipeline flattens only the **entry** part of the source path an The sub-directory installs **inside** the skill, not as a sibling directory of its own. Two consequences for the author: -- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. (True for a skill at the standard `/` depth — see the exclusion below.) +- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. (True for a skill at the standard `/` depth — see the exclusions below.) - **A sub-document is content, not a skill.** It is not a separate installed entry, gets no name prefix, and its frontmatter (if any) is left alone. Do not give a sub-document a `name:` matching a skill name, and do not expect a sub-directory name to be invocable. -## The one exclusion: a skill that is not at the standard depth +## The two exclusions: every entry must sit at the declared depth -The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`). A skill that sits **shallower** than that — directly at the registry root, with no category directory — cannot ship a sub-directory: its `/references/` has exactly the shape of a real `/` entry, so it would install as a **sibling** `--references/` rather than inside the skill. The pipeline **fails the copy loudly** in that case instead of installing something broken: +The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`) and cannot represent a directory holding files at any other depth. Both sides fail the copy **loudly**, before a single file is written, rather than installing something broken. + +### Too shallow: a directory above the entry depth that holds files of its own + +Stated as the pipeline enforces it: **a directory shallower than `flattenDepth` may not hold files of its own once it owns sub-directories.** Two shapes hit this, not one: + +- a **skill at the registry root**, with no category directory: its `/references/` has exactly the shape of a real `/` entry, so it would install as a **sibling** `--references/` rather than inside the skill; +- a **category directory with a file of its own** (e.g. a `/README.md` beside the skill directories): the pipeline tells a category apart from an entry by whether it holds files directly — deliberately, so no `SKILL.md` knowledge leaks into a transform shared by non-skill registries — so a category that holds a file reads as an entry with a sub-directory. ```text Ambiguous layout for a bounded flatten (flattenDepth=2): 'next' is 1 segment(s) deep, holds files directly AND owns the sub-directory 'next/references'. … ``` -To give such a skill a sub-directory, first move it under a category directory (that renames the installed skill, so it changes the name users invoke — decide deliberately). +Ways out, per shape: move a registry-root skill under a category directory (that renames the installed skill, so it changes the name users invoke — decide deliberately), or move the category-level file into a skill directory (or out of the registry). + +### Too deep: an entry below the entry depth that nothing owns as content + +A `SKILL.md` one level too deep (`///SKILL.md` at `flattenDepth: 2`) would install under a directory with no entrypoint at its root — invisible to the skill loader. It is told apart from legitimate content by its nearest ancestor at the entry depth: content belongs to an ancestor that holds files of its own (`/` holds `SKILL.md`), an entry too deep has no such owner. Move it to the entry depth. ## Authoring rules -1. **Only the entry directory holds `SKILL.md`.** A `SKILL.md` inside a sub-directory would read as a second skill; put the entrypoint at the skill's root and nowhere else. +1. **Only the entry directory holds `SKILL.md`.** A `SKILL.md` inside a sub-directory installs as content — no name sync, no invocable directory — so it would be a skill nobody can reach; put the entrypoint at the skill's root and nowhere else. Where a skills conformance gate runs, it fails on any `SKILL.md` below the entry depth. 2. **Point at sub-documents with relative links** from `SKILL.md`, at the step that needs them, so the executor follows the pointer when it gets there — the same way it follows pointers to guideline files. 3. **Keep the entrypoint self-sufficient for the happy path.** A sub-document is for depth (long catalogs, worked examples, edge-case tables), not for material the skill needs on every run. 4. **One level is usually enough.** Deeper nesting is preserved too, but it costs the reader a hop for no gain in most cases. diff --git a/.pair/llms.txt b/.pair/llms.txt index f31f70c8e..fb593777b 100644 --- a/.pair/llms.txt +++ b/.pair/llms.txt @@ -41,6 +41,7 @@ - [Structured Monolith Architecture](.pair/knowledge/guidelines/architecture/deployment-architectures/structured-monolith.md) - [Bounded Context Patterns and Implementation](.pair/knowledge/guidelines/architecture/design-patterns/bounded-contexts.md) - [Context Map Inline-Maintenance Guideline](.pair/knowledge/guidelines/architecture/design-patterns/context-map-maintenance.md) +- [Coupling Balance](.pair/knowledge/guidelines/architecture/design-patterns/coupling-balance.md) - [Domain-Driven Design (DDD) Implementation Guide](.pair/knowledge/guidelines/architecture/design-patterns/domain-driven-design.md) - [System Integration Patterns](.pair/knowledge/guidelines/architecture/design-patterns/integration-patterns.md) - [Monorepo Architecture](.pair/knowledge/guidelines/architecture/design-patterns/monorepo.md) @@ -332,6 +333,7 @@ - [Graceful Degradation — Standard Bullets](.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/graceful-degradation.md) - [Guided / Quick Setup Convention](.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/guided-quick-setup.md) - [Idempotency Convention](.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/idempotency.md) +- [Nested Sub-Documents (Progressive Disclosure)](.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md) - [Output Format Shapes](.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/output-shapes.md) - [Skill Conventions — Shared KB References](.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/README.md) - [`/pair-capability-record-decision` Invocation Contract](.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/record-decision-contract.md) diff --git a/apps/website/content/docs/reference/configuration.mdx b/apps/website/content/docs/reference/configuration.mdx index c03f47a7b..a37122621 100644 --- a/apps/website/content/docs/reference/configuration.mdx +++ b/apps/website/content/docs/reference/configuration.mdx @@ -43,6 +43,7 @@ Override with `--config ` on any command. | `description` | `string` | Yes | Human-readable description shown by `--list-targets` | | `include` | `string[]` | No | Glob patterns for files to include (default: all) | | `flatten` | `boolean` | No | Flatten directory hierarchy into dash-separated names | +| `flattenDepth` | `number` | No | Bound flattening to the registry's entry depth — the first N source segments are joined, anything deeper is preserved as a real sub-path. Requires `flatten: true`; must be a positive integer. Omitted = every separator is flattened | | `prefix` | `string` | No | Prefix to prepend to flattened directory names | | `targets` | `Target[]` | No | Multi-target distribution configuration | @@ -105,14 +106,33 @@ The transform also updates: - Markdown links between transformed files - Skill cross-references (e.g., `/verify-quality` → `/pair-capability-verify-quality`) +### Bounded Flattening (`flattenDepth`) + +Without `flattenDepth`, **every** separator is flattened — so a sub-directory of an entry becomes a sibling entry of its own. Set `flattenDepth` to the number of source segments an entry occupies and only those are joined; anything deeper stays a real sub-path inside the entry: + +```text +Source: .skills/process/review/references/deep.md +Flatten: process/review/references → process-review/references # flattenDepth: 2 +Prefix: process-review/references → pair-process-review/references +Target: .claude/skills/pair-process-review/references/deep.md +``` + +Compare with the unbounded result for the same source: `.claude/skills/pair-process-review-references/deep.md` — a sibling directory outside the skill, with the skill's own `./references/deep.md` link pointing at nothing. + +Notes: +- Set it when your registry's entries own sub-directories (e.g. an Agent Skills `references/` progressive-disclosure directory). +- A directory deeper than `flattenDepth` is treated as **content**, not an entry: it gets no prefix, no frontmatter `name` sync, and no skill-reference mapping. +- Every entry must sit at that same depth. A source layout the depth cannot represent — an entry shallower than `flattenDepth` that owns a sub-directory, or one deeper than it — **aborts the copy** with an explicit error instead of installing something broken. + ## Example: Skills Registry ```json { "skills": { "source": ".skills", - "behavior": "mirror", + "behavior": "overwrite", "flatten": true, + "flattenDepth": 2, "prefix": "pair", "description": "Agent skills distributed to AI tool directories", "targets": [ diff --git a/apps/website/content/docs/reference/skill-management.mdx b/apps/website/content/docs/reference/skill-management.mdx index 965127437..e1debacfc 100644 --- a/apps/website/content/docs/reference/skill-management.mdx +++ b/apps/website/content/docs/reference/skill-management.mdx @@ -153,6 +153,7 @@ With the default config (`prefix: "pair"`, `flatten: true`): | `process/review/` | `pair-process-review/` | `/pair-process-review` | | `capability/verify-quality/` | `pair-capability-verify-quality/` | `/pair-capability-verify-quality` | | `capability/record-decision/` | `pair-capability-record-decision/` | `/pair-capability-record-decision` | +| `process/review/references/` (sub-dir of a skill) | `pair-process-review/references/` | none — content, not a skill | ### Collision Detection @@ -170,16 +171,23 @@ If two different source paths flatten to the same installed name (e.g. `a/b/` an The naming transform applies two functions in order: -1. **Flatten**: Replace `/` with `-` +1. **Flatten**: Replace `/` with `-`, bounded to the registry's entry depth (`flattenDepth: 2` for skills — see [Configuration](/docs/reference/configuration)) - `capability/verify-quality` → `capability-verify-quality` + - `process/review/references` → `process-review/references` (a skill's own sub-directory is preserved, not flattened into a sibling) -2. **Prefix**: Prepend the prefix with `-` separator +2. **Prefix**: Prepend the prefix with `-` separator (only the top-level segment) - `capability-verify-quality` → `pair-capability-verify-quality` + - `process-review/references` → `pair-process-review/references` ```typescript -// Flatten: replaces '/' with '-' -function flattenPath(dirName: string): string { - return dirName.replace(/\//g, '-') +// Flatten: joins the first `flattenDepth` segments with '-' and preserves the +// rest as a real sub-path. Omit flattenDepth to join every separator. +function flattenPath(dirName: string, flattenDepth?: number): string { + const segments = dirName.split('/') + if (flattenDepth === undefined || segments.length <= flattenDepth) { + return segments.join('-') + } + return [segments.slice(0, flattenDepth).join('-'), ...segments.slice(flattenDepth)].join('/') } // Prefix: prepends prefix with '-' separator diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts index e003998b4..b8adc56f5 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts @@ -395,11 +395,6 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => expect(skill).toContain('[deep dive](./references/deep.md)') }) - // The link half of #407. A sub-doc's link UP to its skill points at the - // PARENT directory, which the file's own dir-mapping cannot rebase — it only - // covers targets inside itself. Before the fix this fell through to the - // source-root fallback and became `../../../source/process/review/SKILL.md`: - // a path back into the dataset layout, dead in the install. // Round-3 Major on PR #411: the shallow guard's symmetric twin. An entry // DEEPER than flattenDepth silently mis-installed — a REGRESSION the bounded // flatten introduced, since full flattening handled it fine. @@ -440,6 +435,11 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => ).resolves.toBeDefined() }) + // The link half of #407. A sub-doc's link UP to its skill points at the + // PARENT directory, which the file's own dir-mapping cannot rebase — it only + // covers targets inside itself. Before the fix this fell through to the + // source-root fallback and became `../../../source/process/review/SKILL.md`: + // a path back into the dataset layout, dead in the install. it('keeps a nested sub-doc back-link pointing at its own skill (#407)', async () => { const fileService = createTestFileService({ '/dataset/source/process/review/SKILL.md': @@ -549,6 +549,62 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => expect(await fileService.exists('/dataset/target/pair-process-review/SKILL.md')).toBe(true) }) + // #411 round 4: the enforced rule is BROADER than "a registry-root skill + // cannot ship a sub-dir", and the broader form is the documented one (ADR-020 + // Trade-offs, nested-sub-documents.md). A CATEGORY dir with a file of its own + // hits it too, because entry-vs-category is decided by "holds files directly" + // — the price of keeping SKILL.md knowledge out of a transform four non-skill + // registries share. Pinned here so the rule cannot silently narrow, and so + // the cost of adding a category-level README is a visible, intentional fact. + it('refuses a CATEGORY dir that holds a file of its own beside a skill (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/README.md': '# Process skills', + '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + }) + + const run = copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }) + + await expect(run).rejects.toThrow(/Ambiguous layout for a bounded flatten/) + // The remediation must name the way out for THIS shape, not only "move the + // directory deeper", which is wrong advice for a category dir. + await expect(run).rejects.toThrow(/move\/remove the file\(s\) held directly by 'process'/) + expect(await fileService.exists('/dataset/target/pair-process/README.md')).toBe(false) + expect(await fileService.exists('/dataset/target/pair-process-review/SKILL.md')).toBe(false) + }) + + // #411 round 4: `flattenDepth` without `flatten` is a contradiction — the path + // transform stays unbounded while entry classification would be bounded, so a + // `references/` dir was dropped from the skill-name map and the frontmatter + // sync although nothing was flattened. The CLI rejects the combination, but + // `copyDirectoryWithTransforms` is public API, so the option is normalised + // away here: ONE source of truth for "is this copy bounded?". + it('ignores flattenDepth entirely when flatten is false, so every dir is an entry', async () => { + const fileService = createTestFileService({ + '/dataset/source/process/review/SKILL.md': '---\nname: review\n---\n# /review', + '/dataset/source/process/review/references/deep.md': + '---\nname: references\n---\n# Deep dive', + }) + + const result = await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: false, prefix: 'pair', flattenDepth: 2, targets: [] }, + }) + + // Unbounded path transform ⇒ unbounded classification: the nested dir is an + // entry like any other, exactly as before #407. + expect(result.skillNameMap!.get('references')).toBe('pair-process/review/references') + expect(result.skillNameMap!.get('review')).toBe('pair-process/review') + }) + // #407 review finding: with a bounded flatten, `process/review/references` // is a real sub-path and so appears in the dir mapping next to the skill // dirs. Treating it as a SKILL registered `references` as a skill name diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts index a238dbf28..67b33943c 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.ts @@ -1,9 +1,14 @@ import { join, relative, dirname } from 'path/posix' -import { logger, createError } from '../../observability' +import { logger } from '../../observability' import { copyFileHelper } from '../../file-system' import { FileSystemService } from '../../file-system' import { SyncOptions } from '../SyncOptions' -import { transformPath, detectCollisions, isRegistryEntryPath } from '../naming-transforms' +import { transformPath, isRegistryEntryPath } from '../naming-transforms' +import { + validateNoCollisions, + validateNoShallowEntryWithSubdir, + validateNoDeepEntry, +} from './layout-validation' import { rewriteLinksAfterTransform, PathMappingEntry } from '../link-rewriter' import { syncFrontmatter } from '../frontmatter-transform' import { @@ -39,158 +44,6 @@ async function collectFiles( return result } -/** - * Collects unique subdirectory names from a file list, validates no - * flatten collisions exist, and throws if any are found. - */ -function validateNoCollisions( - files: string[], - transformOpts: TransformOpts, - srcPath: string, -): void { - const dirSet = new Set() - for (const filePath of files) { - const dir = dirname(filePath) - if (dir !== '.') dirSet.add(dir) - } - const transformedDirs = [...dirSet].map(d => transformPath(d, transformOpts)) - const collisions = detectCollisions(transformedDirs) - if (collisions.length > 0) { - throw createError({ - type: 'IO_ERROR', - message: `Flatten naming collision detected: ${collisions.join(', ')}. Different source paths resolve to the same target name.`, - operation: 'copyDir', - path: srcPath, - }) - } -} - -/** - * Two facts about the source tree's shape, from the flat file list: which - * directories hold files DIRECTLY, and for each directory one example - * sub-directory (ancestors included, since a file list only names leaf dirs). - * Consumed by `validateNoShallowEntryWithSubdir`. - */ -function collectDirShapes(files: string[]): { - dirsWithOwnFiles: Set - firstChildDirOf: Map -} { - const dirsWithOwnFiles = new Set() - const firstChildDirOf = new Map() - for (const filePath of files) { - const dir = dirname(filePath) - // The source ROOT is never an entry: its files are copied straight to the - // destination root, untransformed. - if (dir === '.') continue - dirsWithOwnFiles.add(dir) - const segments = dir.split('/') - for (let i = 1; i < segments.length; i++) { - const parent = segments.slice(0, i).join('/') - if (!firstChildDirOf.has(parent)) { - firstChildDirOf.set(parent, segments.slice(0, i + 1).join('/')) - } - } - } - return { dirsWithOwnFiles, firstChildDirOf } -} - -/** - * Rejects the one source shape a bounded flatten cannot represent: an entry that - * sits SHALLOWER than `flattenDepth` and owns a sub-directory (#407 review). - * - * `flattenDepth` is a positional statement about the source layout — "an entry is - * N segments deep" (ADR-020). A registry whose entries are not all at that depth - * breaks it: in `.skills/` today `next/` is a ONE-segment entry while - * `process/review/` is two, so `next/references` — content of `next` — has the - * same shape as the entry `process/review`. It would install as the sibling - * `pair-next-references/`, reintroducing for `next` all four defects this option - * removes for a two-segment entry: misplacement outside the skill, a dead - * `./references/…` forward link, a bogus `references` skill-name mapping that - * leaks into unrelated files, and a path written as the sub-doc's `name:`. - * - * That shape is unrepresentable, not merely unhandled, so it fails loudly here — - * before any file is copied — rather than being guessed at. A category directory - * (only sub-directories) and an entry (files of its own) are told apart by - * whether the directory holds files DIRECTLY: no `SKILL.md` knowledge, per - * ADR-020's coupling argument. Files at the source ROOT are exempt: they are - * copied straight to the destination root and are never entries. - */ -function validateNoShallowEntryWithSubdir( - files: string[], - transformOpts: TransformOpts, - srcPath: string, -): void { - const { flattenDepth } = transformOpts - if (!transformOpts.flatten || flattenDepth === undefined || flattenDepth < 2) return - - const { dirsWithOwnFiles, firstChildDirOf } = collectDirShapes(files) - - for (const dir of dirsWithOwnFiles) { - const depth = dir.split('/').length - if (depth >= flattenDepth) continue - const child = firstChildDirOf.get(dir) - if (child === undefined) continue - throw createError({ - type: 'IO_ERROR', - message: - `Ambiguous layout for a bounded flatten (flattenDepth=${flattenDepth}): '${dir}' is ${depth} segment(s) deep, ` + - `holds files directly AND owns the sub-directory '${child}'. '${child}' is ${flattenDepth} segment(s) deep, so it cannot be told apart ` + - `from a real entry and would install as a sibling entry instead of inside '${dir}'. ` + - `Move '${dir}' ${flattenDepth - depth} level(s) deeper (e.g. under a category directory), or drop the sub-directory.`, - operation: 'copyDir', - path: join(srcPath, dir), - }) - } -} - -/** - * The DEEPER half of the same layout mismatch, and the reason it needs its own - * rule: `validateNoShallowEntryWithSubdir` above rejects an entry SHALLOWER than - * `flattenDepth`. An entry DEEPER than it is equally unrepresentable and was - * silently mis-installed — a regression introduced by the bounded flatten itself. - * - * `capability/sub/foo/SKILL.md` (three segments, `flattenDepth` 2) installed at - * `pair-capability-sub/foo/SKILL.md`: a pseudo-entry directory with NO `SKILL.md` - * at its root, so the skill loader never sees the skill. Two further defects - * followed silently, because `isRegistryEntryPath` reports false for it: the - * frontmatter `name:` was left unsynced, and no entry reached `skillNameMap`, so - * a `/foo` reference in an unrelated skill stayed dangling. Before the bounded - * flatten the same source produced a perfectly usable `pair-capability-sub-foo/`. - * - * Telling a too-deep ENTRY from legitimate CONTENT uses the shape data already - * collected, with no `SKILL.md` knowledge (ADR-020's coupling argument): a - * directory deeper than `flattenDepth` that holds files directly is content **iff** - * its nearest ancestor at depth <= `flattenDepth` also holds files directly — that - * ancestor is the entry the content belongs to. `process/review/references` passes - * (its depth-2 ancestor `process/review` holds files); `capability/sub/foo` fails - * (`capability/sub` holds none, so nothing owns it). - */ -function validateNoDeepEntry(files: string[], transformOpts: TransformOpts, srcPath: string): void { - const { flattenDepth } = transformOpts - if (!transformOpts.flatten || flattenDepth === undefined || flattenDepth < 1) return - - const { dirsWithOwnFiles } = collectDirShapes(files) - - for (const dir of dirsWithOwnFiles) { - const segments = dir.split('/') - if (segments.length <= flattenDepth) continue - const ancestor = segments.slice(0, flattenDepth).join('/') - if (dirsWithOwnFiles.has(ancestor)) continue // content of a real entry - throw createError({ - type: 'IO_ERROR', - message: - `Ambiguous layout for a bounded flatten (flattenDepth=${flattenDepth}): '${dir}' is ` + - `${segments.length} segment(s) deep and holds files directly, but its ancestor at depth ` + - `${flattenDepth} ('${ancestor}') holds none — so nothing owns it as content and it is an ` + - `entry too deep. It would install at a path with no entry root, invisible to the skill ` + - `loader, with an unsynced frontmatter name and no skill-name mapping. ` + - `Move it to depth ${flattenDepth}, or give '${ancestor}' files of its own.`, - operation: 'copyDir', - path: join(srcPath, dir), - }) - } -} - /** * Copies a single file to its transformed location and tracks the * directory mapping for later link rewriting. @@ -341,18 +194,35 @@ async function copyAllFilesWithTransform(params: { } /** - * Copies a directory with flatten/prefix naming transforms applied. - * Each file's directory path (relative to source) is transformed, then - * the file is copied to the transformed location under the target. + * The naming-transform options for this copy, from the caller's `SyncOptions`. + * + * `flattenDepth` is DROPPED when `flatten` is false, deliberately: it is a bound + * ON flattening, so with no flattening there is nothing to bound. Keeping it + * would leave the pipeline internally inconsistent — `transformPath` ignores the + * depth unless `flatten` is set, while `isRegistryEntryPath` consults it + * unconditionally, so a `{ flatten: false, flattenDepth: 2 }` copy would apply an + * UNBOUNDED path transform yet classify a third-level directory as content + * (silently dropping it from the skill-name map and the frontmatter `name:` sync). + * The CLI rejects that combination at config validation + * (`validateFlattenDepthField`), but `copyDirectoryWithTransforms` is public API + * of `@pair/content-ops`; normalising here keeps ONE source of truth for + * "bounded?" instead of gating every consumer of the option (#411 review). */ function buildTransformOpts(options?: SyncOptions): TransformOpts { const flatten = options?.flatten ?? false const prefix = options?.prefix - const flattenDepth = options?.flattenDepth + const flattenDepth = flatten ? options?.flattenDepth : undefined const base: TransformOpts = flattenDepth === undefined ? { flatten } : { flatten, flattenDepth } return prefix ? { ...base, prefix } : base } +/** + * Copies a directory with flatten/prefix naming transforms applied. + * Each file's directory path (relative to source) is transformed, then the file + * is copied to the transformed location under the target. Source-layout + * invariants are checked FIRST (see `layout-validation.ts`), so an unrepresentable + * layout aborts before anything is written. + */ export async function copyDirectoryWithTransforms(params: { fileService: FileSystemService srcPath: string @@ -415,10 +285,20 @@ export async function copyDirectoryWithTransforms(params: { * because one source subdirectory then maps to exactly one top-level target * directory. Under a BOUNDED flatten (`flattenDepth`, #407) a source * subdirectory maps to a target SUB-PATH instead, so cleanup descends into a - * transformed entry as well: without that, a `references/` removed from the - * source would stay installed forever and its progressive-disclosure docs would - * keep being loaded. Descent is gated on `flattenDepth` being present, so the - * unbounded path stays byte-for-byte as before. + * transformed entry as well: at that granularity a `references/` removed from the + * source is what would otherwise stay installed forever, with its + * progressive-disclosure docs still being loaded. Descent is gated on + * `flattenDepth` being present, so the unbounded path stays byte-for-byte as before. + * + * NOT a live fix — FORWARD-COMPATIBILITY, and deliberately so: this whole + * function runs only under `behavior: 'mirror'`, and the one registry declaring + * `flattenDepth` today (`skills`) declares `behavior: 'overwrite'`, so on the real + * `pair update` path a deleted `references/` still survives — exactly as a deleted + * whole skill directory always has under `overwrite`. The descent keeps the + * library's mirror contract correct at the granularity the bounded flatten + * introduced, for a `skills` flip to `mirror` or any other registry adopting the + * option; it is exercised by unit tests, not by production configuration. + * Recorded as an ACCEPTED RESIDUAL in ADR-020's Trade-offs (#411 review). * * `topLevelFiles` (file names copied directly from the source root, with no * subdirectory of their own) must be included in `expected` alongside the diff --git a/packages/content-ops/src/ops/copy/layout-validation.ts b/packages/content-ops/src/ops/copy/layout-validation.ts new file mode 100644 index 000000000..0a01e91cd --- /dev/null +++ b/packages/content-ops/src/ops/copy/layout-validation.ts @@ -0,0 +1,188 @@ +/** + * Source-layout invariants checked BEFORE a transformed copy writes anything. + * + * Separate from `copy-directory-transforms.ts` on purpose: these are pure shape + * analysis over the flat file list (no file system, no state shared with the copy + * path) and they change for a different reason — registry layout rules, not copy + * mechanics (#411 review). + * + * All three throw the ops layer's typed `IO_ERROR` and are called before the + * first file is copied, so a rejected layout leaves no half-install. + */ +import { join, dirname } from 'path/posix' +import { createError } from '../../observability' +import { transformPath, detectCollisions } from '../naming-transforms' +import type { TransformOpts } from './copy-types' + +/** + * Collects unique subdirectory names from a file list, validates no + * flatten collisions exist, and throws if any are found. + */ +export function validateNoCollisions( + files: string[], + transformOpts: TransformOpts, + srcPath: string, +): void { + const dirSet = new Set() + for (const filePath of files) { + const dir = dirname(filePath) + if (dir !== '.') dirSet.add(dir) + } + const transformedDirs = [...dirSet].map(d => transformPath(d, transformOpts)) + const collisions = detectCollisions(transformedDirs) + if (collisions.length > 0) { + throw createError({ + type: 'IO_ERROR', + message: `Flatten naming collision detected: ${collisions.join(', ')}. Different source paths resolve to the same target name.`, + operation: 'copyDir', + path: srcPath, + }) + } +} + +/** + * Two facts about the source tree's shape, from the flat file list: which + * directories hold files DIRECTLY, and for each directory one example + * sub-directory (ancestors included, since a file list only names leaf dirs). + * Consumed by `validateNoShallowEntryWithSubdir`. + */ +export function collectDirShapes(files: string[]): { + dirsWithOwnFiles: Set + firstChildDirOf: Map +} { + const dirsWithOwnFiles = new Set() + const firstChildDirOf = new Map() + for (const filePath of files) { + const dir = dirname(filePath) + // The source ROOT is never an entry: its files are copied straight to the + // destination root, untransformed. + if (dir === '.') continue + dirsWithOwnFiles.add(dir) + const segments = dir.split('/') + for (let i = 1; i < segments.length; i++) { + const parent = segments.slice(0, i).join('/') + if (!firstChildDirOf.has(parent)) { + firstChildDirOf.set(parent, segments.slice(0, i + 1).join('/')) + } + } + } + return { dirsWithOwnFiles, firstChildDirOf } +} + +/** + * Rejects source shapes a bounded flatten cannot represent on the SHALLOW side: + * a directory shallower than `flattenDepth` that holds files directly AND owns a + * sub-directory (#407 review). + * + * `flattenDepth` is a positional statement about the source layout — "an entry is + * N segments deep" (ADR-020). A directory holding files at a shallower depth + * breaks it: in `.skills/` today `next/` is a ONE-segment entry while + * `process/review/` is two, so `next/references` — content of `next` — has the + * same shape as the entry `process/review`. It would install as the sibling + * `pair-next-references/`, reintroducing for `next` all four defects this option + * removes for a two-segment entry: misplacement outside the skill, a dead + * `./references/…` forward link, a bogus `references` skill-name mapping that + * leaks into unrelated files, and a path written as the sub-doc's `name:`. + * + * The rule is deliberately BROADER than the case that motivated it, and the + * broader form is the documented one (ADR-020 Trade-offs, + * `nested-sub-documents.md`): a CATEGORY directory with a file of its own (a + * `process/README.md` beside `process/review/`) is rejected too. A category + * directory (only sub-directories) and an entry (files of its own) are told apart + * by whether the directory holds files DIRECTLY — no `SKILL.md` knowledge, per + * ADR-020's coupling argument — so a category that holds a file is + * indistinguishable from an entry with a sub-directory. The remediation names + * both ways out. Files at the source ROOT are exempt: they are copied straight to + * the destination root and are never entries. + * + * The shape is unrepresentable, not merely unhandled, so it fails loudly here — + * before any file is copied — rather than being guessed at. + */ +export function validateNoShallowEntryWithSubdir( + files: string[], + transformOpts: TransformOpts, + srcPath: string, +): void { + const { flattenDepth } = transformOpts + if (!transformOpts.flatten || flattenDepth === undefined || flattenDepth < 2) return + + const { dirsWithOwnFiles, firstChildDirOf } = collectDirShapes(files) + + for (const dir of dirsWithOwnFiles) { + const depth = dir.split('/').length + if (depth >= flattenDepth) continue + const child = firstChildDirOf.get(dir) + if (child === undefined) continue + const childDepth = child.split('/').length + throw createError({ + type: 'IO_ERROR', + message: + `Ambiguous layout for a bounded flatten (flattenDepth=${flattenDepth}): '${dir}' is ${depth} segment(s) deep, ` + + `holds files directly AND owns the sub-directory '${child}'. '${child}' is ${childDepth} segment(s) deep, so it cannot be told apart ` + + `from a real entry and would install as a sibling entry instead of inside '${dir}'. ` + + `Move '${dir}' ${flattenDepth - depth} level(s) deeper (e.g. under a category directory), ` + + `or move/remove the file(s) held directly by '${dir}' (a category directory must hold sub-directories only), ` + + `or drop the sub-directory.`, + operation: 'copyDir', + path: join(srcPath, dir), + }) + } +} + +/** + * The DEEPER half of the same layout mismatch, and the reason it needs its own + * rule: `validateNoShallowEntryWithSubdir` above rejects an entry SHALLOWER than + * `flattenDepth`. An entry DEEPER than it is equally unrepresentable and was + * silently mis-installed — a regression introduced by the bounded flatten itself. + * + * `capability/sub/foo/SKILL.md` (three segments, `flattenDepth` 2) installed at + * `pair-capability-sub/foo/SKILL.md`: a pseudo-entry directory with NO `SKILL.md` + * at its root, so the skill loader never sees the skill. Two further defects + * followed silently, because `isRegistryEntryPath` reports false for it: the + * frontmatter `name:` was left unsynced, and no entry reached `skillNameMap`, so + * a `/foo` reference in an unrelated skill stayed dangling. Before the bounded + * flatten the same source produced a perfectly usable `pair-capability-sub-foo/`. + * + * Telling a too-deep ENTRY from legitimate CONTENT uses the shape data already + * collected, with no `SKILL.md` knowledge (ADR-020's coupling argument): a + * directory deeper than `flattenDepth` that holds files directly is content **iff** + * its nearest ancestor at depth <= `flattenDepth` also holds files directly — that + * ancestor is the entry the content belongs to. `process/review/references` passes + * (its depth-2 ancestor `process/review` holds files); `capability/sub/foo` fails + * (`capability/sub` holds none, so nothing owns it). + * + * What this CANNOT see: an entrypoint file placed inside legitimate content (a + * `process/review/references/SKILL.md`) is correctly-shaped content here, and + * recognising it would need the marker-file knowledge this layer refuses. That + * one is caught statically over the dataset corpus by the skills conformance + * gate instead (`skills:conformance`). + */ +export function validateNoDeepEntry( + files: string[], + transformOpts: TransformOpts, + srcPath: string, +): void { + const { flattenDepth } = transformOpts + if (!transformOpts.flatten || flattenDepth === undefined || flattenDepth < 1) return + + const { dirsWithOwnFiles } = collectDirShapes(files) + + for (const dir of dirsWithOwnFiles) { + const segments = dir.split('/') + if (segments.length <= flattenDepth) continue + const ancestor = segments.slice(0, flattenDepth).join('/') + if (dirsWithOwnFiles.has(ancestor)) continue // content of a real entry + throw createError({ + type: 'IO_ERROR', + message: + `Ambiguous layout for a bounded flatten (flattenDepth=${flattenDepth}): '${dir}' is ` + + `${segments.length} segment(s) deep and holds files directly, but its ancestor at depth ` + + `${flattenDepth} ('${ancestor}') holds none — so nothing owns it as content and it is an ` + + `entry too deep. It would install at a path with no entry root, invisible to the skill ` + + `loader, with an unsynced frontmatter name and no skill-name mapping. ` + + `Move it to depth ${flattenDepth}, or give '${ancestor}' files of its own.`, + operation: 'copyDir', + path: join(srcPath, dir), + }) + } +} diff --git a/packages/content-ops/src/ops/naming-transforms.test.ts b/packages/content-ops/src/ops/naming-transforms.test.ts index bdb0c941f..b83f16360 100644 --- a/packages/content-ops/src/ops/naming-transforms.test.ts +++ b/packages/content-ops/src/ops/naming-transforms.test.ts @@ -254,10 +254,22 @@ describe('bounded flatten is not less traversal-safe than the unbounded form (#4 expect(() => flattenPath('./evil', 1)).toThrow(/refusing to preserve/) }) - it('the unbounded form stays safe by construction, as before', () => { + it('a multi-segment unbounded path stays safe by construction, as before', () => { + // Every separator becomes a hyphen, so the `..` is inert — unchanged. expect(flattenPath('../evil')).toBe('..-evil') }) + it('a SINGLE-segment path is refused in BOTH forms — no separator to hyphenate', () => { + // #411 round 4: the "safe by construction" claim covers only multi-segment + // paths. `flattenPath('..')` returned '..' verbatim, so the unbounded default + // four registries use was the LESS safe of the two once round 3 hardened the + // bounded form. Applied by shape now, so the two branches are symmetric. + expect(() => flattenPath('..')).toThrow(/refusing to preserve/) + expect(() => flattenPath('.')).toThrow(/refusing to preserve/) + expect(() => flattenPath('..', 2)).toThrow(/refusing to preserve/) + expect(() => flattenPath('..', 1)).toThrow(/refusing to preserve/) + }) + it('a `..` in the head of a deeper bound is refused too', () => { expect(() => flattenPath('../a/b', 2)).toThrow(/refusing to preserve/) }) diff --git a/packages/content-ops/src/ops/naming-transforms.ts b/packages/content-ops/src/ops/naming-transforms.ts index c0ababed3..5c40e6fce 100644 --- a/packages/content-ops/src/ops/naming-transforms.ts +++ b/packages/content-ops/src/ops/naming-transforms.ts @@ -49,18 +49,23 @@ function assertValidFlattenDepth(flattenDepth: number, operation: string, path: } /** - * A bounded result is joined onto a destination root by the copy pipeline, so a - * `.`/`..` segment surviving in it could escape that root. The unbounded form is - * traversal-safe by construction (every separator becomes a hyphen); the bounded - * form has to say no explicitly. + * A result is joined onto a destination root by the copy pipeline, so a `.`/`..` + * segment surviving in it could escape that root. * - * Checks EVERY segment, not only the preserved tail. Round-3 review of PR #411: - * with `flattenDepth === 1` the head is a single un-joined segment, so - * `flattenPath('../evil', 1)` returned `'../evil'` — leaving the bounded form - * strictly LESS traversal-safe than the unbounded one it replaces, in exactly the - * dimension this guard advertises. Not reachable through the CLI today (dir names - * come from `dirname(relative(...))`, and no registry declares depth 1), which is - * why it was a hole rather than a live bug. + * The "safe by construction" argument for the unbounded form — every separator + * becomes a hyphen, so `../evil` → `..-evil` is inert — holds only for a path with + * a separator to hyphenate. A SINGLE segment has none, in either form: + * `flattenPath('..')` returned `'..'` unchanged. So the guard is applied by SHAPE, + * not by branch (round-4 review of PR #411): every segment of a bounded result + * plus any single-segment result, bounded or not. That keeps the two branches + * symmetric — the bounded form is never LESS safe than the unbounded one it + * replaces (round-3's hole: `flattenPath('../evil', 1)` returned `'../evil'`), and + * never MORE strict than it either. + * + * Not reachable through the CLI (directory names come from + * `dirname(relative(root, path))`, which yields neither `.` nor `..` for a file + * under the root, and the source root's own files bypass the transform), so this + * is defensive depth for a programmatic `@pair/content-ops` caller. */ function assertNoTraversal(segments: string[], dirName: string): void { const offender = segments.find(segment => segment === '.' || segment === '..') @@ -95,8 +100,8 @@ function assertNoTraversal(segments: string[], dirName: string): void { * Omitted ⇒ every separator is flattened, exactly as before. Registries whose * entries are single-segment are unaffected either way. * - * Throws when `flattenDepth` is present but not a positive integer, and when the - * preserved tail contains a `.`/`..` segment (see the two assertions above). + * Throws when `flattenDepth` is present but not a positive integer, and when a + * `.`/`..` segment would survive into the result (see the two assertions above). */ export function flattenPath(dirName: string, flattenDepth?: number): string { // Validated before ANY early return: an invalid depth must be rejected @@ -105,17 +110,16 @@ export function flattenPath(dirName: string, flattenDepth?: number): string { if (flattenDepth !== undefined) assertValidFlattenDepth(flattenDepth, 'flattenPath', dirName) const trimmed = dirName.replace(/^\/+/, '').replace(/\/+$/, '') if (trimmed === '') return '' - if (flattenDepth === undefined) return trimmed.replace(/\//g, '-') const segments = trimmed.split('/') + // No separator to hyphenate ⇒ the segment survives verbatim, in BOTH forms. + if (segments.length === 1) assertNoTraversal(segments, dirName) + if (flattenDepth === undefined) return trimmed.replace(/\//g, '-') + // Fewer segments than the entry depth: nothing below the entry to preserve, so // this is the unbounded result — a shallower entry is never padded. - if (segments.length <= flattenDepth) { - // With flattenDepth 1 a single segment is returned UN-joined, so hyphen-joining - // cannot neutralise a `..` here either. - if (segments.length === 1) assertNoTraversal(segments, dirName) - return segments.join('-') - } + if (segments.length <= flattenDepth) return segments.join('-') + assertNoTraversal(segments, dirName) const tail = segments.slice(flattenDepth) return [segments.slice(0, flattenDepth).join('-'), ...tail].join('/') diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md index a3887dfc1..61c80a469 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -26,23 +26,34 @@ The distribution pipeline flattens only the **entry** part of the source path an The sub-directory installs **inside** the skill, not as a sibling directory of its own. Two consequences for the author: -- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. (True for a skill at the standard `/` depth — see the exclusion below.) +- **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. (True for a skill at the standard `/` depth — see the exclusions below.) - **A sub-document is content, not a skill.** It is not a separate installed entry, gets no name prefix, and its frontmatter (if any) is left alone. Do not give a sub-document a `name:` matching a skill name, and do not expect a sub-directory name to be invocable. -## The one exclusion: a skill that is not at the standard depth +## The two exclusions: every entry must sit at the declared depth -The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`). A skill that sits **shallower** than that — directly at the registry root, with no category directory — cannot ship a sub-directory: its `/references/` has exactly the shape of a real `/` entry, so it would install as a **sibling** `--references/` rather than inside the skill. The pipeline **fails the copy loudly** in that case instead of installing something broken: +The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`) and cannot represent a directory holding files at any other depth. Both sides fail the copy **loudly**, before a single file is written, rather than installing something broken. + +### Too shallow: a directory above the entry depth that holds files of its own + +Stated as the pipeline enforces it: **a directory shallower than `flattenDepth` may not hold files of its own once it owns sub-directories.** Two shapes hit this, not one: + +- a **skill at the registry root**, with no category directory: its `/references/` has exactly the shape of a real `/` entry, so it would install as a **sibling** `--references/` rather than inside the skill; +- a **category directory with a file of its own** (e.g. a `/README.md` beside the skill directories): the pipeline tells a category apart from an entry by whether it holds files directly — deliberately, so no `SKILL.md` knowledge leaks into a transform shared by non-skill registries — so a category that holds a file reads as an entry with a sub-directory. ```text Ambiguous layout for a bounded flatten (flattenDepth=2): 'next' is 1 segment(s) deep, holds files directly AND owns the sub-directory 'next/references'. … ``` -To give such a skill a sub-directory, first move it under a category directory (that renames the installed skill, so it changes the name users invoke — decide deliberately). +Ways out, per shape: move a registry-root skill under a category directory (that renames the installed skill, so it changes the name users invoke — decide deliberately), or move the category-level file into a skill directory (or out of the registry). + +### Too deep: an entry below the entry depth that nothing owns as content + +A `SKILL.md` one level too deep (`///SKILL.md` at `flattenDepth: 2`) would install under a directory with no entrypoint at its root — invisible to the skill loader. It is told apart from legitimate content by its nearest ancestor at the entry depth: content belongs to an ancestor that holds files of its own (`/` holds `SKILL.md`), an entry too deep has no such owner. Move it to the entry depth. ## Authoring rules -1. **Only the entry directory holds `SKILL.md`.** A `SKILL.md` inside a sub-directory would read as a second skill; put the entrypoint at the skill's root and nowhere else. +1. **Only the entry directory holds `SKILL.md`.** A `SKILL.md` inside a sub-directory installs as content — no name sync, no invocable directory — so it would be a skill nobody can reach; put the entrypoint at the skill's root and nowhere else. Where a skills conformance gate runs, it fails on any `SKILL.md` below the entry depth. 2. **Point at sub-documents with relative links** from `SKILL.md`, at the step that needs them, so the executor follows the pointer when it gets there — the same way it follows pointers to guideline files. 3. **Keep the entrypoint self-sufficient for the happy path.** A sub-document is for depth (long catalogs, worked examples, edge-case tables), not for material the skill needs on every run. 4. **One level is usually enough.** Deeper nesting is preserved too, but it costs the reader a hop for no gain in most cases. diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts index 11c61e70f..c73699d18 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.ts @@ -32,10 +32,11 @@ * artifact the dataset contributes, not only `SKILL.md`. The case list is * derived from the dataset at collection time and is recursive, so a new * sub-doc — or the first `references/` subdir — is covered with no test edit - * and no count anywhere. Caveat for that first `references/` subdir: today's - * pipeline INSTALLS it wrongly (flattened out of its skill, links broken) — a - * defect tracked in #407; this guard faithfully mirrors that behavior rather - * than endorsing it (see `installedArtifactPath`). + * and no count anywhere. That first `references/` subdir now installs INSIDE its + * skill, with both link directions intact — #407 bounded the flatten to the + * registry's entry depth, and `SKILL_COPY_OPTS` carries the `flattenDepth: 2` the + * registry declares, so the derivation below is correct rather than + * faithfully-wrong (see `installedArtifactPath`). * * ACCEPTED RESIDUAL — orphans (decided in #384's review): because the guard is * directional and `pair update` copies with behavior 'overwrite' (no @@ -199,18 +200,18 @@ export function datasetSkillArtifacts(tree: DatasetTree): string[] { * rather than under a preserved `references/`: the pipeline flattens every * directory segment, not just the skill's own. * - * That nested-flatten mapping is CURRENT PIPELINE BEHAVIOR, mirrored here - * faithfully — and it is a DEFECT, tracked in #407 (a skill's `references/` - * sub-doc installs outside its skill dir with both links broken). Do NOT read it - * as a sanctioned layout for a new `references/` sub-doc. Nor does it self-correct: - * #407's fix changes the copy pipeline's per-file PLACEMENT (`copy-directory-transforms.ts` - * maps `dirname(file)` through `transformPath` and joins), not `transformPath` itself, - * so this derivation goes STALE — what actually happens is that the produced-paths - * cross-check (derivation vs. the pipeline's real output set) fails loudly, and - * this function plus its expectations must be updated in that PR. + * Since #407 the mapping is BOUNDED: `SKILL_COPY_OPTS.flattenDepth` is the + * registry's entry depth, so a third segment (`references/`) is preserved as a + * real sub-path and a sub-doc installs INSIDE its skill + * (`process/review/references/deep.md` → `pair-process-review/references/deep.md`), + * with both link directions intact. That is the sanctioned layout for a new + * sub-doc — see `skill-conventions/nested-sub-documents.md` for the authoring rules + * and the two layouts the bound cannot represent. * - * That correspondence is not taken on trust — a guard test asserts this - * derivation reproduces the pipeline's actual output paths set-for-set. + * The correspondence with the real pipeline is not taken on trust — a guard test + * asserts this derivation reproduces the pipeline's actual output paths + * set-for-set, so a placement change in `copy-directory-transforms.ts` that this + * derivation does not follow fails loudly instead of drifting. */ export function installedArtifactPath(datasetArtifact: string): string { const dir = posix.dirname(datasetArtifact) diff --git a/packages/knowledge-hub/src/tools/skills-conformance-check.test.ts b/packages/knowledge-hub/src/tools/skills-conformance-check.test.ts index 6e42c47f0..aaecbe7b5 100644 --- a/packages/knowledge-hub/src/tools/skills-conformance-check.test.ts +++ b/packages/knowledge-hub/src/tools/skills-conformance-check.test.ts @@ -12,8 +12,11 @@ import { countByCategory, checkProseCounts, checkCategoryLabelCounts, + checkEntrypointDepth, + ENTRY_DEPTH, runChecks, } from './skills-conformance-check' +import { SKILL_COPY_OPTS } from './skill-md-mirror' import { join as pathJoin } from 'node:path' describe('parseFrontmatter', () => { @@ -302,3 +305,81 @@ describe('runChecks — block-scalar size-gate cannot be bypassed (finding 1)', expect(errors.length).toBeGreaterThan(0) }) }) + +/** + * Authoring rule 1 of `skill-conventions/nested-sub-documents.md` ("only the entry + * directory holds SKILL.md") was stated but unenforced — the same silent-hole class + * as #407's too-deep entry, one layer up. A `SKILL.md` inside a real skill's + * `references/` is correctly-shaped CONTENT for the copy pipeline's layout guards + * (recognising it there would need the `SKILL.md` knowledge ADR-020 keeps out of a + * shared transform), and the mirror-equality guard derives the installed path from + * that same transform, so it agrees with itself. Static corpus knowledge is the + * right layer, and this is it. + */ +describe('checkEntrypointDepth (#411 round 4)', () => { + const at = (rel: string) => pathJoin('/corpus', rel) + + it('accepts a SKILL.md at the entry depth, and the bare meta skill above it', () => { + expect( + checkEntrypointDepth('/corpus', [ + at('process/review/SKILL.md'), + at('capability/grill/SKILL.md'), + at('next/SKILL.md'), + ]), + ).toEqual([]) + }) + + it('ignores non-entrypoint markdown at any depth', () => { + // Sub-documents are the whole point of the convention — only the entrypoint + // NAME is depth-constrained. + expect( + checkEntrypointDepth('/corpus', [ + at('process/review/references/deep.md'), + at('process/review/merge-and-cascade.md'), + ]), + ).toEqual([]) + }) + + it('rejects a SKILL.md below the entry depth — it would install non-invocable', () => { + const errors = checkEntrypointDepth('/corpus', [at('process/review/references/SKILL.md')]) + expect(errors).toHaveLength(1) + expect(errors[0]).toContain('references') + expect(errors[0]).toContain('non-invocable') + }) + + it('rejects a SKILL.md at the registry root, which has no skill directory at all', () => { + const errors = checkEntrypointDepth('/corpus', [at('SKILL.md')]) + expect(errors).toHaveLength(1) + expect(errors[0]).toContain('registry root') + }) + + it("ENTRY_DEPTH is the registry's declared flattenDepth, not an independent number", () => { + // Same fact as `apps/pair-cli/config.json`'s `skills.flattenDepth`, itself + // pinned to that file by skill-md-mirror's own test. Duplicated as a plain + // constant so this gate script keeps running under ts-node with no build. + expect(ENTRY_DEPTH).toBe(SKILL_COPY_OPTS.flattenDepth) + }) +}) + +describe('runChecks — a too-deep SKILL.md fails the gate (#411 round 4)', () => { + const root = mkdtempSync(join(tmpdir(), 'skills-conformance-depth-')) + afterAll(() => rmSync(root, { recursive: true, force: true })) + + it('reads a SKILL.md the entry walk never reaches, and reports it (drives CLI exit 1)', () => { + mkdirSync(join(root, 'process/review/references'), { recursive: true }) + writeFileSync( + join(root, 'process/review', 'SKILL.md'), + '---\nname: review\ndescription: "Reviews."\n---\nbody', + ) + writeFileSync( + join(root, 'process/review/references', 'SKILL.md'), + '---\nname: bogus\ndescription: "Would install silently non-invocable."\n---\nbody', + ) + + const { errors, skillCount } = runChecks(root) + // The entry walk still counts ONE skill — which is exactly why nothing saw the + // second file before this check. + expect(skillCount).toBe(1) + expect(errors.some(e => e.includes('references') && e.includes('SKILL.md'))).toBe(true) + }) +}) diff --git a/packages/knowledge-hub/src/tools/skills-conformance-check.ts b/packages/knowledge-hub/src/tools/skills-conformance-check.ts index b3889177d..1e3141fde 100644 --- a/packages/knowledge-hub/src/tools/skills-conformance-check.ts +++ b/packages/knowledge-hub/src/tools/skills-conformance-check.ts @@ -17,7 +17,17 @@ * SKILL.md matches the real corpus dir count. Hard error, like every other * check here (promoted from WARN once #313/T1 (#325) regenerated next's * catalog to the real, stable count). - * 5. KB prose counts — the skill-count figures restated in the onboarding KB + * 5. Entrypoint depth — every `SKILL.md` sits at the registry's ENTRY depth + * (`//SKILL.md`, or the bare `/SKILL.md` meta skill), + * never below it. A `SKILL.md` inside a skill's sub-directory (e.g. + * `process/review/references/SKILL.md`) installs as CONTENT under the bounded + * flatten (#407, ADR-020): no name prefix, no frontmatter `name:` sync, no + * skill-name mapping — a skill nobody can invoke, and until this check nothing + * saw it (the corpus walk below only reads `//SKILL.md`, and + * the mirror-equality guard derives the installed path from the same + * transform, so it agrees with itself). The convention it enforces: + * `skill-conventions/nested-sub-documents.md`, authoring rule 1. + * 6. KB prose counts — the skill-count figures restated in the onboarding KB * prose (way-of-working.md, getting-started.md, skills-guide.md) match the * real corpus, across every restated form: the number-before-noun * "N skills"/"N Agent Skills" total, the "(P process + C capability + N @@ -311,6 +321,53 @@ export function checkCategoryLabelCounts( return errors } +// --- Entrypoint depth --- + +/** + * The `skills` registry's ENTRY depth in directory segments: `/` + * (2) or the bare meta skill `` (1). Same fact as `flattenDepth` in the + * registry config, pinned to `SKILL_COPY_OPTS` by test rather than imported, so + * this gate script stays dependency-free (it runs via ts-node before any build). + */ +export const ENTRY_DEPTH = 2 + +/** + * Every `SKILL.md` must sit AT the entry depth, never below it. + * + * A `SKILL.md` inside a skill's sub-directory (`process/review/references/SKILL.md`) + * is legitimately-shaped CONTENT for the copy pipeline's layout guards — telling it + * apart would need the marker-file knowledge ADR-020 keeps out of a transform four + * non-skill registries share. So it installs at + * `pair-process-review/references/SKILL.md`: no prefix, frontmatter `name:` left + * unsynced, absent from the skill-name map — a skill nobody can invoke, with no + * signal anywhere. Static corpus knowledge is the right layer for it; this is that + * check (`nested-sub-documents.md`, authoring rule 1). + * + * Takes the RECURSIVE markdown walk, not `collectSkillFiles`: the whole point is + * to see files the entry walk never reaches. + */ +export function checkEntrypointDepth(skillsDir: string, markdownFiles: string[]): string[] { + const errors: string[] = [] + for (const file of markdownFiles) { + if (basename(file) !== 'SKILL.md') continue + const rel = relative(skillsDir, file) + const depth = rel.split(sep).length - 1 + if (depth >= 1 && depth <= ENTRY_DEPTH) continue + const where = + depth > ENTRY_DEPTH + ? `below the entry depth, so it installs as content inside another skill` + : `at the registry root, so it installs as a loose file with no skill directory` + errors.push( + `${rel}: SKILL.md is ${depth} directory level(s) deep — ${where}. ` + + `A skill entrypoint must sit at the entry depth (1..${ENTRY_DEPTH}: '//SKILL.md', ` + + `or '/SKILL.md' for the meta skill): no prefix, no frontmatter name sync and no ` + + `skill-name mapping are applied anywhere else, so the skill would be non-invocable. ` + + `Move it to the skill root, or rename it if it is a sub-document.`, + ) + } + return errors +} + // --- Corpus walk --- /** @@ -372,6 +429,8 @@ export function runChecks(skillsDir: string): RunResult { for (const e of checkLinks(file, fm.body)) errors.push(`${rel}: ${e}`) } + errors.push(...checkEntrypointDepth(skillsDir, collectSkillMarkdownFiles(skillsDir))) + const nextFile = files.find(f => basename(dirname(f)) === 'next') if (nextFile) { errors.push(...checkCatalogCounts(readFileSync(nextFile, 'utf-8'), files.length)) @@ -399,7 +458,7 @@ if (require.main === module) { if (errors.length === 0) { console.log( - `PASS — ${skillCount} skills conformant (frontmatter portability, size limits, pointer resolution, catalog counts, KB prose counts incl. category headings/table cells)`, + `PASS — ${skillCount} skills conformant (frontmatter portability, size limits, pointer resolution, entrypoint depth, catalog counts, KB prose counts incl. category headings/table cells)`, ) process.exit(0) } else { From 109b18a69179cbbe1445e3c6727a5957d516221b Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 19:34:02 +0200 Subject: [PATCH 11/12] =?UTF-8?q?[US-407]=20docs:=20round=205=20=E2=80=94?= =?UTF-8?q?=20say=20what=20the=20code=20enforces=20(remedy,=20mixed=20dept?= =?UTF-8?q?h,=20guard=20asymmetry)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- ...bounded-flatten-depth-entry-granularity.md | 2 +- .../skill-conventions/nested-sub-documents.md | 5 ++-- .../content/docs/reference/configuration.mdx | 2 +- .../docs/tutorials/managing-ai-artifacts.mdx | 4 ++++ .../copy/copy-directory-transforms.test.ts | 24 +++++++++++++++++++ .../src/ops/copy/layout-validation.ts | 11 +++++++-- .../src/ops/naming-transforms.test.ts | 5 +++- .../content-ops/src/ops/naming-transforms.ts | 15 ++++++++---- .../skill-conventions/nested-sub-documents.md | 5 ++-- 9 files changed, 60 insertions(+), 13 deletions(-) diff --git a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md index e3370c1f1..caf4af110 100644 --- a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md +++ b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md @@ -60,7 +60,7 @@ Corollary, recorded because it is not obvious: once flatten is bounded, a sub-di - A skill may ship a nested `references/` (or any sub-dir) and it installs **inside** the skill, with both link directions intact — the standard progressive-disclosure layout becomes usable in the pair corpus. - Non-skill registries are byte-for-byte unaffected; the change is provably scoped by the absence of the option. - An invalid depth fails loudly at config validation (`pair update`/`install`/`package`) and in `flattenPath` itself, instead of silently reverting to the defect. -- A bounded flatten cannot produce a traversal-unsafe result: a `.`/`..` segment that would survive into the result is rejected rather than joined onto the destination root. Applied by SHAPE, not by branch, so the two forms stay symmetric — the unbounded form's "safe by construction" argument (every separator becomes a hyphen) does not cover a single-segment path, which has no separator, so that case is rejected in both forms. +- A bounded flatten cannot produce a traversal-unsafe result: a `.`/`..` segment that would survive into the result is rejected rather than joined onto the destination root. Applied by SHAPE, not by branch — the unbounded form's "safe by construction" argument (every separator becomes a hyphen) does not cover a single-segment path, which has no separator, so that case is rejected in both forms. The invariant is that each form is checked exactly where its own output could carry a live `.`/`..`, so neither form is less safe than the other; they are deliberately NOT equally strict, since a preserved tail is live where a hyphenated segment is inert. ### Trade-offs and Limitations diff --git a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md index 61c80a469..cab2cd208 100644 --- a/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md +++ b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -29,9 +29,9 @@ The sub-directory installs **inside** the skill, not as a sibling directory of i - **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. (True for a skill at the standard `/` depth — see the exclusions below.) - **A sub-document is content, not a skill.** It is not a separate installed entry, gets no name prefix, and its frontmatter (if any) is left alone. Do not give a sub-document a `name:` matching a skill name, and do not expect a sub-directory name to be invocable. -## The two exclusions: every entry must sit at the declared depth +## The two exclusions: layouts the entry depth cannot represent -The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`) and cannot represent a directory holding files at any other depth. Both sides fail the copy **loudly**, before a single file is written, rather than installing something broken. +The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`). A directory holding files at another depth is tolerated while it stays unambiguous — pair's own root-level `next` skill installs fine as `pair-next` — and becomes unrepresentable only in the two shapes below. Both fail the copy **loudly**, before a single file is written, rather than installing something broken. ### Too shallow: a directory above the entry depth that holds files of its own @@ -57,3 +57,4 @@ A `SKILL.md` one level too deep (`///SKILL.md` at `fl 2. **Point at sub-documents with relative links** from `SKILL.md`, at the step that needs them, so the executor follows the pointer when it gets there — the same way it follows pointers to guideline files. 3. **Keep the entrypoint self-sufficient for the happy path.** A sub-document is for depth (long catalogs, worked examples, edge-case tables), not for material the skill needs on every run. 4. **One level is usually enough.** Deeper nesting is preserved too, but it costs the reader a hop for no gain in most cases. +5. **Link a sub-document only from within its own skill.** Link rewriting for content OUTSIDE the skills registry (a knowledge-base guideline, an `AGENTS.md`) maps a skill's **entrypoint** path only — `///SKILL.md` → the installed skill directory. A link from outside to a sub-document path is left pointing at the source tree, which a consuming project does not install: a silently dead link. From outside, link the skill's `SKILL.md` and let it point onward (rule 2); per-file mappings are the follow-up if a direct deep link ever becomes necessary. Links *inside* the copied registry are rebased by the copy itself, so this constrains only the cross-registry direction. diff --git a/apps/website/content/docs/reference/configuration.mdx b/apps/website/content/docs/reference/configuration.mdx index a37122621..808a2c3d3 100644 --- a/apps/website/content/docs/reference/configuration.mdx +++ b/apps/website/content/docs/reference/configuration.mdx @@ -122,7 +122,7 @@ Compare with the unbounded result for the same source: `.claude/skills/pair-proc Notes: - Set it when your registry's entries own sub-directories (e.g. an Agent Skills `references/` progressive-disclosure directory). - A directory deeper than `flattenDepth` is treated as **content**, not an entry: it gets no prefix, no frontmatter `name` sync, and no skill-reference mapping. -- Every entry must sit at that same depth. A source layout the depth cannot represent — an entry shallower than `flattenDepth` that owns a sub-directory, or one deeper than it — **aborts the copy** with an explicit error instead of installing something broken. +- Entries normally all sit at that depth, and a mixed-depth corpus is still workable: a shallower entry is fine while it owns no sub-directory (pair's own root-level `next` skill installs beside 39 two-segment ones), and **aborts the copy** only once it does. So does an entry deeper than the depth that nothing owns as content. Either way the copy fails with an explicit error before writing anything, instead of installing something broken. ## Example: Skills Registry diff --git a/apps/website/content/docs/tutorials/managing-ai-artifacts.mdx b/apps/website/content/docs/tutorials/managing-ai-artifacts.mdx index c25c0ae3c..8b3ae0fd1 100644 --- a/apps/website/content/docs/tutorials/managing-ai-artifacts.mdx +++ b/apps/website/content/docs/tutorials/managing-ai-artifacts.mdx @@ -87,6 +87,7 @@ For the **folder-organized layout**, start with a single assistant: "source": ".skills", "behavior": "mirror", "flatten": true, + "flattenDepth": 2, "prefix": "acme", "description": "Acme team skills", "targets": [ @@ -97,6 +98,8 @@ For the **folder-organized layout**, start with a single assistant: } ``` +`flattenDepth: 2` says an entry is `/` deep, so anything below it (a skill's `references/` progressive-disclosure directory) installs **inside** the skill instead of beside it. Set it whenever your skills own sub-directories — see [Bounded Flattening](/docs/reference/configuration#bounded-flattening-flattendepth). + This delivers all skills to Claude. When your team adopts other assistants (GitHub Copilot, Cursor, Windsurf), update `targets`: ```json @@ -106,6 +109,7 @@ This delivers all skills to Claude. When your team adopts other assistants (GitH "source": ".skills", "behavior": "mirror", "flatten": true, + "flattenDepth": 2, "prefix": "acme", "description": "Acme team skills", "targets": [ diff --git a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts index b8adc56f5..6d2e07d06 100644 --- a/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts +++ b/packages/content-ops/src/ops/copy/copy-directory-transforms.test.ts @@ -415,6 +415,30 @@ describe('copyDirectoryWithTransforms (via copyPathOps, flatten/prefix)', () => ).rejects.toThrow(/entry too deep|holds none/) }) + // Round-5 Minor on PR #411: the second way out ("give the ancestor files of + // its own") makes THIS rule pass by reclassifying the offender as content — + // which, for a registry whose entries carry an entrypoint file, produces the + // very non-invocable install the same message cites as the reason to reject. + // Consumers of this package have no `skills:conformance` gate to catch the + // follow-on shape, so the advice must stay qualified. + it('qualifies the "give the ancestor files" remedy as content-only (#407)', async () => { + const fileService = createTestFileService({ + '/dataset/source/capability/sub/foo/SKILL.md': '---\nname: foo\n---\n# /foo', + }) + + await expect( + copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', flattenDepth: 2, targets: [] }, + }), + ).rejects.toThrow( + /IF 'capability\/sub\/foo' is meant to be CONTENT of it .* installs as content, not as an entry/s, + ) + }) + // The rule must NOT fire on legitimate content, which is the whole point of // the story: `references` is deeper than flattenDepth too, but its depth-2 // ancestor holds files, so something owns it. diff --git a/packages/content-ops/src/ops/copy/layout-validation.ts b/packages/content-ops/src/ops/copy/layout-validation.ts index 0a01e91cd..baac9b747 100644 --- a/packages/content-ops/src/ops/copy/layout-validation.ts +++ b/packages/content-ops/src/ops/copy/layout-validation.ts @@ -155,7 +155,12 @@ export function validateNoShallowEntryWithSubdir( * `process/review/references/SKILL.md`) is correctly-shaped content here, and * recognising it would need the marker-file knowledge this layer refuses. That * one is caught statically over the dataset corpus by the skills conformance - * gate instead (`skills:conformance`). + * gate instead (`skills:conformance`). A consumer of this package has no such + * gate, so the error message QUALIFIES its second way out ("give the ancestor + * files of its own"): that remedy silences this rule by turning the offender + * into content, which is right for a marker-less registry and wrong for one + * whose entries carry an entrypoint file — say so rather than advise it flatly + * (round-5 review of PR #411). */ export function validateNoDeepEntry( files: string[], @@ -180,7 +185,9 @@ export function validateNoDeepEntry( `${flattenDepth} ('${ancestor}') holds none — so nothing owns it as content and it is an ` + `entry too deep. It would install at a path with no entry root, invisible to the skill ` + `loader, with an unsynced frontmatter name and no skill-name mapping. ` + - `Move it to depth ${flattenDepth}, or give '${ancestor}' files of its own.`, + `Move it to depth ${flattenDepth}, or give '${ancestor}' files of its own IF '${dir}' is ` + + `meant to be CONTENT of it — note that an entrypoint file inside content installs as ` + + `content, not as an entry, i.e. with exactly the symptoms above.`, operation: 'copyDir', path: join(srcPath, dir), }) diff --git a/packages/content-ops/src/ops/naming-transforms.test.ts b/packages/content-ops/src/ops/naming-transforms.test.ts index b83f16360..ea258b87f 100644 --- a/packages/content-ops/src/ops/naming-transforms.test.ts +++ b/packages/content-ops/src/ops/naming-transforms.test.ts @@ -263,7 +263,10 @@ describe('bounded flatten is not less traversal-safe than the unbounded form (#4 // #411 round 4: the "safe by construction" claim covers only multi-segment // paths. `flattenPath('..')` returned '..' verbatim, so the unbounded default // four registries use was the LESS safe of the two once round 3 hardened the - // bounded form. Applied by shape now, so the two branches are symmetric. + // bounded form. Applied by shape now, so neither form is less safe than the + // other — NOT equally strict: see 'still flattens a .. into a hyphen when it + // sits within the entry segments' for the deliberate asymmetry (a preserved + // sub-path is live, a hyphenated one is inert). expect(() => flattenPath('..')).toThrow(/refusing to preserve/) expect(() => flattenPath('.')).toThrow(/refusing to preserve/) expect(() => flattenPath('..', 2)).toThrow(/refusing to preserve/) diff --git a/packages/content-ops/src/ops/naming-transforms.ts b/packages/content-ops/src/ops/naming-transforms.ts index 5c40e6fce..5088ba63d 100644 --- a/packages/content-ops/src/ops/naming-transforms.ts +++ b/packages/content-ops/src/ops/naming-transforms.ts @@ -57,10 +57,17 @@ function assertValidFlattenDepth(flattenDepth: number, operation: string, path: * a separator to hyphenate. A SINGLE segment has none, in either form: * `flattenPath('..')` returned `'..'` unchanged. So the guard is applied by SHAPE, * not by branch (round-4 review of PR #411): every segment of a bounded result - * plus any single-segment result, bounded or not. That keeps the two branches - * symmetric — the bounded form is never LESS safe than the unbounded one it - * replaces (round-3's hole: `flattenPath('../evil', 1)` returned `'../evil'`), and - * never MORE strict than it either. + * plus any single-segment result, bounded or not. + * + * The invariant is "each form is validated exactly where its OWN output could + * carry a live `.`/`..`", which is what makes neither form less safe than the + * other — not equal strictness. The bounded form is deliberately stricter where a + * tail survives: `flattenPath('a/../b', 1)` throws while `flattenPath('a/../b')` + * returns the inert `'a-..-b'`, and a `..` among the JOINED entry segments stays + * unchecked in both (`flattenPath('process/../review', 3)` → `'process-..-review'`). + * Do not "restore symmetry" by weakening the tail check — a preserved sub-path is + * live, a hyphenated one is not; round-3's hole was exactly that + * (`flattenPath('../evil', 1)` returned `'../evil'`). * * Not reachable through the CLI (directory names come from * `dirname(relative(root, path))`, which yields neither `.` nor `..` for a file diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md index 61c80a469..cab2cd208 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -29,9 +29,9 @@ The sub-directory installs **inside** the skill, not as a sibling directory of i - **Relative links keep working in both directions.** `SKILL.md` → `./references/deep-dive.md` and `deep-dive.md` → `../SKILL.md` are both correct in the source tree AND in the installed tree, because the whole directory moves together. Write plain relative links; do not pre-compensate for the install path. (True for a skill at the standard `/` depth — see the exclusions below.) - **A sub-document is content, not a skill.** It is not a separate installed entry, gets no name prefix, and its frontmatter (if any) is left alone. Do not give a sub-document a `name:` matching a skill name, and do not expect a sub-directory name to be invocable. -## The two exclusions: every entry must sit at the declared depth +## The two exclusions: layouts the entry depth cannot represent -The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`) and cannot represent a directory holding files at any other depth. Both sides fail the copy **loudly**, before a single file is written, rather than installing something broken. +The pipeline is told **how deep an entry is** (`flattenDepth` in the registry config — 2 for `/`). A directory holding files at another depth is tolerated while it stays unambiguous — pair's own root-level `next` skill installs fine as `pair-next` — and becomes unrepresentable only in the two shapes below. Both fail the copy **loudly**, before a single file is written, rather than installing something broken. ### Too shallow: a directory above the entry depth that holds files of its own @@ -57,3 +57,4 @@ A `SKILL.md` one level too deep (`///SKILL.md` at `fl 2. **Point at sub-documents with relative links** from `SKILL.md`, at the step that needs them, so the executor follows the pointer when it gets there — the same way it follows pointers to guideline files. 3. **Keep the entrypoint self-sufficient for the happy path.** A sub-document is for depth (long catalogs, worked examples, edge-case tables), not for material the skill needs on every run. 4. **One level is usually enough.** Deeper nesting is preserved too, but it costs the reader a hop for no gain in most cases. +5. **Link a sub-document only from within its own skill.** Link rewriting for content OUTSIDE the skills registry (a knowledge-base guideline, an `AGENTS.md`) maps a skill's **entrypoint** path only — `///SKILL.md` → the installed skill directory. A link from outside to a sub-document path is left pointing at the source tree, which a consuming project does not install: a silently dead link. From outside, link the skill's `SKILL.md` and let it point onward (rule 2); per-file mappings are the follow-up if a direct deep link ever becomes necessary. Links *inside* the copied registry are rebased by the copy itself, so this constrains only the cross-registry direction. From 7db5d1da4a47b1cbac674f41b076dac0c6c53151 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 20:36:10 +0200 Subject: [PATCH 12/12] =?UTF-8?q?[US-407]=20fix:=20round-4=20review=20?= =?UTF-8?q?=E2=80=94=20null=20flattenDepth=20reaches=20validation,=20docs/?= =?UTF-8?q?ADR=20wording?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- ...bounded-flatten-depth-entry-granularity.md | 2 +- apps/pair-cli/src/registry/resolver.test.ts | 19 +++++++++++++++++++ apps/pair-cli/src/registry/resolver.ts | 5 ++++- .../content/docs/reference/configuration.mdx | 2 +- .../src/ops/copy/layout-validation.ts | 6 ++++-- .../src/tools/skill-md-mirror.ts | 8 ++------ 6 files changed, 31 insertions(+), 11 deletions(-) diff --git a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md index caf4af110..f59e29bc8 100644 --- a/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md +++ b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md @@ -67,7 +67,7 @@ Corollary, recorded because it is not obvious: once flatten is bounded, a sub-di - **The number is a duplicated fact.** `.skills/` layout depth is asserted in `apps/pair-cli/config.json` and mirrored in `SKILL_COPY_OPTS`. A pin test ties the two together, but a `.skills/` reorganization must still update the number deliberately. - **Two flatten semantics coexist** in one codebase (bounded for `skills`, unbounded elsewhere). Accepted as the price of not changing registries that have no defect; the regression-witness test documents the unbounded branch as intentional, not vestigial. - Collision detection (`detectCollisions`) now compares sub-paths, not only top-level names. No behaviour change for the current corpus (no dataset skill dir is deeper than two segments) but a future nested-name clash surfaces as a sub-path collision message. -- **Every entry must sit at the declared depth; both deviations abort the copy.** The depth assumes a uniform layout, and `.skills/` already bends it: `next/` is a ONE-segment entry (installed as `pair-next`) beside 39 two-segment ones. The enforced rules, stated as the code enforces them (not only for the case that motivated them): +- **A deviation from the declared depth aborts the copy once the shape becomes ambiguous.** Neither guard fires on depth alone — each needs a second condition, stated per shape below — so "wrong depth" is not by itself an error. The depth assumes a uniform layout, and `.skills/` already bends it: `next/` is a ONE-segment entry (installed as `pair-next`) beside 39 two-segment ones. The enforced rules, stated as the code enforces them (not only for the case that motivated them): - **Too shallow** — *any* directory shallower than `flattenDepth` that holds files directly AND owns a sub-directory. That is broader than "a registry-root skill cannot ship a sub-directory": it equally rejects a **category directory with a file of its own** (a `process/README.md` beside `process/review/`), because entry-vs-category is decided by "holds files directly" — the price of keeping marker-file knowledge (`SKILL.md`) out of a transform four non-skill registries share. The shape is genuinely unrepresentable either way: `next/references` has the same shape as the entry `process/review` and would install as the sibling `pair-next-references/` — the whole defect back. Consequence for authors: giving `next` a sub-directory requires moving it under a category first, which **renames the installed skill users invoke**; a category-level file must move into a skill directory instead. - **Too deep** — a directory deeper than `flattenDepth` holding files whose nearest ancestor at the entry depth holds none, i.e. an entry too deep with nothing owning it as content. It would install under a directory with no entrypoint at its root, invisible to the skill loader. - Both deviations are stated in `nested-sub-documents.md` as the convention's two exclusions, with the way out per shape. The too-deep case is guarded twice: at copy time (above) for a directory nothing owns, and statically over the dataset corpus by the skills conformance gate (`skills:conformance` fails on any `SKILL.md` below the entry depth) — because a `SKILL.md` inside a real skill's `references/` IS legitimately-shaped content for the copy-time guard, and would install silently non-invocable. diff --git a/apps/pair-cli/src/registry/resolver.test.ts b/apps/pair-cli/src/registry/resolver.test.ts index a1d7ef298..5f49caee9 100644 --- a/apps/pair-cli/src/registry/resolver.test.ts +++ b/apps/pair-cli/src/registry/resolver.test.ts @@ -33,6 +33,25 @@ describe('registry resolver', () => { expect(registries['knowledge']?.flattenDepth).toBeUndefined() }) + // An explicit null is a config MISTAKE, not an omission: dropping it here would + // silently restore the unbounded flatten this story removed. Carried through so + // validateFlattenDepthField rejects it by name. + it('carries an explicit null flattenDepth through to validation instead of dropping it', () => { + const config = { + asset_registries: { + skills: { + source: '.skills', + flatten: true, + flattenDepth: null, + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + }, + } + const registries = extractRegistries(config) + expect('flattenDepth' in (registries['skills'] as object)).toBe(true) + expect(registries['skills']?.flattenDepth).toBeNull() + }) + it('resolveTarget correctly resolves relative and absolute targets', () => { const fs = new InMemoryFileSystemService({}, cwd, cwd) const config = { diff --git a/apps/pair-cli/src/registry/resolver.ts b/apps/pair-cli/src/registry/resolver.ts index 11ea7be51..b39d6116a 100644 --- a/apps/pair-cli/src/registry/resolver.ts +++ b/apps/pair-cli/src/registry/resolver.ts @@ -80,7 +80,10 @@ function normalizeRegistryConfig(name: string, raw: Record): Re // Carried through as-is (no coercion): validateFlattenDepthField rejects a // non-positive-integer loudly rather than letting a config typo degrade into // an unbounded flatten, which is exactly the #407 defect. - ...(raw['flattenDepth'] != null && { flattenDepth: raw['flattenDepth'] as number }), + // `!== undefined`, not `!= null`: an explicit `"flattenDepth": null` must reach + // validateFlattenDepthField and be rejected by name, not be silently dropped into + // the unbounded default — a null is a config mistake, not an omission. + ...(raw['flattenDepth'] !== undefined && { flattenDepth: raw['flattenDepth'] as number }), ...(raw['prefix'] != null && { prefix: String(raw['prefix']) }), targets, } diff --git a/apps/website/content/docs/reference/configuration.mdx b/apps/website/content/docs/reference/configuration.mdx index 808a2c3d3..c56873428 100644 --- a/apps/website/content/docs/reference/configuration.mdx +++ b/apps/website/content/docs/reference/configuration.mdx @@ -176,7 +176,7 @@ pair validate-config --config ./custom-config.json - Required fields present (`behavior`, `source`, `description`) - Behavior values are valid (`mirror`, `add`, `overwrite`, `skip`) - Target paths are accessible -- Transform config is valid (non-empty prefix, not combined with symlink mode) +- Transform config is valid (non-empty prefix, not combined with symlink mode; `flattenDepth` a positive integer, and only alongside `flatten: true`) - Source paths exist (if specified) - Include patterns are valid arrays - No registry's `target` overlaps a reserved path — the working area (default or overridden `working_path`) — and `working_path` is project-relative; see [Working Area Override](#working-area-override) diff --git a/packages/content-ops/src/ops/copy/layout-validation.ts b/packages/content-ops/src/ops/copy/layout-validation.ts index baac9b747..160c8e1d2 100644 --- a/packages/content-ops/src/ops/copy/layout-validation.ts +++ b/packages/content-ops/src/ops/copy/layout-validation.ts @@ -155,8 +155,10 @@ export function validateNoShallowEntryWithSubdir( * `process/review/references/SKILL.md`) is correctly-shaped content here, and * recognising it would need the marker-file knowledge this layer refuses. That * one is caught statically over the dataset corpus by the skills conformance - * gate instead (`skills:conformance`). A consumer of this package has no such - * gate, so the error message QUALIFIES its second way out ("give the ancestor + * gate instead (`skills:conformance`). A project whose own `config.json` declares a bounded + * registry has no such gate — `skills:conformance` is corpus-specific to this + * repository, not part of the published package — so the error message QUALIFIES + * its second way out ("give the ancestor * files of its own"): that remedy silences this rule by turning the offender * into content, which is right for a marker-less registry and wrong for one * whose entries carry an entrypoint file — say so rather than advise it flatly diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts index c73699d18..50a29f675 100644 --- a/packages/knowledge-hub/src/tools/skill-md-mirror.ts +++ b/packages/knowledge-hub/src/tools/skill-md-mirror.ts @@ -194,13 +194,9 @@ export function datasetSkillArtifacts(tree: DatasetTree): string[] { * * Composes the REAL `transformPath` over the artifact's dataset directory with * the registry's options — exactly what the copy pipeline's per-file transform - * does (`dirname(file)` → `transformPath` → join the untouched file name). This - * is why a NESTED sub-directory lands in its OWN flattened top-level dir - * (`process/review/references/deep.md` → `pair-process-review-references/deep.md`) - * rather than under a preserved `references/`: the pipeline flattens every - * directory segment, not just the skill's own. + * does (`dirname(file)` → `transformPath` → join the untouched file name). * - * Since #407 the mapping is BOUNDED: `SKILL_COPY_OPTS.flattenDepth` is the + * The mapping is BOUNDED: `SKILL_COPY_OPTS.flattenDepth` is the * registry's entry depth, so a third segment (`references/`) is preserved as a * real sub-path and a sub-doc installs INSIDE its skill * (`process/review/references/deep.md` → `pair-process-review/references/deep.md`),