diff --git a/.claude/skills/pair-process-plan-epics/SKILL.md b/.claude/skills/pair-process-plan-epics/SKILL.md index acc6df30..1c280922 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/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md b/.pair/adoption/tech/adr/adr-005-skills-infrastructure.md index 3ded67ed..d00169a2 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: "overwrite"`, 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 00000000..f59e29bc --- /dev/null +++ b/.pair/adoption/tech/adr/adr-020-bounded-flatten-depth-entry-granularity.md @@ -0,0 +1,86 @@ +# 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. 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 + +**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 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 + +- **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 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. +- 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 + +- **`.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 167e3dd1..0a7606d4 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 00000000..cab2cd20 --- /dev/null +++ b/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -0,0 +1,60 @@ +# 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. (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: layouts the entry depth cannot represent + +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 + +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'. … +``` + +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 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. +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/.pair/llms.txt b/.pair/llms.txt index f31f70c8..fb593777 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/pair-cli/config.json b/apps/pair-cli/config.json index f9d6a072..96e3591c 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 e0f6a467..d732823f 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 e5d09ae1..f628de4b 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 9f73650a..4e0fc926 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 e4a1292b..5f49caee 100644 --- a/apps/pair-cli/src/registry/resolver.test.ts +++ b/apps/pair-cli/src/registry/resolver.test.ts @@ -13,6 +13,45 @@ 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() + }) + + // 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 bcc66747..b39d6116 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,13 @@ 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. + // `!== 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/pair-cli/src/registry/validation.test.ts b/apps/pair-cli/src/registry/validation.test.ts index 32003d93..c2758384 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 f642adef..4c97316b 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' @@ -81,6 +87,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 +145,28 @@ 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`. + * + * 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 (!isValidFlattenDepth(flattenDepth)) { + 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/apps/website/content/docs/reference/configuration.mdx b/apps/website/content/docs/reference/configuration.mdx index c03f47a7..c5687342 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. +- 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 ```json { "skills": { "source": ".skills", - "behavior": "mirror", + "behavior": "overwrite", "flatten": true, + "flattenDepth": 2, "prefix": "pair", "description": "Agent skills distributed to AI tool directories", "targets": [ @@ -156,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/apps/website/content/docs/reference/skill-management.mdx b/apps/website/content/docs/reference/skill-management.mdx index 96512743..e1debacf 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/apps/website/content/docs/tutorials/managing-ai-artifacts.mdx b/apps/website/content/docs/tutorials/managing-ai-artifacts.mdx index c25c0ae3..8b3ae0fd 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/index.ts b/packages/content-ops/src/index.ts index 4119260b..91f39445 100644 --- a/packages/content-ops/src/index.ts +++ b/packages/content-ops/src/index.ts @@ -55,7 +55,15 @@ 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, + isValidFlattenDepth, + type TransformOpts, +} from './ops/naming-transforms' export { rewriteLinksInFile, rewriteLinksAfterTransform, diff --git a/packages/content-ops/src/ops/SyncOptions.ts b/packages/content-ops/src/ops/SyncOptions.ts index 4848f083..97bf9fa5 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 e166960f..6d2e07d0 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,331 @@ 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)') + }) + + // 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/) + }) + + // 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. + 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() + }) + + // 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. + // 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\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', 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') + + 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) + }) + + // #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 + // 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)', () => { @@ -464,6 +789,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 0a630ddc..67b33943 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 } 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,32 +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, - }) - } -} - /** * Copies a single file to its transformed location and tracks the * directory mapping for later link rewriting. @@ -104,6 +83,7 @@ async function copyFileWithTransform(ctx: { targetFilePath, dirMappingFiles, topLevelFiles, + isEntryDir: isRegistryEntryPath(dir, transformOpts.flattenDepth), }) } @@ -112,6 +92,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 +107,7 @@ async function trackTransformedFile(ctx: { targetFilePath: string dirMappingFiles: Map topLevelFiles: Set + isEntryDir: boolean }): Promise { const { fileService, @@ -130,6 +117,7 @@ async function trackTransformedFile(ctx: { targetFilePath, dirMappingFiles, topLevelFiles, + isEntryDir, } = ctx if (dir === '.') { @@ -142,7 +130,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) { @@ -206,16 +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 - return prefix ? { flatten, prefix } : { flatten } + 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 @@ -230,6 +237,8 @@ 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 }) @@ -271,9 +280,25 @@ 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: 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 @@ -291,17 +316,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/copy/copy-types.ts b/packages/content-ops/src/ops/copy/copy-types.ts index 158f8f89..81ea4454 100644 --- a/packages/content-ops/src/ops/copy/copy-types.ts +++ b/packages/content-ops/src/ops/copy/copy-types.ts @@ -10,5 +10,9 @@ export type CopyPathOpsResult = { skillLinkPathMap?: SkillLinkPathMap } -/** Naming transform options (flatten and/or prefix) applied during a copy. */ -export type TransformOpts = { flatten: boolean; prefix?: string } +/** + * 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 } from '../naming-transforms' 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 00000000..160c8e1d --- /dev/null +++ b/packages/content-ops/src/ops/copy/layout-validation.ts @@ -0,0 +1,197 @@ +/** + * 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`). 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 + * (round-5 review of PR #411). + */ +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 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/link-rewriter.ts b/packages/content-ops/src/ops/link-rewriter.ts index 6d8c1620..f312b7b7 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 }> } /** @@ -69,11 +77,16 @@ 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, 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, @@ -93,12 +106,22 @@ type ComputeNewHrefParams = { newFileDir: string datasetRoot?: string sourceContentRoot?: string + movedDirs?: Array<{ originalDir: string; newDir: string }> } /** * 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 @@ -106,10 +129,21 @@ 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) + // 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 if (datasetRoot && sourceContentRoot) { @@ -119,18 +153,52 @@ function resolveAbsoluteTarget(params: { } /** - * 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). + * Rebase a target through whichever moved directory contains it, preferring the + * 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 computeNewHref(params: ComputeNewHrefParams): string | null { - const { href, originalFileDir, newFileDir, datasetRoot, sourceContentRoot } = params - if (isExternalLink(href) || href.startsWith('#')) return null +function rebaseWithinMovedDirs( + absoluteTarget: string, + movedDirs?: Array<{ originalDir: string; newDir: string }>, +): string | null { + if (!movedDirs || movedDirs.length === 0) return null + 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) best = { length: originalDir.length, rebased } + } + return best?.rebased ?? null +} +/** + * 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 } +} + +/** + * 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) + if (!split) return null + const { pathPart, anchorPart } = split const absoluteTarget = resolveAbsoluteTarget({ originalFileDir, @@ -138,6 +206,7 @@ function computeNewHref(params: ComputeNewHrefParams): string | null { pathPart, ...(datasetRoot && { datasetRoot }), ...(sourceContentRoot && { sourceContentRoot }), + ...(movedDirs && { movedDirs }), }) let newRelativePath = posix.relative(newFileDir, absoluteTarget) @@ -205,7 +274,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 +300,7 @@ export async function rewriteLinksInFile(params: RewriteLinksInFileParams): Prom newFileDir, datasetRoot, ...(sourceContentRoot && { sourceContentRoot }), + ...(movedDirs && { movedDirs }), }) if (!newHref) continue @@ -254,6 +325,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 +341,7 @@ export async function rewriteLinksAfterTransform( newDir: entry.newDir, datasetRoot, ...(sourceContentRoot && { sourceContentRoot }), + movedDirs, }) } } diff --git a/packages/content-ops/src/ops/naming-transforms.test.ts b/packages/content-ops/src/ops/naming-transforms.test.ts index 1e27aa28..ea258b87 100644 --- a/packages/content-ops/src/ops/naming-transforms.test.ts +++ b/packages/content-ops/src/ops/naming-transforms.test.ts @@ -1,5 +1,12 @@ import { describe, it, expect } from 'vitest' -import { flattenPath, prefixPath, transformPath, detectCollisions } from './naming-transforms' +import { + flattenPath, + prefixPath, + transformPath, + detectCollisions, + isRegistryEntryPath, + isValidFlattenDepth, +} from './naming-transforms' describe('flattenPath', () => { it('converts nested path separators to hyphens', () => { @@ -101,3 +108,172 @@ 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') + }) + + // 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 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 + // `/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') + }) +}) + +// 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) + }) + + 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)', () => { + 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', + ) + }) +}) + +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('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 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/) + 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 dd76221e..5088ba63 100644 --- a/packages/content-ops/src/ops/naming-transforms.ts +++ b/packages/content-ops/src/ops/naming-transforms.ts @@ -4,15 +4,157 @@ * 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 + * 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 } + +/** + * 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, + }) + } +} + +/** + * A result is joined onto a destination root by the copy pipeline, so a `.`/`..` + * segment surviving in it could escape that root. + * + * 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. + * + * 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 + * 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 === '..') + if (offender !== undefined) { + throw createError({ + type: 'IO_ERROR', + message: + `flattenPath: refusing to preserve the relative segment '${offender}' of '${dirName}' — ` + + 'a bounded result is joined onto the destination root and could escape it.', + operation: 'flattenPath', + path: dirName, + }) + } +} /** * Flatten a path by replacing directory separators with hyphens. * Example: 'catalog/next' → 'catalog-next' + * + * `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 + * 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. + * + * 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): string { +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 '' - 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) return segments.join('-') + + assertNoTraversal(segments, dirName) + const tail = segments.slice(flattenDepth) + 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 + // 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 + return trimmed.split('/').length <= flattenDepth } /** @@ -36,15 +178,15 @@ 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 }, -): string { +export function transformPath(dirName: string, options: Partial): 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 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 e518bb03..e8f63c71 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 e8f241b3..2ceff877 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`) } 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 881434f5..93b72c75 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 00000000..cab2cd20 --- /dev/null +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/technical-standards/ai-development/skill-conventions/nested-sub-documents.md @@ -0,0 +1,60 @@ +# 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. (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: layouts the entry depth cannot represent + +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 + +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'. … +``` + +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 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. +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/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.test.ts index 4a45dd94..b78b714f 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', } @@ -218,17 +221,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 @@ -305,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', ) }) @@ -447,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', @@ -457,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 @@ -471,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. @@ -489,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') }) @@ -548,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') }) }) diff --git a/packages/knowledge-hub/src/tools/skill-md-mirror.ts b/packages/knowledge-hub/src/tools/skill-md-mirror.ts index 39f14347..50a29f67 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`), @@ -24,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 @@ -56,7 +65,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 @@ -185,24 +194,20 @@ 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). * - * 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. + * 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 6e42c47f..aaecbe7b 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 b3889177..1e3141fd 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 { diff --git a/packages/knowledge-hub/src/tools/skills-guide-mirror.ts b/packages/knowledge-hub/src/tools/skills-guide-mirror.ts index 82fda503..09066251 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) } /**