diff --git a/.pair/adoption/decision-log/2026-07-08-test-file-colocation-multi-module.md b/.pair/adoption/decision-log/2026-07-08-test-file-colocation-multi-module.md new file mode 100644 index 00000000..6900f10c --- /dev/null +++ b/.pair/adoption/decision-log/2026-07-08-test-file-colocation-multi-module.md @@ -0,0 +1,40 @@ +# Decision: Test co-location for multi-module tests — root-of-call-chain, not a standalone file + +## Date + +2026-07-08 + +## Status + +Active + +## Category + +Convention Adoption + +## Context + +The existing co-location rule (`file-structure.md`: "Always co-locate test files with implementation") only covers the 1:1 case — one module, one sibling test file. It has no guidance for a test that exercises more than one module together (e.g. an integration test driving two collaborating command handlers end-to-end). Found in #238: `apps/pair-cli/src/commands/update/idempotent-skill-registry.test.ts` tested both `update/handler.ts`'s `handleUpdateCommand` and `install/handler.ts`'s `handleInstallCommand` from a standalone file matching neither, alongside the two directories' own pre-existing `handler.test.ts` siblings. + +## Decision + +A test spanning multiple modules is placed in the test file already co-located with the *root* module of the call chain it verifies — the module whose exported entry point the test is actually asserting on, even if the test also drives other modules as setup or as part of the same flow. No standalone integration-test file is created for the combination. + +Applied to `idempotent-skill-registry.test.ts`: its `update`-focused cases (idempotency across repeated runs, prefix change, removed-skill reference) moved into `update/handler.test.ts` (root: `handleUpdateCommand`); its `install`-only cases (external KB via `--source`, name collision) moved into `install/handler.test.ts` (root: `handleInstallCommand`). The standalone file was deleted. + +This does not apply to end-to-end/page-level tests (named after the user flow or page they exercise, e.g. `landing.e2e.test.ts`) or content/asset-validation tests (named after the asset they validate, e.g. `agents-md.test.ts`, which has no single source module to co-locate against) — both are pre-existing, distinct categories this decision leaves untouched. + +## Alternatives Considered + +- **Keep standalone integration-test files, named after the scenario**: Rejected. Scales into a parallel "integration tests" file tree that duplicates or drifts from the sibling `handler.test.ts` files already covering the same root modules, and breaks the "one root module → one test file" discoverability the co-location rule exists for. +- **One `integration.test.ts` per command directory**: Rejected. Adds a second file per directory with no clearer ownership than the existing `handler.test.ts` — the root-of-call-chain rule gives an unambiguous single destination without adding a new file-naming pattern. + +## Consequences + +- `file-structure.md`'s Co-location Rules section gains an explicit rule for the multi-module case, closing the gap this decision was prompted by. +- Future integration-style tests in this codebase (and any project adopting this KB) have an unambiguous placement rule instead of ad hoc naming per author. + +## Adoption Impact + +- `packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md` (+ root mirror `.pair/knowledge/guidelines/code-design/code-organization/file-structure.md`): Co-location Rules section extended with the multi-module case and its two exceptions. +- `apps/pair-cli/src/commands/update/idempotent-skill-registry.test.ts`: deleted; its cases moved into `update/handler.test.ts` and `install/handler.test.ts`. diff --git a/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md b/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md index e31226f8..eab679d8 100644 --- a/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md +++ b/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md @@ -48,6 +48,10 @@ UserProfile.stories.tsx (if using Storybook) ```text +**Tests spanning multiple modules**: when a test exercises more than one implementation file together (e.g. an integration test driving two collaborating functions end-to-end), it does not get its own standalone file — it goes in the test file already co-located with the *root* module of that call chain, the one whose exported entry point the test is actually verifying. A test that seeds its fixture through module B but asserts on module A's behavior belongs in `A.test.ts`, not a new `A-and-b-integration.test.ts`. This keeps the co-location rule intact (one root module → one test file) instead of a proliferation of ad hoc integration-test files that duplicate coverage or drift out of sync with the modules they actually exercise. + +This does not apply to two other, already-common test categories that are correctly named after what they validate rather than a source module: end-to-end/page-level tests (e.g. `landing.e2e.test.ts`, testing a user flow across many files by design) and content/asset-validation tests (e.g. asserting on a generated markdown file's content, where there is no single source module to co-locate against). + **Types**: Co-locate types when feature-specific: ``` diff --git a/apps/pair-cli/src/commands/install/handler.test.ts b/apps/pair-cli/src/commands/install/handler.test.ts index 0666f933..8c1328ac 100644 --- a/apps/pair-cli/src/commands/install/handler.test.ts +++ b/apps/pair-cli/src/commands/install/handler.test.ts @@ -775,3 +775,102 @@ describe('install — Bug 5: skill ref rewrite with agents-before-skills order', expect(agentsContent).not.toMatch(/(? { + const moduleDir = '/project' + const datasetSrc = `${moduleDir}/packages/knowledge-hub/dataset` + + test('AC5: external KB installed via local --source path applies the same flatten/prefix/rewrite pipeline', async () => { + const externalKbPath = '/external/kb' + const fs = new InMemoryFileSystemService( + { + [`${moduleDir}/package.json`]: JSON.stringify({ + name: 'test-project', + version: '0.1.0', + }), + [`${moduleDir}/config.json`]: JSON.stringify({ + asset_registries: { + skills: { + source: '.skills', + behavior: 'mirror', + flatten: true, + prefix: 'ext', + description: 'Agent skills', + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + agents: { + source: 'AGENTS.md', + behavior: 'mirror', + description: 'AI agents guidance', + targets: [{ path: 'AGENTS.md', mode: 'canonical' }], + }, + }, + }), + // Minimal nested external KB — standard `.skills///` layout + [`${externalKbPath}/AGENTS.md`]: '# AGENTS\n\nRun /next for the external KB.\n', + [`${externalKbPath}/.skills/catalog/next/SKILL.md`]: '---\nname: next\n---\n# /next\n', + }, + moduleDir, + moduleDir, + ) + + const installConfig: InstallCommandConfig = { + command: 'install', + resolution: 'local', + path: externalKbPath, + offline: true, + kb: true, + } + await handleInstallCommand(installConfig, fs) + + // Flattened + prefixed exactly like the official dataset pipeline + await expect( + fs.exists(`${moduleDir}/.claude/skills/ext-catalog-next/SKILL.md`), + ).resolves.toBe(true) + const skillContent = await fs.readFile(`${moduleDir}/.claude/skills/ext-catalog-next/SKILL.md`) + expect(skillContent).toContain('name: ext-catalog-next') + + // Cross-registry rewrite applied without any source-side restructuring + const agentsContent = await fs.readFile(`${moduleDir}/AGENTS.md`) + expect(agentsContent).toContain('/ext-catalog-next') + }) + + test('name collision after flattening fails install with an explicit error', async () => { + const fs = new InMemoryFileSystemService( + { + [`${moduleDir}/package.json`]: JSON.stringify({ + name: 'test-project', + version: '0.1.0', + }), + [`${moduleDir}/packages/knowledge-hub/package.json`]: JSON.stringify({ + name: '@pair/knowledge-hub', + }), + [`${moduleDir}/config.json`]: JSON.stringify({ + asset_registries: { + skills: { + source: '.skills', + behavior: 'mirror', + flatten: true, + description: 'Agent skills', + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + }, + }), + // Two distinct source paths that flatten to the same target name + [`${datasetSrc}/.skills/a/b/SKILL.md`]: '# Skill 1', + [`${datasetSrc}/.skills/a-b/SKILL.md`]: '# Skill 2', + }, + moduleDir, + moduleDir, + ) + + const installConfig: InstallCommandConfig = { + command: 'install', + resolution: 'default', + kb: true, + offline: false, + } + + await expect(handleInstallCommand(installConfig, fs)).rejects.toThrow(/collision/i) + }) +}) diff --git a/apps/pair-cli/src/commands/install/handler.ts b/apps/pair-cli/src/commands/install/handler.ts index e36528db..15a593bb 100644 --- a/apps/pair-cli/src/commands/install/handler.ts +++ b/apps/pair-cli/src/commands/install/handler.ts @@ -14,7 +14,7 @@ import { doCopyAndUpdateLinks, buildCopyOptions, postCopyOps, - applySkillRefsToNonSkillRegistries, + reconcileSkillNameRegistry, resolveEffectiveDatasetRoot, writeProjectLlmsTxt, type RegistryConfig, @@ -273,9 +273,8 @@ async function executeInstall(context: InstallContext): Promise { const { results, skillNameMap } = await installAllRegistries(context) - if (skillNameMap.size > 0) { - await applySkillRefsToNonSkillRegistries({ fs, baseTarget, pushLog }, registries, skillNameMap) - } + await reconcileSkillNameRegistry({ fs, baseTarget, pushLog }, registries, skillNameMap) + if (options?.linkStyle) { await applyLinkTransformation(fs, { linkStyle: options.linkStyle }, pushLog, 'install') } diff --git a/apps/pair-cli/src/commands/update/handler.test.ts b/apps/pair-cli/src/commands/update/handler.test.ts index a124820d..2a964faf 100644 --- a/apps/pair-cli/src/commands/update/handler.test.ts +++ b/apps/pair-cli/src/commands/update/handler.test.ts @@ -1,6 +1,8 @@ import { describe, expect, beforeEach, vi, test } from 'vitest' import { handleUpdateCommand } from './handler' import type { UpdateCommandConfig } from './parser' +import { handleInstallCommand } from '../install/handler' +import type { InstallCommandConfig } from '../install/parser' import { InMemoryFileSystemService, MockHttpClientService } from '@pair/content-ops' describe('handleUpdateCommand - integration with in-memory services', () => { @@ -949,3 +951,239 @@ describe('Bug 5: skill ref rewrite with agents-before-skills config order', () = expect(agentsContent).not.toMatch(/(? { + const moduleDir = '/project' + const datasetSrc = `${moduleDir}/packages/knowledge-hub/dataset` + + function skillsAgentsConfig(prefix: string) { + return { + asset_registries: { + skills: { + source: '.skills', + behavior: 'mirror', + flatten: true, + prefix, + description: 'Agent skills', + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + agents: { + source: 'AGENTS.md', + behavior: 'mirror', + description: 'AI agents guidance', + targets: [{ path: 'AGENTS.md', mode: 'canonical' }], + }, + }, + } + } + + function seedFs(prefix: string) { + return new InMemoryFileSystemService( + { + [`${moduleDir}/package.json`]: JSON.stringify({ + name: 'test-project', + version: '0.1.0', + }), + [`${moduleDir}/packages/knowledge-hub/package.json`]: JSON.stringify({ + name: '@pair/knowledge-hub', + }), + [`${moduleDir}/config.json`]: JSON.stringify(skillsAgentsConfig(prefix)), + [`${datasetSrc}/.skills/process/next/SKILL.md`]: + '---\nname: next\n---\n# /next\n\nComposes /verify-quality.\n', + [`${datasetSrc}/.skills/capability/verify-quality/SKILL.md`]: + '---\nname: verify-quality\n---\n# /verify-quality\n', + [`${datasetSrc}/AGENTS.md`]: '# AGENTS\n\nRun /next to get started.\n', + }, + moduleDir, + moduleDir, + ) + } + + test('AC4: install -> update -> update produces byte-identical skill, AGENTS.md and manifest content', async () => { + const fs = seedFs('pair') + const httpClient = new MockHttpClientService() + + const installConfig: InstallCommandConfig = { + command: 'install', + resolution: 'default', + kb: true, + offline: false, + } + await handleInstallCommand(installConfig, fs, { httpClient }) + + const skillPath = `${moduleDir}/.claude/skills/pair-process-next/SKILL.md` + const agentsPath = `${moduleDir}/AGENTS.md` + const manifestPath = `${moduleDir}/.pair/.skill-name-map.json` + + const afterInstall = { + skill: await fs.readFile(skillPath), + agents: await fs.readFile(agentsPath), + manifest: await fs.readFile(manifestPath), + } + + // Sanity: rewriting actually happened + expect(afterInstall.agents).toContain('/pair-process-next') + expect(afterInstall.manifest).toContain('pair-process-next') + + const updateConfig: UpdateCommandConfig = { + command: 'update', + resolution: 'default', + kb: true, + offline: false, + } + + await handleUpdateCommand(updateConfig, fs, { httpClient, persistBackup: false }) + const afterUpdate1 = { + skill: await fs.readFile(skillPath), + agents: await fs.readFile(agentsPath), + manifest: await fs.readFile(manifestPath), + } + expect(afterUpdate1).toEqual(afterInstall) + + await handleUpdateCommand(updateConfig, fs, { httpClient, persistBackup: false }) + const afterUpdate2 = { + skill: await fs.readFile(skillPath), + agents: await fs.readFile(agentsPath), + manifest: await fs.readFile(manifestPath), + } + expect(afterUpdate2).toEqual(afterUpdate1) + }) + + test('AC4 edge case: prefix change removes the old flattened dir and rewrites already-installed references recorded in the manifest', async () => { + // Simulates the state left behind by a previous install/update run with + // prefix "pair": the manifest, the flattened skill dir, and a stale + // reference baked into an `add`-behavior adoption doc that is never + // re-derived from source (so it can only be fixed via the recorded + // mapping, not by re-copying). + const fs = new InMemoryFileSystemService( + { + [`${moduleDir}/package.json`]: JSON.stringify({ + name: 'test-project', + version: '0.1.0', + }), + [`${moduleDir}/packages/knowledge-hub/package.json`]: JSON.stringify({ + name: '@pair/knowledge-hub', + }), + [`${moduleDir}/config.json`]: JSON.stringify({ + asset_registries: { + skills: { + source: '.skills', + behavior: 'mirror', + flatten: true, + prefix: 'foo', // prefix changed from "pair" to "foo" + description: 'Agent skills', + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + adoption: { + source: 'adoption', + behavior: 'add', // never re-copied once a file exists at target + description: 'Adoption doc', + targets: [{ path: '.pair/adoption/', mode: 'canonical' }], + }, + }, + }), + [`${datasetSrc}/.skills/process/next/SKILL.md`]: '---\nname: next\n---\n# /next\n', + [`${datasetSrc}/adoption/ADOPTED.md`]: + '# fresh adoption doc (never used — target pre-exists)\n', + // Previous run's leftovers: + [`${moduleDir}/.pair/.skill-name-map.json`]: JSON.stringify({ + version: 1, + skills: { next: 'pair-process-next' }, + }), + [`${moduleDir}/.claude/skills/pair-process-next/SKILL.md`]: + '---\nname: pair-process-next\n---\n# /pair-process-next\n', + [`${moduleDir}/.pair/adoption/ADOPTED.md`]: 'Use /pair-process-next for planning.\n', + }, + moduleDir, + moduleDir, + ) + + const updateConfig: UpdateCommandConfig = { + command: 'update', + resolution: 'default', + kb: true, + offline: false, + } + await handleUpdateCommand(updateConfig, fs, { persistBackup: false }) + + // Old prefixed directory is gone, new one exists + await expect( + fs.exists(`${moduleDir}/.claude/skills/pair-process-next/SKILL.md`), + ).resolves.toBe(false) + await expect(fs.exists(`${moduleDir}/.claude/skills/foo-process-next/SKILL.md`)).resolves.toBe( + true, + ) + + // The `add`-behavior file was never re-copied, but its stale reference + // is rewritten via the recorded previous mapping. + const adoptionContent = await fs.readFile(`${moduleDir}/.pair/adoption/ADOPTED.md`) + expect(adoptionContent).toBe('Use /foo-process-next for planning.\n') + + // Manifest reflects the new mapping for the next run + const manifest = JSON.parse(await fs.readFile(`${moduleDir}/.pair/.skill-name-map.json`)) + expect(manifest.skills.next).toBe('foo-process-next') + }) + + test('AC4 edge case: a reference to a removed skill is left as-is (not rewritten, not deleted)', async () => { + const fs = new InMemoryFileSystemService( + { + [`${moduleDir}/package.json`]: JSON.stringify({ + name: 'test-project', + version: '0.1.0', + }), + [`${moduleDir}/packages/knowledge-hub/package.json`]: JSON.stringify({ + name: '@pair/knowledge-hub', + }), + [`${moduleDir}/config.json`]: JSON.stringify({ + asset_registries: { + skills: { + source: '.skills', + behavior: 'mirror', + flatten: true, + prefix: 'pair', + description: 'Agent skills', + targets: [{ path: '.claude/skills/', mode: 'canonical' }], + }, + adoption: { + source: 'adoption', + behavior: 'add', + description: 'Adoption doc', + targets: [{ path: '.pair/adoption/', mode: 'canonical' }], + }, + }, + }), + // "oldskill" no longer exists in the source dataset — only "next" remains + [`${datasetSrc}/.skills/process/next/SKILL.md`]: '---\nname: next\n---\n# /next\n', + [`${datasetSrc}/adoption/ADOPTED.md`]: + '# fresh adoption doc (never used — target pre-exists)\n', + [`${moduleDir}/.pair/.skill-name-map.json`]: JSON.stringify({ + version: 1, + skills: { next: 'pair-process-next', oldskill: 'pair-capability-oldskill' }, + }), + [`${moduleDir}/.claude/skills/pair-process-next/SKILL.md`]: + '---\nname: pair-process-next\n---\n# /pair-process-next\n', + [`${moduleDir}/.pair/adoption/ADOPTED.md`]: + 'Use /pair-capability-oldskill for the legacy flow.\n', + }, + moduleDir, + moduleDir, + ) + + const updateConfig: UpdateCommandConfig = { + command: 'update', + resolution: 'default', + kb: true, + offline: false, + } + await handleUpdateCommand(updateConfig, fs, { persistBackup: false }) + + // Left exactly as-is — there is no correct new name to rewrite it to + const adoptionContent = await fs.readFile(`${moduleDir}/.pair/adoption/ADOPTED.md`) + expect(adoptionContent).toBe('Use /pair-capability-oldskill for the legacy flow.\n') + + // The removed skill's own installed dir is gone (mirror cleanup) + await expect( + fs.exists(`${moduleDir}/.claude/skills/pair-capability-oldskill/SKILL.md`), + ).resolves.toBe(false) + }) +}) diff --git a/apps/pair-cli/src/commands/update/handler.ts b/apps/pair-cli/src/commands/update/handler.ts index 61c5c6cc..6c79e851 100644 --- a/apps/pair-cli/src/commands/update/handler.ts +++ b/apps/pair-cli/src/commands/update/handler.ts @@ -12,7 +12,7 @@ import { doCopyAndUpdateLinks, buildCopyOptions, postCopyOps, - applySkillRefsToNonSkillRegistries, + reconcileSkillNameRegistry, handleBackupRollback, resolveEffectiveDatasetRoot, writeProjectLlmsTxt, @@ -269,13 +269,11 @@ async function updateRegistries(context: UpdateContext): Promise 0) { - await applySkillRefsToNonSkillRegistries( - { fs, baseTarget, pushLog }, - registries, - accumulatedSkillNameMap, - ) - } + await reconcileSkillNameRegistry( + { fs, baseTarget, pushLog }, + registries, + accumulatedSkillNameMap, + ) presenter.summary(results, 'update', Date.now() - startTime) return results diff --git a/apps/pair-cli/src/registry/skill-refs.test.ts b/apps/pair-cli/src/registry/skill-refs.test.ts index 35093110..cef4b8da 100644 --- a/apps/pair-cli/src/registry/skill-refs.test.ts +++ b/apps/pair-cli/src/registry/skill-refs.test.ts @@ -1,7 +1,13 @@ import { describe, it, expect } from 'vitest' import { InMemoryFileSystemService } from '@pair/content-ops' import type { SkillNameMap } from '@pair/content-ops' -import { rewriteSkillRefsInTarget, applySkillRefsToNonSkillRegistries } from './skill-refs' +import { + rewriteSkillRefsInTarget, + applySkillRefsToNonSkillRegistries, + detectOrphanedSkillReferences, + resolveSkillNameManifestPath, + reconcileSkillNameRegistry, +} from './skill-refs' import type { RegistryConfig } from './resolver' describe('rewriteSkillRefsInTarget', () => { @@ -208,3 +214,187 @@ describe('applySkillRefsToNonSkillRegistries', () => { expect(result).toBe('# AGENTS\n\nNo skills.') }) }) + +describe('resolveSkillNameManifestPath', () => { + it('resolves under .pair/ relative to baseTarget', () => { + const fs = new InMemoryFileSystemService({}, '/project', '/project') + expect(resolveSkillNameManifestPath(fs, '/project')).toBe( + '/project/.pair/.skill-name-map.json', + ) + }) + + it('is outside the knowledge and adoption registry targets', () => { + const fs = new InMemoryFileSystemService({}, '/project', '/project') + const manifestPath = resolveSkillNameManifestPath(fs, '/project') + expect(manifestPath.startsWith('/project/.pair/knowledge')).toBe(false) + expect(manifestPath.startsWith('/project/.pair/adoption')).toBe(false) + }) +}) + +describe('detectOrphanedSkillReferences', () => { + const noopLog = () => {} + + it('does nothing when there are no orphaned names', async () => { + const fs = new InMemoryFileSystemService({ '/project/AGENTS.md': '# AGENTS' }, '/project', '/project') + const registries: Record = { + agents: { + source: 'AGENTS.md', + behavior: 'mirror', + description: 'Agents', + include: [], + flatten: false, + targets: [{ path: 'AGENTS.md', mode: 'canonical' }], + }, + } + + await detectOrphanedSkillReferences({ fs, baseTarget: '/project', pushLog: noopLog }, registries, []) + + // no-op — nothing to assert beyond "did not throw" + }) + + it('warns when an orphaned installed name is still referenced', async () => { + const fs = new InMemoryFileSystemService( + { '/project/AGENTS.md': '# AGENTS\n\nRun /pair-removed for legacy setup.' }, + '/project', + '/project', + ) + const registries: Record = { + agents: { + source: 'AGENTS.md', + behavior: 'mirror', + description: 'Agents', + include: [], + flatten: false, + targets: [{ path: 'AGENTS.md', mode: 'canonical' }], + }, + } + + const warnings: string[] = [] + const pushLog = (level: string, message: string) => { + if (level === 'warn') warnings.push(message) + } + + await detectOrphanedSkillReferences( + { fs, baseTarget: '/project', pushLog }, + registries, + ['pair-removed'], + ) + + expect(warnings).toHaveLength(1) + expect(warnings[0]).toContain('/pair-removed') + expect(warnings[0]).toContain('AGENTS.md') + + // content is left untouched + const content = await fs.readFile('/project/AGENTS.md') + expect(content).toBe('# AGENTS\n\nRun /pair-removed for legacy setup.') + }) + + it('does not warn when the orphaned name only appears in a fenced code block', async () => { + const fs = new InMemoryFileSystemService( + { '/project/AGENTS.md': ['# AGENTS', '```', '/pair-removed', '```'].join('\n') }, + '/project', + '/project', + ) + const registries: Record = { + agents: { + source: 'AGENTS.md', + behavior: 'mirror', + description: 'Agents', + include: [], + flatten: false, + targets: [{ path: 'AGENTS.md', mode: 'canonical' }], + }, + } + + const warnings: string[] = [] + const pushLog = (level: string, message: string) => { + if (level === 'warn') warnings.push(message) + } + + await detectOrphanedSkillReferences( + { fs, baseTarget: '/project', pushLog }, + registries, + ['pair-removed'], + ) + + expect(warnings).toHaveLength(0) + }) + + it('does not throw when target does not exist', async () => { + const fs = new InMemoryFileSystemService({}, '/project', '/project') + const registries: Record = { + agents: { + source: 'AGENTS.md', + behavior: 'mirror', + description: 'Agents', + include: [], + flatten: false, + targets: [{ path: 'AGENTS.md', mode: 'canonical' }], + }, + } + + await expect( + detectOrphanedSkillReferences( + { fs, baseTarget: '/project', pushLog: noopLog }, + registries, + ['pair-removed'], + ), + ).resolves.not.toThrow() + }) +}) + +describe('reconcileSkillNameRegistry', () => { + // #238 finding: when this run's skillNameMap is empty — not just "no + // manifest yet" but also "flatten/prefix was disabled for every registry + // this run" — reconcile is a full no-op (no orphan warnings, no rewrite, + // no manifest write), even though a previous manifest exists. + // + // This is intentional, not an oversight: with an empty current map we + // cannot tell "skill removed from the registry" apart from "skill still + // present but now installed under its unprefixed name" (the latter never + // gets a skillNameMap entry when transforms are off — see + // `hasNamingTransforms` in copyPathOps.ts). Attempting orphan detection + // from the stale manifest alone would flag every previously-renamed skill + // as "removed", which is actively misleading for the common case where it + // is still installed, just unprefixed. No-op is the safer failure mode. + it('is a no-op when the current run has no renames, even with a stale manifest', async () => { + const previousManifest = JSON.stringify({ version: 1, skills: { next: 'pair-process-next' } }) + const fs = new InMemoryFileSystemService( + { + '/project/AGENTS.md': '# AGENTS\n\nRun /pair-process-next to get started.\n', + '/project/.pair/.skill-name-map.json': previousManifest, + }, + '/project', + '/project', + ) + + const registries: Record = { + agents: { + source: 'AGENTS.md', + behavior: 'mirror', + description: 'Agents', + include: [], + flatten: false, + targets: [{ path: 'AGENTS.md', mode: 'canonical' }], + }, + } + + const warnings: string[] = [] + const pushLog = (level: string, message: string) => { + if (level === 'warn') warnings.push(message) + } + + await reconcileSkillNameRegistry({ fs, baseTarget: '/project', pushLog }, registries, new Map()) + + // No orphan warning — "pair-process-next" is not misreported as removed. + expect(warnings).toHaveLength(0) + + // Non-skill registry content is left untouched (no rewrite attempted). + const agents = await fs.readFile('/project/AGENTS.md') + expect(agents).toBe('# AGENTS\n\nRun /pair-process-next to get started.\n') + + // The stale manifest is left as recorded, not overwritten with an empty map. + const manifest = await fs.readFile('/project/.pair/.skill-name-map.json') + expect(manifest).toBe(previousManifest) + }) +}) diff --git a/apps/pair-cli/src/registry/skill-refs.ts b/apps/pair-cli/src/registry/skill-refs.ts index 6046d74e..863f61e7 100644 --- a/apps/pair-cli/src/registry/skill-refs.ts +++ b/apps/pair-cli/src/registry/skill-refs.ts @@ -1,9 +1,30 @@ import type { FileSystemService, SkillNameMap } from '@pair/content-ops' -import { rewriteSkillReferences, walkMarkdownFiles } from '@pair/content-ops' +import { + rewriteSkillReferences, + findSkillReferences, + walkMarkdownFiles, + readSkillNameManifest, + writeSkillNameManifest, + buildTransitionMap, + findOrphanedInstalledNames, + mergeSkillNameMaps, +} from '@pair/content-ops' import type { LogEntry } from '#diagnostics' import type { RegistryConfig } from './resolver' import { getNonSymlinkTargets } from './layout' +/** + * Path of the CLI-internal manifest that records the skill name map from + * the last install/update run. Deliberately outside every registry's + * target scope (`.pair/knowledge`, `.pair/adoption`, `.skills` targets) + * so it is never touched by mirror cleanup or content diffing. + */ +export function resolveSkillNameManifestPath(fs: FileSystemService, baseTarget: string): string { + return baseTarget + ? fs.resolve(baseTarget, '.pair', '.skill-name-map.json') + : fs.resolve('.pair', '.skill-name-map.json') +} + /** Minimal context for skill reference rewrite operations. */ export type SkillRefContext = { fs: FileSystemService @@ -64,3 +85,98 @@ export async function applySkillRefsToNonSkillRegistries( } } } + +/** + * Warns when a skill invocation still references an installed name that no + * longer has a matching entry in the registry (the skill was removed or + * disabled between runs). Such references are intentionally left as-is — + * there is no correct new name to rewrite them to — so this only reports, + * it never modifies content. + */ +export async function detectOrphanedSkillReferences( + context: SkillRefContext, + registries: Record, + orphanedInstalledNames: string[], +): Promise { + if (orphanedInstalledNames.length === 0) return + + const { fs, baseTarget, pushLog } = context + + for (const [, config] of Object.entries(registries)) { + for (const targetCfg of getNonSymlinkTargets(config)) { + const target = baseTarget + ? fs.resolve(baseTarget, targetCfg.path) + : fs.resolve(targetCfg.path) + if (!(await fs.exists(target))) continue + + const stat = await fs.stat(target) + const files: string[] = stat.isDirectory() + ? await walkMarkdownFiles(target, fs) + : target.endsWith('.md') + ? [target] + : [] + + for (const filePath of files) { + const content = await fs.readFile(filePath) + const found = findSkillReferences(content, orphanedInstalledNames) + for (const name of found) { + pushLog( + 'warn', + `Skill reference rewriter: /${name} invoked in ${filePath} is no longer in the skill registry (removed or disabled) — left as-is`, + ) + } + } + } + } +} + +/** + * Reconciles this run's skill name map against the previously recorded one + * (see `resolveSkillNameManifestPath`), then rewrites references and warns + * about orphaned ones, and finally records the new mapping for next time. + * + * This is what makes cross-reference rewriting idempotent across a prefix + * (or flatten) change: an already-installed reference like `/pair-next` + * that lives in a file never re-derived from source (e.g. an `add`-behavior + * adoption doc) still gets rewritten to `/foo-next`, because the previous + * install/update's mapping — not a guess from the current config — tells us + * `pair-next` used to mean `next`. + * + * No-op (including no manifest write) when this run produced no renames — + * e.g. flatten/prefix disabled — so a stale manifest from an earlier, + * different configuration is left untouched rather than misinterpreted. + */ +export async function reconcileSkillNameRegistry( + context: SkillRefContext, + registries: Record, + skillNameMap: SkillNameMap, +): Promise { + // Covers both "no manifest yet" and "flatten/prefix disabled for every + // registry this run" (accumulated map empty either way — see + // `hasNamingTransforms` in copyPathOps.ts, which skips building a + // skillNameMap entirely when no transform is active). Intentionally a + // full no-op in both cases: with an empty current map we cannot tell + // "skill removed from the registry" apart from "skill still installed, + // just no longer prefixed" (the latter never produces a map entry), so + // even attempting orphan detection from the stale manifest alone would + // misreport still-installed skills as removed. See + // skill-refs.test.ts > reconcileSkillNameRegistry for the locked-in case. + if (skillNameMap.size === 0) return + + const { fs, baseTarget } = context + const manifestPath = resolveSkillNameManifestPath(fs, baseTarget) + + const previousMap = await readSkillNameManifest(fs, manifestPath) + const transitionMap = buildTransitionMap(previousMap, skillNameMap) + const orphanedNames = findOrphanedInstalledNames(previousMap, skillNameMap) + + const combinedMap = mergeSkillNameMaps(skillNameMap, transitionMap) + await applySkillRefsToNonSkillRegistries(context, registries, combinedMap) + await detectOrphanedSkillReferences(context, registries, orphanedNames) + + // Not atomic with the two passes above: if either throws partway through, the + // manifest is left un-updated for a run that partially applied its rewrites. + // Low impact by design — rewriting is idempotent, so the next successful run + // re-diffs against the still-valid (if stale) previous map and self-corrects. + await writeSkillNameManifest(fs, manifestPath, skillNameMap) +} diff --git a/apps/website/content/docs/reference/skill-management.mdx b/apps/website/content/docs/reference/skill-management.mdx index b6cbb386..9dcda3a3 100644 --- a/apps/website/content/docs/reference/skill-management.mdx +++ b/apps/website/content/docs/reference/skill-management.mdx @@ -154,6 +154,10 @@ With the default config (`prefix: "pair"`, `flatten: true`): | `capability/verify-quality/` | `pair-capability-verify-quality/` | `/pair-capability-verify-quality` | | `capability/record-decision/` | `pair-capability-record-decision/` | `/pair-capability-record-decision` | +### Collision Detection + +If two different source paths flatten to the same installed name (e.g. `a/b/` and `a-b/` both becoming `a-b/`), install fails outright with an explicit error listing the collision instead of silently overwriting one skill with the other. + ### Three Namespace Prefixes | Prefix | Count | Purpose | @@ -202,6 +206,46 @@ Skill references (slash commands in markdown) are rewritten using boundary-aware | `name: pair-next` (no leading slash) | `name: pair-next` (unchanged) | | `path/next/page` (mid-path) | `path/next/page` (unchanged) | +### Fenced Code Blocks Are Skipped + +Rewriting is applied line-by-line and tracks fenced code blocks (` ``` ` or `~~~`). Content inside a fence is left untouched — a skill name quoted as example text isn't a live invocation. Inline single-backtick code spans (`` `/next` ``) are still rewritten; only triple-backtick/tilde *blocks* are excluded. + +```text +Run `/next` to start. <- rewritten (inline code span) +[fenced code block containing "/next"] <- left as-is +Then run /implement. <- rewritten (prose) +``` + +The relative-link rewriter (Stage 3) gets this for free: it walks a real markdown AST, so links inside fenced or inline code were never extracted as links in the first place. + +## Idempotent Updates and Prefix Changes + +### The Problem + +`pair update` re-derives skill bodies from source on every run, so a rename (e.g. a prefix change in `pair.config.json`) always produces the correct *new* skill files. But two kinds of content are **not** re-derived from source every run: + +- Files copied with `add` behavior (e.g. adoption docs) — never overwritten once they exist, so a stale reference baked in during an earlier install (`/pair-next`) is never revisited by a plain copy-and-rewrite pass. +- Anything a previous run wrote using the *old* prefix, where the current config only knows the *new* one. + +Guessing at the old prefix by string-matching (`"pair-"` → `"foo-"`) is explicitly avoided — it produces false positives on unrelated content that happens to start with the same string. + +### The Fix: A Recorded Mapping + +Each install/update writes a small manifest (`.pair/.skill-name-map.json`) recording this run's `{shortName: installedName}` map. The next run reads it back and computes an exact transition map (`oldInstalledName -> newInstalledName`) for every skill whose installed name changed — then applies that transition on top of the normal short-name rewrite when reconciling non-skill registries. The manifest is then rewritten with the current mapping. + +This is why `install -> update -> update` is byte-stable, and why changing `prefix` between two runs correctly: + +- removes the old flattened directory (e.g. `pair-catalog-next/`) instead of leaving it orphaned alongside the new one, +- rewrites already-installed references — including ones living in `add`-behavior files — to the new name. + +### Removed or Disabled Skills + +If a skill disappears from the source registry between runs, its old installed name has no valid new target. Any remaining `/oldName` invocation is left exactly as-is, and the install/update report logs a warning so it can be cleaned up by hand. + +### External KBs + +The same registry-build, rewrite, and manifest pipeline runs regardless of where the dataset comes from — the official KB, a project override, or an external KB installed via `--source`. No source-side restructuring is required or supported; the nested `.skills///` layout is flattened, prefixed, and rewritten identically either way. + ## Windows Compatibility On Windows, symlink targets fall back to `copy` mode because symlinks are not reliably supported. This means secondary targets (`.github/skills/`, `.cursor/skills/`) get independent copies instead of symlinks. diff --git a/packages/content-ops/src/index.ts b/packages/content-ops/src/index.ts index 6b535a08..e2444179 100644 --- a/packages/content-ops/src/index.ts +++ b/packages/content-ops/src/index.ts @@ -55,8 +55,17 @@ export { syncFrontmatter } from './ops/frontmatter-transform' export { rewriteSkillReferences, buildSkillNameMap, + findSkillReferences, type SkillNameMap, } from './ops/skill-reference-rewriter' +export { + readSkillNameManifest, + writeSkillNameManifest, + buildTransitionMap, + findOrphanedInstalledNames, + mergeSkillNameMaps, + type SkillNameManifest, +} from './ops/skill-name-manifest' export { validatePathOps } from './ops/validatePathOps' export { processFilesWithLinkReplacements, diff --git a/packages/content-ops/src/ops/copyPathOps.test.ts b/packages/content-ops/src/ops/copyPathOps.test.ts index 7adc52a7..5e889383 100644 --- a/packages/content-ops/src/ops/copyPathOps.test.ts +++ b/packages/content-ops/src/ops/copyPathOps.test.ts @@ -499,6 +499,119 @@ describe('copyPathOps - flatten and prefix', () => { }), ).rejects.toThrow(/collision/i) }) + + describe('mirror behavior — idempotent updates (AC4)', () => { + it('removes a stale flattened directory when its source skill is gone', async () => { + const fileService = new InMemoryFileSystemService( + { + '/dataset/source/catalog/next/SKILL.md': '---\nname: next\n---\n# /next', + // Stale leftover from a previous run — no longer present under source + '/dataset/target/pair-catalog-removed/SKILL.md': '---\nname: pair-catalog-removed\n---', + }, + '/', + '/', + ) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', defaultBehavior: 'mirror', targets: [] }, + }) + + await expect( + fileService.exists('/dataset/target/pair-catalog-removed/SKILL.md'), + ).resolves.toBe(false) + await expect(fileService.exists('/dataset/target/pair-catalog-next/SKILL.md')).resolves.toBe( + true, + ) + }) + + it('removes the old prefixed directory after a prefix change', async () => { + const fileService = new InMemoryFileSystemService( + { + '/dataset/source/catalog/next/SKILL.md': '---\nname: next\n---\n# /next', + // Leftover from a previous install with prefix "pair" + '/dataset/target/pair-catalog-next/SKILL.md': '---\nname: pair-catalog-next\n---', + }, + '/', + '/', + ) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'foo', defaultBehavior: 'mirror', targets: [] }, + }) + + await expect(fileService.exists('/dataset/target/pair-catalog-next/SKILL.md')).resolves.toBe( + false, + ) + await expect(fileService.exists('/dataset/target/foo-catalog-next/SKILL.md')).resolves.toBe( + true, + ) + }) + + it('does not delete a root-level (non-nested) source file on a second mirror run', async () => { + // Regression: cleanupStaleTransformedEntries built its "expected" set only from + // dirMappingFiles, which copyFileWithTransform only populates for files under a + // subdirectory (dir !== '.'). A file copied directly from the source root was never + // registered as expected, so a second mirror run would delete it as "stale". + const fileService = new InMemoryFileSystemService( + { + '/dataset/source/README.md': '# Root-level file, no subdirectory', + '/dataset/source/catalog/next/SKILL.md': '---\nname: next\n---\n# /next', + }, + '/', + '/', + ) + + const runOnce = () => + copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', defaultBehavior: 'mirror', targets: [] }, + }) + + await runOnce() + await expect(fileService.exists('/dataset/target/README.md')).resolves.toBe(true) + + // Second run must be idempotent — the root-level file must survive. + await runOnce() + await expect(fileService.exists('/dataset/target/README.md')).resolves.toBe(true) + await expect(fileService.exists('/dataset/target/pair-catalog-next/SKILL.md')).resolves.toBe( + true, + ) + }) + + it('does not clean up stale entries when behavior is not mirror', async () => { + const fileService = new InMemoryFileSystemService( + { + '/dataset/source/catalog/next/SKILL.md': '---\nname: next\n---\n# /next', + '/dataset/target/pair-catalog-removed/SKILL.md': '---\nname: pair-catalog-removed\n---', + }, + '/', + '/', + ) + + await copyPathOps({ + fileService, + source: 'source', + target: 'target', + datasetRoot: '/dataset', + options: { flatten: true, prefix: 'pair', defaultBehavior: 'overwrite', targets: [] }, + }) + + await expect( + fileService.exists('/dataset/target/pair-catalog-removed/SKILL.md'), + ).resolves.toBe(true) + }) + }) }) describe('copyPathOps - error cases', () => { diff --git a/packages/content-ops/src/ops/copyPathOps.ts b/packages/content-ops/src/ops/copyPathOps.ts index 0b3411d4..0ccc4917 100644 --- a/packages/content-ops/src/ops/copyPathOps.ts +++ b/packages/content-ops/src/ops/copyPathOps.ts @@ -504,8 +504,10 @@ async function copyFileWithTransform(ctx: { destPath: string transformOpts: TransformOpts dirMappingFiles: Map + topLevelFiles: Set }): Promise { - const { fileService, filePath, srcPath, destPath, transformOpts, dirMappingFiles } = ctx + const { fileService, filePath, srcPath, destPath, transformOpts, dirMappingFiles, topLevelFiles } = + ctx const dir = dirname(filePath) const fileName = filePath.slice(dir === '.' ? 0 : dir.length + 1) const transformedDir = dir === '.' ? null : transformPath(dir, transformOpts) @@ -515,6 +517,13 @@ async function copyFileWithTransform(ctx: { const targetFilePath = join(targetDir, fileName) await copyFileHelper(fileService, join(srcPath, filePath), targetFilePath, 'overwrite') + if (dir === '.') { + // Root-level source file — has no transformed subdirectory of its own, so it's + // never added to dirMappingFiles. Track it separately so cleanupStaleTransformedEntries + // knows it's a legitimate copy, not a stale leftover (see that function's docstring). + topLevelFiles.add(fileName) + } + if (dir !== '.' && transformedDir) { const leafName = dir.split('/').pop()! if (leafName !== transformedDir) { @@ -551,6 +560,36 @@ function buildPathMapping( return pathMapping } +/** + * Copies every file with naming transforms applied, collecting both the + * per-subdirectory mapping (for link rewriting, skill renames, and mirror + * cleanup of transformed directories) and the set of root-level file names + * (for mirror cleanup of files that have no subdirectory of their own). + */ +async function copyAllFilesWithTransform(params: { + fileService: FileSystemService + files: string[] + srcPath: string + destPath: string + transformOpts: TransformOpts +}): Promise<{ dirMappingFiles: Map; topLevelFiles: Set }> { + const { fileService, files, srcPath, destPath, transformOpts } = params + const dirMappingFiles = new Map() + const topLevelFiles = new Set() + for (const filePath of files) { + await copyFileWithTransform({ + fileService, + filePath, + srcPath, + destPath, + transformOpts, + dirMappingFiles, + topLevelFiles, + }) + } + return { dirMappingFiles, topLevelFiles } +} + /** * Copies a directory with flatten/prefix naming transforms applied. * Each file's directory path (relative to source) is transformed, then @@ -575,15 +614,21 @@ export async function copyDirectoryWithTransforms(params: { await fileService.mkdir(destPath, { recursive: true }) - const dirMappingFiles = new Map() - for (const filePath of files) { - await copyFileWithTransform({ + const { dirMappingFiles, topLevelFiles } = await copyAllFilesWithTransform({ + fileService, + files, + srcPath, + destPath, + transformOpts, + }) + + if (options?.defaultBehavior === 'mirror') { + await cleanupStaleTransformedEntries({ fileService, - filePath, - srcPath, destPath, - transformOpts, dirMappingFiles, + topLevelFiles, + transformOpts, }) } @@ -600,6 +645,47 @@ export async function copyDirectoryWithTransforms(params: { return skillNameMap.size > 0 ? { skillNameMap } : {} } +/** + * Removes stale top-level entries under a flatten/prefix target that no + * longer correspond to a source directory or a root-level source file. This + * is what makes `mirror` behavior idempotent across renames: a removed + * 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). + * + * `topLevelFiles` (file names copied directly from the source root, with no + * subdirectory of their own) must be included in `expected` alongside the + * transformed directory names — they're never in `dirMappingFiles` (which + * only tracks files under a subdirectory), so without this they'd be + * wrongly deleted as stale on the very next mirror run. + */ +async function cleanupStaleTransformedEntries(params: { + fileService: FileSystemService + destPath: string + dirMappingFiles: Map + topLevelFiles: Set + transformOpts: TransformOpts +}): Promise { + const { fileService, destPath, dirMappingFiles, topLevelFiles, transformOpts } = params + + const expected = new Set(topLevelFiles) + for (const originalSubDir of dirMappingFiles.keys()) { + const transformedDir = transformPath(originalSubDir, transformOpts) + expected.add(transformedDir.split('/')[0]!) + } + + 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}`) + } +} + async function rewriteLinksForTransformedDirs( params: { fileService: FileSystemService diff --git a/packages/content-ops/src/ops/skill-name-manifest.test.ts b/packages/content-ops/src/ops/skill-name-manifest.test.ts new file mode 100644 index 00000000..ad0f9847 --- /dev/null +++ b/packages/content-ops/src/ops/skill-name-manifest.test.ts @@ -0,0 +1,196 @@ +import { describe, it, expect } from 'vitest' +import { + readSkillNameManifest, + writeSkillNameManifest, + buildTransitionMap, + findOrphanedInstalledNames, + mergeSkillNameMaps, +} from './skill-name-manifest' +import { InMemoryFileSystemService } from '../test-utils' + +describe('readSkillNameManifest', () => { + it('returns an empty map when the manifest does not exist', async () => { + const fs = new InMemoryFileSystemService({}, '/', '/') + const result = await readSkillNameManifest(fs, '/project/.pair/.skill-name-map.json') + expect(result.size).toBe(0) + }) + + it('returns an empty map when the manifest is malformed JSON', async () => { + const fs = new InMemoryFileSystemService( + { '/project/.pair/.skill-name-map.json': '{not json' }, + '/', + '/', + ) + const result = await readSkillNameManifest(fs, '/project/.pair/.skill-name-map.json') + expect(result.size).toBe(0) + }) + + it('returns an empty map when the manifest has no skills field', async () => { + const fs = new InMemoryFileSystemService( + { '/project/.pair/.skill-name-map.json': JSON.stringify({ version: 1 }) }, + '/', + '/', + ) + const result = await readSkillNameManifest(fs, '/project/.pair/.skill-name-map.json') + expect(result.size).toBe(0) + }) + + it('reads a previously written manifest into a SkillNameMap', async () => { + const fs = new InMemoryFileSystemService( + { + '/project/.pair/.skill-name-map.json': JSON.stringify({ + version: 1, + skills: { next: 'pair-next', 'verify-quality': 'pair-capability-verify-quality' }, + }), + }, + '/', + '/', + ) + const result = await readSkillNameManifest(fs, '/project/.pair/.skill-name-map.json') + expect(result.get('next')).toBe('pair-next') + expect(result.get('verify-quality')).toBe('pair-capability-verify-quality') + }) +}) + +describe('writeSkillNameManifest', () => { + it('writes a manifest that can be read back identically', async () => { + const fs = new InMemoryFileSystemService({}, '/', '/') + const map = new Map([ + ['next', 'pair-next'], + ['implement', 'pair-process-implement'], + ]) + + await writeSkillNameManifest(fs, '/project/.pair/.skill-name-map.json', map) + const roundTripped = await readSkillNameManifest(fs, '/project/.pair/.skill-name-map.json') + + expect(roundTripped).toEqual(map) + }) + + it('overwrites a previous manifest', async () => { + const fs = new InMemoryFileSystemService( + { + '/project/.pair/.skill-name-map.json': JSON.stringify({ + version: 1, + skills: { next: 'pair-next' }, + }), + }, + '/', + '/', + ) + + await writeSkillNameManifest( + fs, + '/project/.pair/.skill-name-map.json', + new Map([['next', 'foo-next']]), + ) + const result = await readSkillNameManifest(fs, '/project/.pair/.skill-name-map.json') + expect(result.get('next')).toBe('foo-next') + }) + + it('is idempotent: writing the same map twice produces byte-identical output', async () => { + const fs = new InMemoryFileSystemService({}, '/', '/') + const map = new Map([['next', 'pair-next']]) + + await writeSkillNameManifest(fs, '/project/.pair/.skill-name-map.json', map) + const first = await fs.readFile('/project/.pair/.skill-name-map.json') + + await writeSkillNameManifest(fs, '/project/.pair/.skill-name-map.json', map) + const second = await fs.readFile('/project/.pair/.skill-name-map.json') + + expect(second).toBe(first) + }) +}) + +describe('buildTransitionMap', () => { + it('maps old installed name to new installed name after a prefix change', () => { + const previous = new Map([['next', 'pair-next']]) + const current = new Map([['next', 'foo-next']]) + const transitions = buildTransitionMap(previous, current) + expect(transitions.get('pair-next')).toBe('foo-next') + }) + + it('omits entries whose installed name did not change', () => { + const previous = new Map([['next', 'pair-next']]) + const current = new Map([['next', 'pair-next']]) + const transitions = buildTransitionMap(previous, current) + expect(transitions.size).toBe(0) + }) + + it('omits entries only present in previous (removed skill)', () => { + const previous = new Map([ + ['next', 'pair-next'], + ['removed', 'pair-removed'], + ]) + const current = new Map([['next', 'pair-next']]) + const transitions = buildTransitionMap(previous, current) + expect(transitions.size).toBe(0) + }) + + it('omits entries only present in current (new skill)', () => { + const previous = new Map([['next', 'pair-next']]) + const current = new Map([ + ['next', 'pair-next'], + ['added', 'pair-added'], + ]) + const transitions = buildTransitionMap(previous, current) + expect(transitions.size).toBe(0) + }) + + it('handles multiple simultaneous renames', () => { + const previous = new Map([ + ['next', 'pair-next'], + ['implement', 'pair-process-implement'], + ]) + const current = new Map([ + ['next', 'foo-next'], + ['implement', 'foo-process-implement'], + ]) + const transitions = buildTransitionMap(previous, current) + expect(transitions.get('pair-next')).toBe('foo-next') + expect(transitions.get('pair-process-implement')).toBe('foo-process-implement') + }) +}) + +describe('findOrphanedInstalledNames', () => { + it('returns installed names for skills removed from the current registry', () => { + const previous = new Map([ + ['next', 'pair-next'], + ['removed', 'pair-removed'], + ]) + const current = new Map([['next', 'pair-next']]) + expect(findOrphanedInstalledNames(previous, current)).toEqual(['pair-removed']) + }) + + it('returns empty array when nothing was removed', () => { + const previous = new Map([['next', 'pair-next']]) + const current = new Map([['next', 'pair-next']]) + expect(findOrphanedInstalledNames(previous, current)).toEqual([]) + }) + + it('returns empty array for a fresh install (no previous manifest)', () => { + const previous = new Map() + const current = new Map([['next', 'pair-next']]) + expect(findOrphanedInstalledNames(previous, current)).toEqual([]) + }) +}) + +describe('mergeSkillNameMaps', () => { + it('merges disjoint maps', () => { + const a = new Map([['next', 'pair-next']]) + const b = new Map([['pair-implement', 'foo-implement']]) + const merged = mergeSkillNameMaps(a, b) + expect(merged.get('next')).toBe('pair-next') + expect(merged.get('pair-implement')).toBe('foo-implement') + }) + + it('later maps win on key collision', () => { + const a = new Map([['next', 'pair-next']]) + const b = new Map([['next', 'foo-next']]) + const merged = mergeSkillNameMaps(a, b) + expect(merged.get('next')).toBe('foo-next') + }) + + it('returns an empty map when called with no arguments', () => { + expect(mergeSkillNameMaps().size).toBe(0) + }) +}) diff --git a/packages/content-ops/src/ops/skill-name-manifest.ts b/packages/content-ops/src/ops/skill-name-manifest.ts new file mode 100644 index 00000000..fab0e124 --- /dev/null +++ b/packages/content-ops/src/ops/skill-name-manifest.ts @@ -0,0 +1,103 @@ +/** + * Persists the skill name registry across install/update runs. + * + * Without a recorded previous mapping, an already-installed reference like + * `/pair-next` cannot be safely rewritten after a prefix change (e.g. to + * `/foo-next`): matching on the literal old prefix string would produce + * false positives on unrelated content, and files copied with `add` + * behavior (never overwritten, e.g. adoption docs) are not re-derived from + * source on every run so their stale references would never be revisited. + * + * Recording {shortName: installedName} from the previous run lets callers + * compute an exact {oldInstalledName: newInstalledName} transition map + * instead of guessing from string patterns. + */ + +import { FileSystemService } from '../file-system' +import type { SkillNameMap } from './skill-reference-rewriter' + +export type SkillNameManifest = { + version: 1 + skills: Record +} + +/** + * Reads a previously recorded skill name map. + * Returns an empty map when the manifest is missing or malformed — + * callers should treat this the same as "no previous install". + */ +export async function readSkillNameManifest( + fileService: FileSystemService, + manifestPath: string, +): Promise { + if (!(await fileService.exists(manifestPath))) return new Map() + + try { + const raw = await fileService.readFile(manifestPath) + const parsed = JSON.parse(raw) as Partial | null + if (!parsed || typeof parsed.skills !== 'object' || parsed.skills === null) return new Map() + return new Map(Object.entries(parsed.skills as Record)) + } catch { + return new Map() + } +} + +/** + * Persists the current skill name map so the next install/update run can + * detect renames (prefix changes) and removals. + */ +export async function writeSkillNameManifest( + fileService: FileSystemService, + manifestPath: string, + skillNameMap: SkillNameMap, +): Promise { + const manifest: SkillNameManifest = { + version: 1, + skills: Object.fromEntries(skillNameMap), + } + await fileService.writeFile(manifestPath, JSON.stringify(manifest, null, 2) + '\n') +} + +/** + * Builds a transition map (oldInstalledName -> newInstalledName) for + * skills present in both the previous and current registry under the same + * short (source) name but with a different installed name — e.g. after a + * prefix change. Entries are only added when the installed name actually + * changed. + */ +export function buildTransitionMap(previous: SkillNameMap, current: SkillNameMap): SkillNameMap { + const transitions: SkillNameMap = new Map() + for (const [shortName, oldInstalledName] of previous) { + const newInstalledName = current.get(shortName) + if (newInstalledName && newInstalledName !== oldInstalledName) { + transitions.set(oldInstalledName, newInstalledName) + } + } + return transitions +} + +/** + * Installed names for short names present in the previous mapping but + * absent from the current registry (removed/disabled skills). Callers + * should warn if these are still referenced — they must be left as-is, + * not rewritten (there is no correct target name to rewrite them to). + */ +export function findOrphanedInstalledNames( + previous: SkillNameMap, + current: SkillNameMap, +): string[] { + const orphaned: string[] = [] + for (const [shortName, oldInstalledName] of previous) { + if (!current.has(shortName)) orphaned.push(oldInstalledName) + } + return orphaned +} + +/** Merges any number of skill name maps into one (later maps win on key collision). */ +export function mergeSkillNameMaps(...maps: SkillNameMap[]): SkillNameMap { + const merged: SkillNameMap = new Map() + for (const map of maps) { + for (const [k, v] of map) merged.set(k, v) + } + return merged +} 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 23eb22c1..e162a5a2 100644 --- a/packages/content-ops/src/ops/skill-reference-rewriter.test.ts +++ b/packages/content-ops/src/ops/skill-reference-rewriter.test.ts @@ -3,6 +3,7 @@ import { rewriteSkillReferences, buildSkillNameMap, rewriteSkillReferencesInFiles, + findSkillReferences, } from './skill-reference-rewriter' import { InMemoryFileSystemService } from '../test-utils' @@ -123,6 +124,108 @@ describe('rewriteSkillReferences', () => { const input = 'run /next:' expect(rewriteSkillReferences(input, map)).toBe('run /pair-next:') }) + + describe('fenced code blocks (AC6)', () => { + it('leaves a fenced code block referencing a skill name untouched', () => { + const input = ['Run `/next` to start.', '```', '/next', '```', 'Then run /implement.'].join( + '\n', + ) + expect(rewriteSkillReferences(input, map)).toBe( + ['Run `/pair-next` to start.', '```', '/next', '```', 'Then run /pair-process-implement.'].join( + '\n', + ), + ) + }) + + it('leaves a fenced block with a language tag and multiple lines untouched', () => { + const input = [ + '```text', + 'pair install', + '/next', + '/implement', + '```', + 'Compose /next.', + ].join('\n') + expect(rewriteSkillReferences(input, map)).toBe( + ['```text', 'pair install', '/next', '/implement', '```', 'Compose /pair-next.'].join( + '\n', + ), + ) + }) + + it('handles a tilde-fenced code block', () => { + const input = ['~~~', '/next', '~~~', 'Run /next.'].join('\n') + expect(rewriteSkillReferences(input, map)).toBe( + ['~~~', '/next', '~~~', 'Run /pair-next.'].join('\n'), + ) + }) + + it('rewrites prose again after a fence closes', () => { + const input = ['```', '/next', '```', 'Run /next after the fence.'].join('\n') + expect(rewriteSkillReferences(input, map)).toBe( + ['```', '/next', '```', 'Run /pair-next after the fence.'].join('\n'), + ) + }) + + it('still rewrites inline single-backtick code spans (not a fence)', () => { + const input = 'Composes `/verify-quality` inline.' + expect(rewriteSkillReferences(input, map)).toBe( + 'Composes `/pair-capability-verify-quality` inline.', + ) + }) + + it('treats an unterminated fence as fenced through end of content', () => { + const input = ['```', '/next', 'still inside /implement'].join('\n') + expect(rewriteSkillReferences(input, map)).toBe(input) + }) + + it('does not treat a backtick-containing info string as a fence-open (CommonMark)', () => { + // Per CommonMark, a backtick-fence's info string must itself be + // backtick-free; a line like this never opens a real fence, so + // rewriting must continue normally afterward instead of being + // suppressed for the rest of the file. + const input = ['intro line', '```inline `code` marker', 'Run /next to start.'].join('\n') + expect(rewriteSkillReferences(input, map)).toBe( + ['intro line', '```inline `code` marker', 'Run /pair-next to start.'].join('\n'), + ) + }) + + it('still treats a tilde-fence with a backtick in its info string as fenced', () => { + // Tilde fences have no backtick restriction on the info string. + const input = ['~~~lang `with backtick`', '/next', '~~~', 'Run /next after.'].join('\n') + expect(rewriteSkillReferences(input, map)).toBe( + ['~~~lang `with backtick`', '/next', '~~~', 'Run /pair-next after.'].join('\n'), + ) + }) + }) +}) + +describe('findSkillReferences', () => { + it('returns empty array when no names given', () => { + expect(findSkillReferences('/next', [])).toEqual([]) + }) + + it('finds a prose invocation', () => { + expect(findSkillReferences('run /old-name here', ['old-name'])).toEqual(['old-name']) + }) + + it('finds a backtick-wrapped invocation', () => { + expect(findSkillReferences('`/old-name`', ['old-name'])).toEqual(['old-name']) + }) + + it('does not find a name only present inside a fenced code block', () => { + const content = ['```', '/old-name', '```'].join('\n') + expect(findSkillReferences(content, ['old-name'])).toEqual([]) + }) + + it('does not match a name that is not present', () => { + expect(findSkillReferences('nothing here', ['old-name'])).toEqual([]) + }) + + it('finds multiple distinct names', () => { + const found = findSkillReferences('/a and /b', ['a', 'b', 'c']) + expect(found.sort()).toEqual(['a', 'b']) + }) }) describe('buildSkillNameMap', () => { diff --git a/packages/content-ops/src/ops/skill-reference-rewriter.ts b/packages/content-ops/src/ops/skill-reference-rewriter.ts index 301ceff2..c8f5c0f4 100644 --- a/packages/content-ops/src/ops/skill-reference-rewriter.ts +++ b/packages/content-ops/src/ops/skill-reference-rewriter.ts @@ -6,6 +6,18 @@ * to their new prefixed names (`/pair-catalog-next`, `/pair-capability-verify-quality`). * * Pattern: pure rewrite function + async file orchestrator (same shape as link-rewriter). + * + * Fenced code blocks (```...``` or ~~~...~~~) are left untouched — a skill name quoted + * as example text inside a fenced block is not a live invocation. Inline code spans + * (single backtick, e.g. `` `/next` ``) are still rewritten. + * + * Known limitation: fence detection is column-anchored to ≤3 leading spaces + * (matching CommonMark's rule for top-level fences), so a fence nested + * inside a blockquote (e.g. `> ```) is never recognized as fenced and its + * content is rewritten like normal prose — the inverse of the guarantee + * above. Not fixed: KB skill bodies don't use blockquoted fences, and + * correctly supporting them needs blockquote-prefix-aware line stripping, + * which is a larger change for a case that doesn't occur in practice. */ import { logger } from '../observability' @@ -25,29 +37,125 @@ function escapeRegex(s: string): string { return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') } +/** + * Builds a boundary-aware, single-line regex matching `/name` for a given skill name. + * Boundary characters (before): start-of-line, whitespace, backtick, double-quote, (, | + * Boundary characters (after): end-of-line, whitespace, backtick, double-quote, ), |, , . : ; ! ? ] + */ +function buildReferenceRegex(name: string): RegExp { + return new RegExp(`(?<=^|[\\s\`"(|])\\/${escapeRegex(name)}(?=$|[\\s\`")|,.:;!?\\]])`, 'g') +} + +/** Detects a fenced code block delimiter line (```/~~~, up to 3 leading spaces, optional info string). */ +const FENCE_OPEN_RE = /^ {0,3}(`{3,}|~{3,})/ + +/** + * Matches a fence-open line, honoring the CommonMark rule that a + * backtick-fence's info string (the text after the marker on the opening + * line) must itself be backtick-free. Without this check, a single-line + * construct like `` ```inline `code` span``` `` — which CommonMark does NOT + * treat as a fence at all — would be misclassified as fence-open here and + * suppress rewriting for the rest of the file (no closing marker would ever + * legitimately match). Tilde fences have no such restriction, since + * backticks in their info string are unambiguous. + * + * Known limitation (not handled): fences nested inside a blockquote + * (e.g. `> ```) are column-anchored out of this check entirely — see + * module docs. + */ +function matchFenceOpen(line: string): { char: string; len: number } | null { + const m = FENCE_OPEN_RE.exec(line) + if (!m) return null + const marker = m[1]! + const char = marker[0]! + if (char === '`') { + const infoString = line.slice(m[0].length) + if (infoString.includes('`')) return null + } + return { char, len: marker.length } +} + +function isFenceClose(line: string, fenceChar: string, fenceLen: number): boolean { + const m = /^ {0,3}(`+|~+)\s*$/.exec(line) + if (!m) return false + const marker = m[1]! + return marker[0] === fenceChar && marker.length >= fenceLen +} + +/** + * Splits content into lines and applies `transform` to every line that is + * NOT inside a fenced code block. Fence delimiter lines and their contents + * are passed through unchanged. Rejoins with '\n'. + */ +function transformOutsideFences(content: string, transform: (line: string) => string): string { + const lines = content.split('\n') + const result: string[] = [] + let fence: { char: string; len: number } | null = null + + for (const line of lines) { + if (fence) { + result.push(line) + if (isFenceClose(line, fence.char, fence.len)) fence = null + continue + } + + const opened = matchFenceOpen(line) + if (opened) { + fence = opened + result.push(line) + continue + } + + result.push(transform(line)) + } + + return result.join('\n') +} + /** * Rewrites skill cross-references in content. * Replaces `/oldName` with `/newName` for every entry in the map. * Entries are processed longest-first to avoid partial matches. - * - * Boundary characters (before): start-of-line, whitespace, backtick, double-quote, (, | - * Boundary characters (after): end-of-line, whitespace, backtick, double-quote, ), |, , . : ; ! ? ] + * Content inside fenced code blocks is left untouched (see module docs). */ export function rewriteSkillReferences(content: string, skillNameMap: SkillNameMap): string { if (skillNameMap.size === 0) return content const sorted = [...skillNameMap.entries()].sort((a, b) => b[0].length - a[0].length) + // Compile each pattern once per call, not once per line — String.replace() resets a + // global regex's lastIndex to 0 at the start of every call, so reusing the same + // compiled RegExp across lines is safe (unlike reusing it across .test() calls). + const compiled = sorted.map( + ([oldName, newName]) => [buildReferenceRegex(oldName), newName] as const, + ) - let result = content - for (const [oldName, newName] of sorted) { - const pattern = new RegExp( - `(?<=^|[\\s\`"(|])\\/${escapeRegex(oldName)}(?=$|[\\s\`")|,.:;!?\\]])`, - 'gm', - ) - result = result.replace(pattern, `/${newName}`) - } + return transformOutsideFences(content, line => { + let result = line + for (const [pattern, newName] of compiled) { + result = result.replace(pattern, `/${newName}`) + } + return result + }) +} - return result +/** + * Scans content (skipping fenced code blocks) for literal `/name` invocations + * matching any of the given names. Used to detect references to skills that + * are no longer in the registry (e.g. removed/disabled) so callers can warn + * instead of silently rewriting or dropping them. + */ +export function findSkillReferences(content: string, names: Iterable): string[] { + const nameList = [...new Set(names)].filter(n => n.length > 0) + if (nameList.length === 0) return [] + + const found = new Set() + transformOutsideFences(content, line => { + for (const name of nameList) { + if (buildReferenceRegex(name).test(line)) found.add(name) + } + return line + }) + return [...found] } /** diff --git a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md index e31226f8..eab679d8 100644 --- a/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md +++ b/packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/code-organization/file-structure.md @@ -48,6 +48,10 @@ UserProfile.stories.tsx (if using Storybook) ```text +**Tests spanning multiple modules**: when a test exercises more than one implementation file together (e.g. an integration test driving two collaborating functions end-to-end), it does not get its own standalone file — it goes in the test file already co-located with the *root* module of that call chain, the one whose exported entry point the test is actually verifying. A test that seeds its fixture through module B but asserts on module A's behavior belongs in `A.test.ts`, not a new `A-and-b-integration.test.ts`. This keeps the co-location rule intact (one root module → one test file) instead of a proliferation of ad hoc integration-test files that duplicate coverage or drift out of sync with the modules they actually exercise. + +This does not apply to two other, already-common test categories that are correctly named after what they validate rather than a source module: end-to-end/page-level tests (e.g. `landing.e2e.test.ts`, testing a user flow across many files by design) and content/asset-validation tests (e.g. asserting on a generated markdown file's content, where there is no single source module to co-locate against). + **Types**: Co-locate types when feature-specific: ```