From 67aa743d28ec7b658caad9e5ccbbf09341d30612 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 12:13:23 -0400 Subject: [PATCH 1/6] Install domain `knowledge/` once per domain instead of once per skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `copy_domain_knowledge` ran per skill per operator, so a domain with K knowledge files and N skills wrote K*N copies each. `perps` shipped 108 files where 27 were needed; `performance` after the pending domain PRs would ship 168 for 21. Knowledge now installs once to `mms--knowledge/`, a sibling of the domain's installed skills. An upgrade removes the per-skill copies an older install left behind, and the shared directory is registered as expected so `--prune-stale` leaves it alone. This also fixes cross-layer references. The per-skill copy sat as a sibling of `references/`, three levels from where it lives in the repo, so `../../../knowledge/x.md` resolved in the repo and broke once installed while `../knowledge/x.md` did the reverse — no relative path was correct in both, and nothing reported the breakage. README and CONTRIBUTING now state the rule: cite knowledge files by name, never by relative path. --- CONTRIBUTING.md | 5 +++ README.md | 26 +++++++++++- test/cli.test.mjs | 82 +++++++++++++++++++++++++++++++++++++ tools/install | 101 ++++++++++++++++++++++++++++++++++++---------- 4 files changed, 192 insertions(+), 22 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9194bd9a..f1b93625 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -122,6 +122,11 @@ domains// knowledge/ # Optional: shared domain reference ``` +Domain `knowledge/` installs once per domain as `mms--knowledge/`, a sibling of the +domain's installed skills — a different shape from the repo's. **Cite knowledge files by +name, not by relative path**: no relative path resolves correctly in both layouts. 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..85afec6b 100644 --- a/README.md +++ b/README.md @@ -134,7 +134,7 @@ domains// scripts/ # optional helper scripts adapters/ # optional runtime payloads used by scripts repos/.md # optional repo-specific overlay - knowledge/ # optional shared domain reference, installed beside each domain skill + knowledge/ # optional shared domain reference, installed once as mms--knowledge/ tools/ install # core writer (mms- prefix, multi-operator output) sync # Flow 2: `yarn skills` wrapper for engineers @@ -143,6 +143,30 @@ tools/ .targets.local.example # template for maintainer config ``` +### Referring to domain knowledge from a skill + +Domain `knowledge/` installs **once per domain**, as a sibling of the domain's skills: + +``` +.claude/skills/mms-/SKILL.md +.claude/skills/mms-/references/… +.claude/skills/mms--knowledge/… # one copy, shared by every skill in the domain +``` + +That layout does not match the repo's, where `knowledge/` sits two levels above a skill and +three above its `references/`. **So cite knowledge files by name, never by relative path** — +no relative path is correct in both layouts, and one written against either will silently +break in the other: + +```markdown +See the `selector-anti-patterns` knowledge file. +See [x](../../../knowledge/selector-anti-patterns.md) +See [x](../knowledge/selector-anti-patterns.md) +``` + +Section anchors have the same problem for a different reason — they break the moment the +target file is reorganized. Name the file and the section in prose instead. + ## Domains today | Domain | Audience | Examples | diff --git a/test/cli.test.mjs b/test/cli.test.mjs index e4829a10..4f916dd9 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -240,3 +240,85 @@ describe('managed skill pruning', () => { assert.equal(existsSync(stale), true); }); }); + +describe('domain knowledge is installed once per domain', () => { + let root; + let source; + let target; + + before(() => { + root = mkdtempSync(path.join(os.tmpdir(), 'mms-knowledge-')); + 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 }); + + // One domain, two skills, two knowledge files. The pre-0.3 installer copied the + // knowledge dir into BOTH skills; the shared layout writes it once. + const knowledge = path.join(source, 'domains', 'testing', 'knowledge'); + mkdirSync(knowledge, { recursive: true }); + writeFileSync(path.join(knowledge, 'alpha.md'), '# Alpha\n'); + writeFileSync(path.join(knowledge, 'beta.md'), '# Beta\n'); + for (const name of ['first', 'second']) { + const dir = path.join(source, 'domains', 'testing', 'skills', name); + mkdirSync(dir, { recursive: true }); + writeFileSync( + path.join(dir, 'skill.md'), + ['---', `name: ${name}`, `description: Skill ${name}`, 'maturity: stable', '---', 'Body.'].join('\n'), + ); + } + }); + + after(() => { + rmSync(root, { recursive: true, force: true }); + }); + + test('writes one shared knowledge dir per operator, not one per skill', () => { + const result = spawnSync( + 'bash', + [INSTALL, '--target', target, '--repo', 'core', '--source', source], + { encoding: 'utf8' }, + ); + assert.equal(result.status, 0, result.stderr); + + for (const base of ['.claude/skills', '.cursor/rules', '.agents/skills']) { + const shared = path.join(target, base, 'mms-testing-knowledge'); + assert.ok(existsSync(path.join(shared, 'alpha.md')), `${base} missing shared alpha.md`); + assert.ok(existsSync(path.join(shared, 'beta.md')), `${base} missing shared beta.md`); + // and NOT duplicated into each skill + for (const name of ['mms-first', 'mms-second']) { + assert.ok( + !existsSync(path.join(target, base, name, 'knowledge')), + `${base}/${name} should not carry a per-skill knowledge copy`, + ); + } + } + }); + + test('upgrading removes a per-skill knowledge copy left by an older install', () => { + const stalePath = path.join(target, '.claude/skills', 'mms-first', 'knowledge'); + mkdirSync(stalePath, { recursive: true }); + writeFileSync(path.join(stalePath, 'alpha.md'), '# stale\n'); + assert.ok(existsSync(path.join(stalePath, 'alpha.md'))); + + const result = spawnSync( + 'bash', + [INSTALL, '--target', target, '--repo', 'core', '--source', source], + { encoding: 'utf8' }, + ); + assert.equal(result.status, 0, result.stderr); + assert.ok(!existsSync(stalePath), 'stale per-skill knowledge dir should be removed on upgrade'); + assert.ok(existsSync(path.join(target, '.claude/skills', 'mms-testing-knowledge', 'alpha.md'))); + }); + + test('--prune-stale keeps the shared knowledge dir', () => { + const result = spawnSync( + 'bash', + [INSTALL, '--target', target, '--repo', 'core', '--source', source, '--prune-stale'], + { encoding: 'utf8' }, + ); + assert.equal(result.status, 0, result.stderr); + assert.ok(existsSync(path.join(target, '.claude/skills', 'mms-testing-knowledge', 'alpha.md'))); + }); +}); diff --git a/tools/install b/tools/install index e39404aa..1c605dcb 100755 --- a/tools/install +++ b/tools/install @@ -364,34 +364,66 @@ copy_bundle_dirs() { done } -copy_domain_knowledge() { - local skill_dir="$1" dest_dir="$2" label="$3" - local domain_dir; domain_dir=$(cd "$skill_dir/../.." && pwd) +# Domain knowledge installs ONCE per domain, to its own `${PREFIX}-knowledge/` +# directory, rather than once per skill. +# +# It used to be copied into every skill directory in the domain, which meant a domain +# with K knowledge files and N skills wrote K*N copies per operator — `performance` +# alone lands 7*8 = 56 — and made "one source" true in the repo but not on disk. +# +# The shared directory also fixes cross-layer references. Per-skill copies sat as +# siblings of `references/`, three levels apart from where they live in the repo, so +# `../../../knowledge/x.md` resolved in the repo and broke once installed while +# `../knowledge/x.md` did the reverse — no relative path was correct in both. Skills +# therefore cite knowledge files BY NAME, and the shared directory is the one place +# every operator resolves that name to. +KNOWLEDGE_DONE="" + +knowledge_out_name() { + printf '%s%s-knowledge' "$PREFIX" "$1" +} + +install_domain_knowledge() { + local domain_name="$1" domain_dir="$2" local knowledge_dir="$domain_dir/knowledge" + [[ -d "$knowledge_dir" ]] || return 0 - if [[ -d "$knowledge_dir" ]]; then - action "$label/knowledge/" - $DRY_RUN && return - mkdir -p "$dest_dir" - rm -rf "$dest_dir/knowledge" - cp -R "$knowledge_dir" "$dest_dir/knowledge" - else - if $DRY_RUN; then - [[ -e "$dest_dir/knowledge" ]] && action "$label/knowledge/ (remove stale)" - return 0 - fi - rm -rf "$dest_dir/knowledge" - fi + local out_name; out_name=$(knowledge_out_name "$domain_name") + # Once per domain per run, however many skills the domain ships. + case " $KNOWLEDGE_DONE " in + *" $domain_name "*) return 0 ;; + esac + KNOWLEDGE_DONE="$KNOWLEDGE_DONE $domain_name" + + local entry parent label dest + for entry in "$CLAUDE_DIR|.claude/skills" "$CURSOR_DIR|.cursor/rules" "$AGENTS_DIR|.agents/skills"; do + parent="${entry%%|*}"; label="${entry#*|}" + dest="$parent/$out_name" + action "$label/$out_name/" + $DRY_RUN && continue + mkdir -p "$parent" + rm -rf "$dest" + cp -R "$knowledge_dir" "$dest" + done +} + +# Pre-0.3 installs put a copy of the domain's knowledge inside each skill directory. +# Remove it so an upgrade does not leave N stale duplicates behind the shared copy. +remove_per_skill_knowledge() { + local dest_dir="$1" label="$2" + [[ -e "$dest_dir/knowledge" ]] || return 0 + action "$label/knowledge/ (remove per-skill copy — now shared)" + $DRY_RUN || rm -rf "$dest_dir/knowledge" } copy_project_bundles() { local skill_dir="$1" out_name="$2" copy_bundle_dirs "$skill_dir" "$CLAUDE_DIR/$out_name" ".claude/skills/$out_name" - copy_domain_knowledge "$skill_dir" "$CLAUDE_DIR/$out_name" ".claude/skills/$out_name" + remove_per_skill_knowledge "$CLAUDE_DIR/$out_name" ".claude/skills/$out_name" copy_bundle_dirs "$skill_dir" "$CURSOR_DIR/$out_name" ".cursor/rules/$out_name" - copy_domain_knowledge "$skill_dir" "$CURSOR_DIR/$out_name" ".cursor/rules/$out_name" + remove_per_skill_knowledge "$CURSOR_DIR/$out_name" ".cursor/rules/$out_name" copy_bundle_dirs "$skill_dir" "$AGENTS_DIR/$out_name" ".agents/skills/$out_name" - copy_domain_knowledge "$skill_dir" "$AGENTS_DIR/$out_name" ".agents/skills/$out_name" + remove_per_skill_knowledge "$AGENTS_DIR/$out_name" ".agents/skills/$out_name" } expected_project_skill_contains() { @@ -438,14 +470,32 @@ copy_user_bundles() { local skill_dir="$1" out_name="$2" if [[ -d "$HOME/.claude" ]]; then copy_bundle_dirs "$skill_dir" "$USER_CLAUDE_DIR/$out_name" "~/.claude/skills/$out_name" - copy_domain_knowledge "$skill_dir" "$USER_CLAUDE_DIR/$out_name" "~/.claude/skills/$out_name" + remove_per_skill_knowledge "$USER_CLAUDE_DIR/$out_name" "~/.claude/skills/$out_name" fi if [[ -d "$HOME/.codex" ]]; then copy_bundle_dirs "$skill_dir" "$USER_CODEX_DIR/$out_name" "~/.codex/skills/$out_name" - copy_domain_knowledge "$skill_dir" "$USER_CODEX_DIR/$out_name" "~/.codex/skills/$out_name" + remove_per_skill_knowledge "$USER_CODEX_DIR/$out_name" "~/.codex/skills/$out_name" fi } +install_user_domain_knowledge() { + local domain_name="$1" domain_dir="$2" + local knowledge_dir="$domain_dir/knowledge" + [[ -d "$knowledge_dir" ]] || return 0 + local out_name; out_name=$(knowledge_out_name "$domain_name") + local entry parent label dest + for entry in "$USER_CLAUDE_DIR|~/.claude/skills|$HOME/.claude" "$USER_CODEX_DIR|~/.codex/skills|$HOME/.codex"; do + parent="${entry%%|*}"; label="${entry#*|}"; label="${label%%|*}" + [[ -d "${entry##*|}" ]] || continue + dest="$parent/$out_name" + action "$label/$out_name/" + $DRY_RUN && continue + mkdir -p "$parent" + rm -rf "$dest" + cp -R "$knowledge_dir" "$dest" + done +} + process_skill() { local skill_dir="$1" domain_name="$2" local skill_name; skill_name=$(basename "$skill_dir") @@ -514,6 +564,7 @@ process_skill() { write_user_claude "$out_name" "$out_name" "$description" "$body" write_user_codex "$out_name" "$out_name" "$description" "$body" copy_user_bundles "$skill_dir" "$out_name" + install_user_domain_knowledge "$domain_name" "$(cd "$skill_dir/../.." && pwd)" return fi @@ -537,6 +588,14 @@ ${overlay_content}" write_agents "$out_name" "$out_name" "$description" "$merged" copy_project_bundles "$skill_dir" "$out_name" EXPECTED_PROJECT_SKILLS+=("$out_name") + + # Shared per-domain knowledge. Guarded internally so it runs once per domain, + # not once per skill; registered as expected so --prune-stale leaves it alone. + local domain_dir; domain_dir=$(cd "$skill_dir/../.." && pwd) + if [[ -d "$domain_dir/knowledge" ]]; then + install_domain_knowledge "$domain_name" "$domain_dir" + EXPECTED_PROJECT_SKILLS+=("$(knowledge_out_name "$domain_name")") + fi } preflight_skill() { From af3ca6f2b70097a611930c404066ff57019c3290 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 12:33:39 -0400 Subject: [PATCH 2/6] Revert the shared-knowledge layout; guard reference resolution instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installing domain `knowledge/` once per domain deduplicated the delivered tree but stranded every skill-relative `knowledge/.md` citation — 12 working references in `domains/perps` alone. The installed tree is generated on every sync, so the duplication it removed was not worth a breaking layout change. `tools/install` is restored byte-for-byte to its previous behavior. What remains is the part that had value independent of the layout: - A regression guard: every `knowledge/…` reference in an emitted skill must resolve on disk after install. Verified to FAIL against the reverted design with `dangling knowledge reference knowledge/alpha.md`, and to pass here — a guard that cannot fire is not a guard. - Its fixture carries a real consumer (a skill body that cites a knowledge file), because the previous fixture had none and was structurally unable to exhibit the regression while every assertion passed. - README and CONTRIBUTING now state the rule the layout difference forces: cite knowledge by name or by the installed-relative path, never a repo-relative one, which is broken in the delivered output with nothing reporting it. --- CONTRIBUTING.md | 7 ++-- README.md | 28 ++++++------- test/cli.test.mjs | 83 ++++++++++++++++--------------------- tools/install | 101 ++++++++++------------------------------------ 4 files changed, 73 insertions(+), 146 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f1b93625..13b7b60a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -122,9 +122,10 @@ domains// knowledge/ # Optional: shared domain reference ``` -Domain `knowledge/` installs once per domain as `mms--knowledge/`, a sibling of the -domain's installed skills — a different shape from the repo's. **Cite knowledge files by -name, not by relative path**: no relative path resolves correctly in both layouts. See +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 diff --git a/README.md b/README.md index 85afec6b..f359f971 100644 --- a/README.md +++ b/README.md @@ -134,7 +134,7 @@ domains// scripts/ # optional helper scripts adapters/ # optional runtime payloads used by scripts repos/.md # optional repo-specific overlay - knowledge/ # optional shared domain reference, installed once as mms--knowledge/ + knowledge/ # optional shared domain reference, installed beside each domain skill tools/ install # core writer (mms- prefix, multi-operator output) sync # Flow 2: `yarn skills` wrapper for engineers @@ -145,27 +145,27 @@ tools/ ### Referring to domain knowledge from a skill -Domain `knowledge/` installs **once per domain**, as a sibling of the domain's skills: +`knowledge/` is copied **beside every skill in the domain**, so the installed tree is +flatter than this repo's: ``` -.claude/skills/mms-/SKILL.md -.claude/skills/mms-/references/… -.claude/skills/mms--knowledge/… # one copy, shared by every skill in the domain +repo domains//knowledge/x.md domains//skills//skill.md +installed .claude/skills/mms-/knowledge/x.md .claude/skills/mms-/SKILL.md ``` -That layout does not match the repo's, where `knowledge/` sits two levels above a skill and -three above its `references/`. **So cite knowledge files by name, never by relative path** — -no relative path is correct in both layouts, and one written against either will silently -break in the other: +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) +See the `selector-anti-patterns` knowledge file. +See [x](knowledge/selector-anti-patterns.md) +See [x](../../knowledge/selector-anti-patterns.md) ``` -Section anchors have the same problem for a different reason — they break the moment the -target file is reorganized. Name the file and the section in prose instead. +`test/cli.test.mjs` guards this: every `knowledge/…` reference in an emitted skill must +resolve on disk after install. ## Domains today diff --git a/test/cli.test.mjs b/test/cli.test.mjs index 4f916dd9..421e74f4 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, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -241,40 +241,46 @@ describe('managed skill pruning', () => { }); }); -describe('domain knowledge is installed once per domain', () => { +describe('installed knowledge references resolve', () => { let root; let source; let target; before(() => { - root = mkdtempSync(path.join(os.tmpdir(), 'mms-knowledge-')); + 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 }); - // One domain, two skills, two knowledge files. The pre-0.3 installer copied the - // knowledge dir into BOTH skills; the shared layout writes it once. + // 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'); - writeFileSync(path.join(knowledge, 'beta.md'), '# Beta\n'); - for (const name of ['first', 'second']) { - const dir = path.join(source, 'domains', 'testing', 'skills', name); - mkdirSync(dir, { recursive: true }); - writeFileSync( - path.join(dir, 'skill.md'), - ['---', `name: ${name}`, `description: Skill ${name}`, 'maturity: stable', '---', 'Body.'].join('\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('writes one shared knowledge dir per operator, not one per skill', () => { + test('every knowledge/ reference in an emitted skill resolves on disk', () => { const result = spawnSync( 'bash', [INSTALL, '--target', target, '--repo', 'core', '--source', source], @@ -282,43 +288,22 @@ describe('domain knowledge is installed once per domain', () => { ); assert.equal(result.status, 0, result.stderr); - for (const base of ['.claude/skills', '.cursor/rules', '.agents/skills']) { - const shared = path.join(target, base, 'mms-testing-knowledge'); - assert.ok(existsSync(path.join(shared, 'alpha.md')), `${base} missing shared alpha.md`); - assert.ok(existsSync(path.join(shared, 'beta.md')), `${base} missing shared beta.md`); - // and NOT duplicated into each skill - for (const name of ['mms-first', 'mms-second']) { + 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(target, base, name, 'knowledge')), - `${base}/${name} should not carry a per-skill knowledge copy`, + existsSync(path.join(skillDir, ref)), + `${base}/${name}: dangling knowledge reference ${ref} — the body cites it but install did not place it there`, ); } } }); - - test('upgrading removes a per-skill knowledge copy left by an older install', () => { - const stalePath = path.join(target, '.claude/skills', 'mms-first', 'knowledge'); - mkdirSync(stalePath, { recursive: true }); - writeFileSync(path.join(stalePath, 'alpha.md'), '# stale\n'); - assert.ok(existsSync(path.join(stalePath, 'alpha.md'))); - - const result = spawnSync( - 'bash', - [INSTALL, '--target', target, '--repo', 'core', '--source', source], - { encoding: 'utf8' }, - ); - assert.equal(result.status, 0, result.stderr); - assert.ok(!existsSync(stalePath), 'stale per-skill knowledge dir should be removed on upgrade'); - assert.ok(existsSync(path.join(target, '.claude/skills', 'mms-testing-knowledge', 'alpha.md'))); - }); - - test('--prune-stale keeps the shared knowledge dir', () => { - const result = spawnSync( - 'bash', - [INSTALL, '--target', target, '--repo', 'core', '--source', source, '--prune-stale'], - { encoding: 'utf8' }, - ); - assert.equal(result.status, 0, result.stderr); - assert.ok(existsSync(path.join(target, '.claude/skills', 'mms-testing-knowledge', 'alpha.md'))); - }); }); diff --git a/tools/install b/tools/install index 1c605dcb..e39404aa 100755 --- a/tools/install +++ b/tools/install @@ -364,66 +364,34 @@ copy_bundle_dirs() { done } -# Domain knowledge installs ONCE per domain, to its own `${PREFIX}-knowledge/` -# directory, rather than once per skill. -# -# It used to be copied into every skill directory in the domain, which meant a domain -# with K knowledge files and N skills wrote K*N copies per operator — `performance` -# alone lands 7*8 = 56 — and made "one source" true in the repo but not on disk. -# -# The shared directory also fixes cross-layer references. Per-skill copies sat as -# siblings of `references/`, three levels apart from where they live in the repo, so -# `../../../knowledge/x.md` resolved in the repo and broke once installed while -# `../knowledge/x.md` did the reverse — no relative path was correct in both. Skills -# therefore cite knowledge files BY NAME, and the shared directory is the one place -# every operator resolves that name to. -KNOWLEDGE_DONE="" - -knowledge_out_name() { - printf '%s%s-knowledge' "$PREFIX" "$1" -} - -install_domain_knowledge() { - local domain_name="$1" domain_dir="$2" +copy_domain_knowledge() { + local skill_dir="$1" dest_dir="$2" label="$3" + local domain_dir; domain_dir=$(cd "$skill_dir/../.." && pwd) local knowledge_dir="$domain_dir/knowledge" - [[ -d "$knowledge_dir" ]] || return 0 - - local out_name; out_name=$(knowledge_out_name "$domain_name") - # Once per domain per run, however many skills the domain ships. - case " $KNOWLEDGE_DONE " in - *" $domain_name "*) return 0 ;; - esac - KNOWLEDGE_DONE="$KNOWLEDGE_DONE $domain_name" - - local entry parent label dest - for entry in "$CLAUDE_DIR|.claude/skills" "$CURSOR_DIR|.cursor/rules" "$AGENTS_DIR|.agents/skills"; do - parent="${entry%%|*}"; label="${entry#*|}" - dest="$parent/$out_name" - action "$label/$out_name/" - $DRY_RUN && continue - mkdir -p "$parent" - rm -rf "$dest" - cp -R "$knowledge_dir" "$dest" - done -} -# Pre-0.3 installs put a copy of the domain's knowledge inside each skill directory. -# Remove it so an upgrade does not leave N stale duplicates behind the shared copy. -remove_per_skill_knowledge() { - local dest_dir="$1" label="$2" - [[ -e "$dest_dir/knowledge" ]] || return 0 - action "$label/knowledge/ (remove per-skill copy — now shared)" - $DRY_RUN || rm -rf "$dest_dir/knowledge" + if [[ -d "$knowledge_dir" ]]; then + action "$label/knowledge/" + $DRY_RUN && return + mkdir -p "$dest_dir" + rm -rf "$dest_dir/knowledge" + cp -R "$knowledge_dir" "$dest_dir/knowledge" + else + if $DRY_RUN; then + [[ -e "$dest_dir/knowledge" ]] && action "$label/knowledge/ (remove stale)" + return 0 + fi + rm -rf "$dest_dir/knowledge" + fi } copy_project_bundles() { local skill_dir="$1" out_name="$2" copy_bundle_dirs "$skill_dir" "$CLAUDE_DIR/$out_name" ".claude/skills/$out_name" - remove_per_skill_knowledge "$CLAUDE_DIR/$out_name" ".claude/skills/$out_name" + copy_domain_knowledge "$skill_dir" "$CLAUDE_DIR/$out_name" ".claude/skills/$out_name" copy_bundle_dirs "$skill_dir" "$CURSOR_DIR/$out_name" ".cursor/rules/$out_name" - remove_per_skill_knowledge "$CURSOR_DIR/$out_name" ".cursor/rules/$out_name" + copy_domain_knowledge "$skill_dir" "$CURSOR_DIR/$out_name" ".cursor/rules/$out_name" copy_bundle_dirs "$skill_dir" "$AGENTS_DIR/$out_name" ".agents/skills/$out_name" - remove_per_skill_knowledge "$AGENTS_DIR/$out_name" ".agents/skills/$out_name" + copy_domain_knowledge "$skill_dir" "$AGENTS_DIR/$out_name" ".agents/skills/$out_name" } expected_project_skill_contains() { @@ -470,32 +438,14 @@ copy_user_bundles() { local skill_dir="$1" out_name="$2" if [[ -d "$HOME/.claude" ]]; then copy_bundle_dirs "$skill_dir" "$USER_CLAUDE_DIR/$out_name" "~/.claude/skills/$out_name" - remove_per_skill_knowledge "$USER_CLAUDE_DIR/$out_name" "~/.claude/skills/$out_name" + copy_domain_knowledge "$skill_dir" "$USER_CLAUDE_DIR/$out_name" "~/.claude/skills/$out_name" fi if [[ -d "$HOME/.codex" ]]; then copy_bundle_dirs "$skill_dir" "$USER_CODEX_DIR/$out_name" "~/.codex/skills/$out_name" - remove_per_skill_knowledge "$USER_CODEX_DIR/$out_name" "~/.codex/skills/$out_name" + copy_domain_knowledge "$skill_dir" "$USER_CODEX_DIR/$out_name" "~/.codex/skills/$out_name" fi } -install_user_domain_knowledge() { - local domain_name="$1" domain_dir="$2" - local knowledge_dir="$domain_dir/knowledge" - [[ -d "$knowledge_dir" ]] || return 0 - local out_name; out_name=$(knowledge_out_name "$domain_name") - local entry parent label dest - for entry in "$USER_CLAUDE_DIR|~/.claude/skills|$HOME/.claude" "$USER_CODEX_DIR|~/.codex/skills|$HOME/.codex"; do - parent="${entry%%|*}"; label="${entry#*|}"; label="${label%%|*}" - [[ -d "${entry##*|}" ]] || continue - dest="$parent/$out_name" - action "$label/$out_name/" - $DRY_RUN && continue - mkdir -p "$parent" - rm -rf "$dest" - cp -R "$knowledge_dir" "$dest" - done -} - process_skill() { local skill_dir="$1" domain_name="$2" local skill_name; skill_name=$(basename "$skill_dir") @@ -564,7 +514,6 @@ process_skill() { write_user_claude "$out_name" "$out_name" "$description" "$body" write_user_codex "$out_name" "$out_name" "$description" "$body" copy_user_bundles "$skill_dir" "$out_name" - install_user_domain_knowledge "$domain_name" "$(cd "$skill_dir/../.." && pwd)" return fi @@ -588,14 +537,6 @@ ${overlay_content}" write_agents "$out_name" "$out_name" "$description" "$merged" copy_project_bundles "$skill_dir" "$out_name" EXPECTED_PROJECT_SKILLS+=("$out_name") - - # Shared per-domain knowledge. Guarded internally so it runs once per domain, - # not once per skill; registered as expected so --prune-stale leaves it alone. - local domain_dir; domain_dir=$(cd "$skill_dir/../.." && pwd) - if [[ -d "$domain_dir/knowledge" ]]; then - install_domain_knowledge "$domain_name" "$domain_dir" - EXPECTED_PROJECT_SKILLS+=("$(knowledge_out_name "$domain_name")") - fi } preflight_skill() { From 6f28987d65922c393826f8fae5b5f8ff49aecedb Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 12:56:27 -0400 Subject: [PATCH 3/6] Check the shipped corpus, not just the installer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fixture guard proves `tools/install` places knowledge where a skill body expects it. It says nothing about whether the skills in this repo cite files their own domain actually ships — and six citations do not. `knowledge/` is copied per domain, so a skill can only cite its own domain's files. Four skills in `coding`, `perps`, and `pr-workflow` cite `knowledge/testing-layers.md`, which lives in `domains/testing/`. The installer has no way to deliver it into those domains, so the reference cannot resolve for any consumer on any operator. Five of the six sit in `repos/metamask-mobile.md` overlays, which is likely why they went unnoticed. Those six are listed in `KNOWN_UNRESOLVED` so the check lands green and blocks new breakage rather than merging red. A second test fails if an entry starts resolving, so the list can only shrink. Both directions verified to fire: a new dangling citation fails the first test naming the offending pair, and satisfying a listed citation fails the second. --- test/cli.test.mjs | 67 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/test/cli.test.mjs b/test/cli.test.mjs index 421e74f4..57f4ab23 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, readFileSync, 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'; @@ -307,3 +307,68 @@ describe('installed knowledge references resolve', () => { } }); }); + +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'); + }); +}); From c21c785520fe67128e22911093cb1e98eab358f3 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 13:45:12 -0400 Subject: [PATCH 4/6] Gate personal references and frozen-branch links in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two checks that found real defects by hand, now standing. Both run under `yarn test`, need no network, and pass on the current corpus, so they gate new breakage rather than landing red. Personal references — this repo is public, so an absolute home path, a personal handle, or a private-repo name is both a leak and a reference no reader but its author can resolve. The path pattern is anchored to a boundary; an unanchored one matches `../pages/home/homepage`. Frozen-branch links — `metamask-extension` moved its default to `main`, but `develop` still exists with a last commit of 2026-01-15. Links to it load and serve stale source, which is worse than a 404 because nothing signals the age. `FROZEN_BRANCHES` is a list so more can be added as branches are retired. Both verified to fire on an injected violation, naming file, line, and reason. Deliberately not gated: requiring every `/blob//` link to be SHA-pinned fires 19 times on existing content, and is the wrong rule anyway — a directory listing should track the default branch. Pin when a link is evidence for a claim; track the branch when it is a place to look. --- test/cli.test.mjs | 57 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/test/cli.test.mjs b/test/cli.test.mjs index 57f4ab23..ed746cb0 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -372,3 +372,60 @@ describe('corpus: knowledge citations resolve within their own domain', () => { 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; + } + + // 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. + test('no personal paths, handles, or private-repo references', () => { + const PERSONAL = [ + [/(^|[\s"'`(])\/(home|Users)\/[a-z][a-z0-9_.-]*/u, 'absolute personal path'], + [/\b(majorlift|MajorLift)\b/u, 'personal handle'], + [/\bexogram[-a-z]*/u, 'private repo'], + [/metamask-extension-skills/u, 'personal repo'], + [/consensys-test\//u, 'personal fork'], + ]; + 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'); + }); +}); From c340a0d50c2daed3e8638998a7c42a177b0c96f5 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 06:11:57 -0400 Subject: [PATCH 5/6] Take the private identifiers out of the privacy test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The denylist named five of them — a home path, a handle, and three repository names — committed to a public repository. A denylist of private identifiers publishes every identifier it protects, so the test leaked precisely what it existed to prevent, and did so more completely than any single skill file had. Structural patterns describe a shape and stay inline: an absolute `/home` or `/Users` path, an ssh remote. Anything naming a particular person, host or repository now comes from `SKILLS_PRIVATE_PATTERNS` — a newline-separated list of regex sources supplied by CI secret or an untracked local file, so the corpus is checked without the corpus being published. A generic email pattern was tried and dropped: it fired on a third-party address in oh-my-opencode's documented config, which is a documentation example rather than a leak. Identity-shaped patterns belong in the configured list, where the person who owns the identity decides. Two-arm verified: fails on a planted reference with the pattern configured, passes with the reference removed. --- test/cli.test.mjs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/cli.test.mjs b/test/cli.test.mjs index ed746cb0..32ebb14e 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -393,13 +393,22 @@ describe('corpus: content is safe to publish and links stay current', () => { // 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'], - [/\b(majorlift|MajorLift)\b/u, 'personal handle'], - [/\bexogram[-a-z]*/u, 'private repo'], - [/metamask-extension-skills/u, 'personal repo'], - [/consensys-test\//u, 'personal fork'], + [/\bgit@[a-z0-9.-]+:[^\s]+/u, 'ssh remote'], + ...(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()) { From 896d5502f321cc457e3d4555daae1d3cda23e68a Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 06:25:46 -0400 Subject: [PATCH 6/6] Derive the identity patterns instead of requiring them to be supplied MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving the denylist to `SKILLS_PRIVATE_PATTERNS` stopped the leak and stopped the check: nothing sets that variable, so the identifier arm matched nothing anywhere. A check that cannot fire is not a weaker check, it is an absent one wearing the name of a check. The identifiers most likely to leak belong to whoever is running, and the environment already knows who that is — `GITHUB_ACTOR` in CI, `USER` and `git config user.name` locally. Values shorter than four characters or on a generic list (runner, ubuntu, ci, node…) are dropped, since a two-letter username matches every file. `SKILLS_PRIVATE_PATTERNS` remains for anything else worth catching. Two-arm verified with no environment configured: fails on a planted reference to the running user's handle, passes with it removed. --- test/cli.test.mjs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/cli.test.mjs b/test/cli.test.mjs index 32ebb14e..b0fe8210 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -391,6 +391,22 @@ describe('corpus: content is safe to publish and links stay current', () => { 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. // @@ -404,6 +420,15 @@ describe('corpus: content is safe to publish and links stay current', () => { 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())