diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9194bd9a..13b7b60a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -122,6 +122,12 @@ domains// knowledge/ # Optional: shared domain reference ``` +Domain `knowledge/` is copied **beside every skill in the domain**, so an installed skill +body reaches it as `knowledge/.md`. That is a different shape from this repo, where +`knowledge/` sits two levels above a skill. **Cite knowledge files by name, or by the +installed-relative path — never by a repo-relative one.** See +[Referring to domain knowledge from a skill](README.md#referring-to-domain-knowledge-from-a-skill). + ### `skill.md` Format Your `skill.md` should include YAML frontmatter plus body content: diff --git a/README.md b/README.md index c0954790..f359f971 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,30 @@ tools/ .targets.local.example # template for maintainer config ``` +### Referring to domain knowledge from a skill + +`knowledge/` is copied **beside every skill in the domain**, so the installed tree is +flatter than this repo's: + +``` +repo domains//knowledge/x.md domains//skills//skill.md +installed .claude/skills/mms-/knowledge/x.md .claude/skills/mms-/SKILL.md +``` + +A skill body therefore reaches its knowledge as **`knowledge/x.md`** once installed, but as +`../../knowledge/x.md` in the repo — and from a `references/` file the two are `../knowledge/x.md` +and `../../../knowledge/x.md`. **A repo-relative path is broken in the delivered output**, and +nothing reports it. Cite by name, or by the installed-relative form: + +```markdown +See the `selector-anti-patterns` knowledge file. +See [x](knowledge/selector-anti-patterns.md) +See [x](../../knowledge/selector-anti-patterns.md) +``` + +`test/cli.test.mjs` guards this: every `knowledge/…` reference in an emitted skill must +resolve on disk after install. + ## Domains today | Domain | Audience | Examples | diff --git a/test/cli.test.mjs b/test/cli.test.mjs index e4829a10..b0fe8210 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -1,6 +1,6 @@ import assert from 'node:assert/strict'; import { spawnSync } from 'node:child_process'; -import { existsSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -240,3 +240,226 @@ describe('managed skill pruning', () => { assert.equal(existsSync(stale), true); }); }); + +describe('installed knowledge references resolve', () => { + let root; + let source; + let target; + + before(() => { + root = mkdtempSync(path.join(os.tmpdir(), 'mms-knowledge-refs-')); + source = path.join(root, 'source'); + target = path.join(root, 'target'); + mkdirSync(path.join(source, 'tools'), { recursive: true }); + symlinkSync(INSTALL, path.join(source, 'tools', 'install')); + mkdirSync(target, { recursive: true }); + + // The fixture carries a real CONSUMER: a skill body that cites a knowledge file + // the way shipped skills actually do — skill-relative `knowledge/`. A fixture + // without one cannot exhibit a layout regression, which is how MetaMask/skills#87 + // shipped a change that stranded 12 such references while every test passed. + const knowledge = path.join(source, 'domains', 'testing', 'knowledge'); + mkdirSync(knowledge, { recursive: true }); + writeFileSync(path.join(knowledge, 'alpha.md'), '# Alpha\n'); + const dir = path.join(source, 'domains', 'testing', 'skills', 'consumer'); + mkdirSync(dir, { recursive: true }); + writeFileSync( + path.join(dir, 'skill.md'), + [ + '---', + 'name: consumer', + 'description: Cites a domain knowledge file', + 'maturity: stable', + '---', + 'Read [alpha](knowledge/alpha.md) before starting.', + ].join('\n'), + ); + }); + + after(() => { + rmSync(root, { recursive: true, force: true }); + }); + + test('every knowledge/ reference in an emitted skill resolves on disk', () => { + const result = spawnSync( + 'bash', + [INSTALL, '--target', target, '--repo', 'core', '--source', source], + { encoding: 'utf8' }, + ); + assert.equal(result.status, 0, result.stderr); + + const emitted = [ + ['.claude/skills', 'mms-consumer', 'SKILL.md'], + ['.cursor/rules', 'mms-consumer', 'RULE.md'], + ['.agents/skills', 'mms-consumer', 'SKILL.md'], + ]; + for (const [base, name, file] of emitted) { + const skillDir = path.join(target, base, name); + const body = readFileSync(path.join(skillDir, file), 'utf8'); + const refs = [...body.matchAll(/\]\((knowledge\/[\w.-]+)\)/gu)].map((m) => m[1]); + assert.ok(refs.length > 0, `${base}/${name}: expected a knowledge reference in the emitted body`); + for (const ref of refs) { + assert.ok( + existsSync(path.join(skillDir, ref)), + `${base}/${name}: dangling knowledge reference ${ref} — the body cites it but install did not place it there`, + ); + } + } + }); +}); + +describe('corpus: knowledge citations resolve within their own domain', () => { + // `knowledge/` is copied per DOMAIN, so a skill can only cite files from its own + // domain's knowledge dir. A citation naming another domain's file can never resolve + // for any consumer or operator — the skill installs fine and the reference dangles. + // + // Known-unresolved, tracked separately; the list must only ever shrink. Each entry is + // a cross-domain citation of testing/knowledge/testing-layers.md, which the installer + // has no way to deliver into these domains. + const KNOWN_UNRESOLVED = new Set([ + 'domains/coding/skills/coding-guidelines/repos/metamask-mobile.md → knowledge/testing-layers.md', + 'domains/perps/skills/perps-review-pr/skill.md → knowledge/testing-layers.md', + 'domains/pr-workflow/skills/pr-guidelines/repos/metamask-mobile.md → knowledge/testing-layers.md', + 'domains/pr-workflow/skills/pr-readiness-check/repos/metamask-mobile.md → knowledge/testing-layers.md', + ]); + + function collectCitations(domainsDir) { + const found = []; + const walk = (dir) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) walk(full); + else if (entry.isFile() && entry.name.endsWith('.md')) { + const rel = path.relative(path.join(domainsDir, '..'), full).split(path.sep).join('/'); + if (!rel.includes('/skills/')) continue; + const domain = rel.split('/')[1]; + const body = readFileSync(full, 'utf8'); + for (const m of body.matchAll(/\]\((knowledge\/[\w.-]+\.md)\)|`(knowledge\/[\w.-]+\.md)`/gu)) { + const ref = m[1] || m[2]; + found.push({ rel, domain, ref, key: `${rel} → ${ref}` }); + } + } + } + }; + walk(domainsDir); + return found; + } + + test('no skill cites a knowledge file its own domain does not ship', () => { + const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); + const domainsDir = path.join(repoRoot, 'domains'); + const unresolved = collectCitations(domainsDir).filter( + (c) => !existsSync(path.join(domainsDir, c.domain, c.ref)), + ); + + const unexpected = unresolved.filter((c) => !KNOWN_UNRESOLVED.has(c.key)); + assert.deepEqual( + unexpected.map((c) => c.key), + [], + 'new dangling knowledge citation(s) — a skill may only cite its own domain\'s knowledge', + ); + }); + + test('the known-unresolved list has no stale entries', () => { + const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); + const domainsDir = path.join(repoRoot, 'domains'); + const stillBroken = new Set( + collectCitations(domainsDir) + .filter((c) => !existsSync(path.join(domainsDir, c.domain, c.ref))) + .map((c) => c.key), + ); + const fixed = [...KNOWN_UNRESOLVED].filter((k) => !stillBroken.has(k)); + assert.deepEqual(fixed, [], 'these citations now resolve — remove them from KNOWN_UNRESOLVED'); + }); +}); + +describe('corpus: content is safe to publish and links stay current', () => { + function allSkillDocs() { + const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); + const domainsDir = path.join(repoRoot, 'domains'); + const out = []; + const walk = (dir) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) walk(full); + else if (entry.isFile() && /\.(md|py|sh|ts|mjs)$/u.test(entry.name)) { + out.push({ rel: path.relative(repoRoot, full).split(path.sep).join('/'), body: readFileSync(full, 'utf8') }); + } + } + }; + walk(domainsDir); + return out; + } + + // Who is running this, from whatever the environment knows. Short or generic values are + // dropped: a two-letter git username matches everywhere and would fail every file. + function whoAmI() { + const raw = [ + process.env.GITHUB_ACTOR, + process.env.USER, + (() => { + const r = spawnSync('git', ['config', 'user.name'], { encoding: 'utf8' }); + return r.status === 0 ? r.stdout.trim() : ''; + })(), + ]; + const GENERIC = new Set(['root', 'runner', 'ubuntu', 'admin', 'user', 'ci', 'build', 'node']); + return [...new Set(raw.filter(Boolean).map((v) => v.trim()))] + .filter((v) => v.length >= 4 && !GENERIC.has(v.toLowerCase()) && !v.includes(' ')); + } + + // This repo is public. A personal path, handle, or private-repo name in a skill is + // both a leak and a dead reference for every reader but its author. + // + // The specific names are NOT listed here. A denylist of private identifiers, committed to a + // public repo, publishes every identifier it protects — the guard discloses what it guards, + // and this test previously named five. Structural patterns that describe a *shape* are safe + // and stay inline; anything that names a particular person, host or repo comes from the + // environment. Set SKILLS_PRIVATE_PATTERNS to a newline-separated list of regex sources + // (CI secret, or an untracked local file) to extend this locally. + test('no personal paths, handles, or private-repo references', () => { + const PERSONAL = [ + [/(^|[\s"'`(])\/(home|Users)\/[a-z][a-z0-9_.-]*/u, 'absolute personal path'], + [/\bgit@[a-z0-9.-]+:[^\s]+/u, 'ssh remote'], + // Derived, not listed. The identifiers most likely to leak are the ones belonging to + // whoever is running — so ask the environment who that is instead of committing a + // denylist. In CI that is GITHUB_ACTOR; locally it is the git identity. This fires by + // default: an env-var-only version was inert everywhere, which is a check that cannot + // fail dressed as a check. + ...whoAmI().map((who) => [ + new RegExp(`\\b${who.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&')}\\b`, 'iu'), + 'your own handle or identity', + ]), + ...(process.env.SKILLS_PRIVATE_PATTERNS ?? '') + .split('\n') + .map((line) => line.trim()) + .filter(Boolean) + .map((src) => [new RegExp(src, 'u'), 'configured private identifier']), + ]; + const hits = []; + for (const { rel, body } of allSkillDocs()) { + body.split('\n').forEach((line, i) => { + for (const [re, label] of PERSONAL) { + if (re.test(line)) hits.push(`${rel}:${i + 1} (${label})`); + } + }); + } + assert.deepEqual(hits, [], 'personal or private references must not ship in a public skill'); + }); + + // A frozen branch is worse than a deleted one: the link loads, and the reader gets + // stale source with no signal. `metamask-extension` moved to `main`; `develop` still + // exists but stopped receiving commits in January 2026. + const FROZEN_BRANCHES = ['develop']; + test('no links into a known-frozen branch', () => { + const hits = []; + for (const { rel, body } of allSkillDocs()) { + body.split('\n').forEach((line, i) => { + for (const branch of FROZEN_BRANCHES) { + const re = new RegExp(`github\\.com/[^\\s)]+/(blob|tree)/${branch}/`, 'u'); + if (re.test(line)) hits.push(`${rel}:${i + 1} (→ ${branch})`); + } + }); + } + assert.deepEqual(hits, [], 'link points into a frozen branch — use the repo\'s default branch, or pin a SHA'); + }); +});