fix(cli): Ensure `knowledge/` citations and cross-skill references resolve by MajorLift · Pull Request #87 · MetaMask/skills

MajorLift · 2026-07-30T16:13:52Z

Closes #88 (skills cite knowledge files their domain does not ship).

Every knowledge/… reference in an installed skill body resolves on disk. The installer now delivers knowledge a skill cites from another domain, and tests keep it that way.

The hazard

Installed skills get knowledge/ beside them; this repo keeps it two levels up. Before this change that broke two ways, silently.

Wrong shape. A repo-relative citation (../../knowledge/x.md) resolves here and 404s once installed. domains/perps/skills alone carries 13 knowledge citations, so any change to where the installer puts knowledge breaks them all with nothing reporting it.

Never delivered. Knowledge was copied per domain, so a skill citing another domain's file got nothing. Six sites across four skills in coding, perps and pr-workflow cite knowledge/testing-layers.md, which lives in domains/testing/. Five sit in repos/metamask-mobile.md overlays, which is plausibly why it went unnoticed.

What ships

resolve_foreign_knowledge scans each skill — body and repos/… overlays, since three of the four affected skills cite from an overlay — and copies in any cited file its own domain cannot satisfy. Existing citations start working unchanged; nothing new to declare. Resolution is by filename across the eleven knowledge files in domains/; an ambiguous name exits non-zero and names the candidates rather than guessing.

Guards in test/cli.test.mjs: the installer places every cited file; the corpus cites only what each domain ships; no personal path, handle or private-repo name reaches a public skill; no link points into a frozen branch.

Also here, from #103 (ci: validate cross-skill references in lint-skill-entry): bare lane ids, [[wiki]] links, unresolvable ## Related entries, plus check-public-refs and skill-audit.

Evidence

One scenario — --repo metamask-mobile --domain coding,perps,pr-workflow,testing — counting every knowledge/… site in emitted skill bodies. Only tools/install varies, and each arm asserts which blob it ran.

`tools/install` from	`.claude/skills`	`.agents/skills`
merge base `cce2ee1`, whose blob is `main`'s today	3 / 9	3 / 9
this branch	9 / 9	9 / 9

.cursor/rules emits no knowledge citations, so it is 0 / 0 in both arms.

The suite on this branch is 84 / 84 at 55b6d4d — 65 on main, plus the guards this branch adds. Each guard was then defeated at its subject — stubbing resolve_foreign_knowledge, stubbing copy_domain_knowledge, planting a foreign citation, a repo-relative path, a personal path, a develop link, and making a listed citation resolve. Seven mutations, nine assertions, every one fired and named the planted fault; baseline zero.

The corpus check caught a real violation on main. #130 (feat(testing): add unified extension-testing skill) added a skill citing a second knowledge file repo-relatively — one test failed naming four citations. 4797d98 fixes them the same way 004d5da fixed testing-layers.md.

Notes

KNOWN_UNRESOLVED keeps the testing-layers.md entries even though the installer now delivers them. Delivery and citation hygiene are different properties: the corpus rule is a source-tree convention, so a citation reads without knowing the installer. Emptying it means moving the file or dropping the convention — both calls for #88, not this PR. A companion test fails on any entry that starts resolving, so the list can only shrink.

Scope. Both the scan and the guard read a skill's emitted body. A citation inside a delivered knowledge/ file is out of scope — domains/perps/knowledge/review-antipatterns.md L111 still dangles. Widening it is a one-line change to sources=(…), better as a follow-up.

Copying, not symlinking — the installed tree is regenerated every sync, so duplication costs only disk, while a symlink strands a relocated skill and checks out on Windows as a regular file whose parent existsSync returns true. Say the word if you want a CHANGELOG.md entry and which release.

Three reference kinds could name something no reader can reach, and none were checked: - Lane ids (`B7`, `C4`) are addresses into `evidence-catalog.md`, not names. A `description` cannot link out to the catalog at all, so a lane id there is unresolvable by construction — that is an error. In the body it is a warning unless the line links the catalog. - `[[snake_case]]` wiki links come from a private authoring vault and render as literal brackets here. Matching on the underscore keeps JS array literals (`[[signer1.address, …]]`) from tripping the rule. - Names in `## Related` must resolve to a skill. A warning rather than an error: the gate runs on the PR's own branch, where a sibling skill shipping in a concurrent PR does not exist yet. `collectSkills` takes an array of roots; passing a bare string iterates its characters and yields zero skills, which would have made every check above pass vacuously.

Copilot

Pull request overview

Adds CI-level protections and documentation to prevent silent breakage where installed skills reference knowledge/… files that aren’t actually present in the installed output, and to validate that in-repo knowledge/… citations resolve within their own domain.

Changes:

Adds an install-time guard test that installs a fixture domain and asserts knowledge/… references in emitted skill outputs resolve on disk.
Adds a corpus-wide test that scans skills for knowledge/… citations and fails on any that the skill’s own domain cannot deliver (with an allowlist for known cross-domain cases).
Documents the correct way to refer to domain knowledge files in skills (installed-relative knowledge/<file>.md or by name; never repo-relative).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
test/cli.test.mjs	Adds new test guards for installed knowledge reference resolution and corpus validation of knowledge citations.
README.md	Documents the knowledge layout difference between repo and installed output and the safe citation formats.
CONTRIBUTING.md	Adds contributor guidance linking to the README rule for knowledge citations.

Suppressed comments (1)

test/cli.test.mjs:340

The corpus citation sweep currently only detects knowledge references in Markdown links or backticks. There are existing plain-text references like knowledge/testing-layers.md (e.g. in domains/testing/skills/unit-testing/skill.md) that won’t be validated, so new dangling citations in that format could slip through. Expand the matcher to also catch bare knowledge/<file>.md occurrences.

          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}` });
          }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

MajorLift · 2026-08-12T22:07:13Z

+      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`);


Fixed: 7adfa62

…nymously This repo is public, so naming a repository discloses that it exists, who owns it and roughly what it holds. A prohibition discloses exactly as much as a recommendation — "do not re-host to <personal repo>, it is private" publishes the name either way — so the rule is about the mention, not the sentiment attached to it. A skill shipped with a personal repo named in a warning, on that reasoning. The check is a request rather than a list. An owner allowlist was tried first and was wrong on its first run: it flagged `nock/nock` and `phishfort/phishfort-lists`, because "is this owner well known" is not the property that matters. The property is whether a reader who is not you can open the link, and an unauthenticated HEAD answers it exactly. Deliberately unauthenticated — a token would see private repos and pass them. `MetaMask` and `Consensys` are exempt: org repos are unreachable publicly but readable by colleagues, and naming them is a deliberate call rather than a leak. Two-arm verified: fires on `MajorLift/Reprise` in the tree that shipped it, silent on the tree with it removed. The first version scanned only skill.md and would have passed the violation that motivated it, which was in a references/ file.

Scoped to the whole repository rather than the PR's changed files: whether this repo names something a reader cannot open is a property of what it publishes, not of what a given PR touched, and the reference that motivated this had sat unnoticed through several PRs that edited the same file. `GH_TOKEN` and `GITHUB_TOKEN` are blanked for the step. The check must run as a stranger — an authenticated request resolves private repositories and passes them, which is the exact failure it exists to catch.

"Did the right skill load" is a question about a probabilistic event — a description matched by a model — so the only honest answer comes from the transcript rather than from the description. This reads one and reports what entered context, by route. Three routes leave three different traces (Skill tool call, slash command, and the loader's base-directory announcement on a description match). Counting one of them reads as silence: run against the session that produced this file, the Skill-tool count alone suggests the reasoning skills were used, and all three together show that not one of them loaded at any point. The second report is the deterministic one. A publish is UNGATED if no gate ran before it at all, and UNCHAINED if a gate ran earlier but not as the same command. The distinction is the finding: a gate that merely ran earlier proves nothing, because the verdict can be read after the write — which is exactly how a blocked artifact reached a public pull request in the session this was written from. Only `gate && publish` makes the shell enforce the dependency. The first version reported that session as clean on the generous rule. It is 195 unchained publishes on the strict one.

…sees The lane-id, wiki-link and `## Related` checks had no cases in a suite that already exercises this script heavily, so each was one edit away from silently matching nothing. Ten cases, each shown to fail against the behaviour it claims to guard rather than merely to pass. Six mutations, five caught by exactly the test that names them: offset zeroed, catalog-link suppression removed, catalog-owner exemption removed, wiki regex loosened to any token, `## Related` running to EOF. The sixth is the interesting one. Deleting `&& name !== skill.name` changes nothing, because the linted skill is collected from the same tree as the known-name set — its own name is always in that set, so the clause never decides the case it appears to. That test is kept for the behaviour and its comment now says which mechanism actually delivers it. The offset assertion derives the expected line by scanning the written file, not by repeating the linter's arithmetic. The frontmatter it strips is exactly what shifts the numbers, so a test that recomputed the offset the same way would agree with the defect it exists to catch. 71 pass / 0 fail; repo lint 0 errors.

`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-<domain>-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.

Installing domain `knowledge/` once per domain deduplicated the delivered tree but stranded every skill-relative `knowledge/<file>.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.

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.

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/<branch>/` 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.

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.

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.

The guard read the delimiter, not the citation. Anchoring `knowledge/` to a preceding `](` or backtick matched two of the three forms skills actually use, and — because the `../` sits between the delimiter and the path — none of the repo-relative ones. That is the form the README calls broken and says this test guards, so the one defect the check existed to catch was the one it could not express. `perps-write-ticket` carried two, under a heading reading "read installed". Both 404 in the installed tree. Fixed here; the check now reports them. Consuming the `../` prefix is what makes them visible. Each repo-relative path ends in a correct-looking `knowledge/<file>.md`, so a matcher that skips the prefix resolves the suffix, finds the file, and passes the citation it should reject — broadening the matcher without capturing the prefix would have hidden these two a second way. The fixture cited one file, as a Markdown link — the form the matcher already handled, so it agreed with the blind spot rather than exposing it. It now cites all three forms, and the assertion is the set found, not `length > 0`: a narrowed matcher returns a subset whose every member resolves, which the old assertion passed. Verified by restoring the link-only matcher: `alpha` found, `beta` and `gamma` missed, test red. Also pins `/bin/bash` at the one call site that reached it through PATH; the other four in this file already did. 47 pass / 0 fail.

The `mobile-testing` skill reaches the knowledge file as `../../knowledge/testing-layers.md`. That path resolves while browsing the repo and 404s in the installed tree, which is the only tree a reader gets. Each site already names the installed form beside it, so the source pointer carries nothing the reader can follow.

Absorbs #94. Knowledge is copied per domain, so a skill citing another domain's file received nothing — four skills across coding, perps and pr-workflow cite testing-layers.md, which lives in testing. resolve_foreign_knowledge() delivers it. The corpus test and the installer fix belong together: the test names those four in KNOWN_UNRESOLVED, and leaving them in separate PRs meant whichever landed second had to reconcile a list the other branch could not see. Its rationale is corrected here rather than the list emptied, because the two are different properties. The corpus rule is a source-tree convention — cite your own domain's knowledge, so the citation reads without knowing the installer. Delivery is what the operator receives. After this commit all four resolve for an operator and still break the convention, and the old comment claimed they could never resolve for any consumer or operator — an impossibility claim falsified by the same change that carried it. Falsifiable: stubbing resolve_foreign_knowledge fails both cross-domain tests; restoring passes 21/21. Suite 74/74, lint 0 errors.

main added extension-testing (#130) with two repo-relative knowledge citations — `../../knowledge/extension-testing-layers.md` and a three-level variant — which are correct in this repo and 404 once installed. Rewritten to the installed-relative form, the same fix this branch already applied to mobile-testing. This is the corpus check finding a real violation rather than a regression: the branch was verified at 74/74 against a base six commits stale, and CI ran it against a main where the violation existed. Suite 74/74 merged with main; lint 52 skills, 0 errors.

Absorbs #103. No file overlap with this branch and no conflict — the two were adjacent in theme rather than entangled, unlike #94, which shared test/cli.test.mjs and the same four citations. Brings three reference checks in the linter: bare lane ids cited without linking the catalog that defines them, [[snake_case]] wiki links from a private authoring vault, and `## Related` entries naming a skill that does not exist on the branch. Plus check-public-refs, which asserts every repository named in the corpus resolves anonymously, and skill-audit. Scope note for reviewers: the lane check has no corpus subject today. Zero matches across 166 skill markdown files here, and zero across 256 non-evidence files on #84's branch, because evidence-catalog.md — the only place [A-G]N vocabulary exists — lives on #84. It is fixture-tested and prospective; it fires the day a non-evidence skill cites a lane. Suite 84/84; lint 52 skills, 0 errors; check-public-refs 6/6 resolve. Falsifiable: stubbing LANE_ID fails 2 lint tests, restoring passes 33/33.

MajorLift changed the title ~~fix(cli): install domain knowledge/ once per domain, not once per skill~~ test(cli): guard that installed knowledge references resolve Jul 30, 2026

MajorLift changed the title ~~test(cli): guard that installed knowledge references resolve~~ test(ci): Enforce that knowledge/ references resolve on disk Jul 30, 2026

MajorLift changed the title ~~test(ci): Enforce that knowledge/ references resolve on disk~~ test(ci): Enforce that knowledge/ installations resolve Jul 30, 2026

MajorLift marked this pull request as ready for review July 30, 2026 18:03

MajorLift requested review from abretonc7s and Copilot August 3, 2026 09:46

Copilot started reviewing on behalf of MajorLift August 3, 2026 09:47 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

MajorLift mentioned this pull request Aug 4, 2026

[DO-NOT-MERGE] trial branch: all skill PRs merged, for install and testing #108

Draft

MajorLift added 12 commits August 4, 2026 06:08

MajorLift force-pushed the jongsun/fix/knowledge-installs-once-per-domain branch from e317b17 to 004d5da Compare August 12, 2026 20:15

MajorLift added 2 commits August 20, 2026 06:46

MajorLift changed the title ~~test(ci): Enforce that knowledge/ installations resolve~~ fix(cli): deliver cross-domain knowledge, and enforce that citations resolve Aug 20, 2026

MajorLift changed the title ~~fix(cli): deliver cross-domain knowledge, and enforce that citations resolve~~ fix(cli): make every knowledge/ citation in an installed skill body resolve Aug 20, 2026

MajorLift changed the title ~~fix(cli): make every knowledge/ citation in an installed skill body resolve~~ fix(cli): make skill references resolve — cross-domain knowledge delivery, and CI validation for the rest Aug 20, 2026

MajorLift changed the title ~~fix(cli): make skill references resolve — cross-domain knowledge delivery, and CI validation for the rest~~ fix(cli): make skill references resolve Aug 20, 2026

MajorLift changed the title ~~fix(cli): make skill references resolve~~ fix(cli): Ensure knowledge/ citations and cross-skill references resolve Aug 20, 2026

MajorLift mentioned this pull request Aug 20, 2026

ci: validate cross-skill references in lint-skill-entry #103

Closed

MajorLift changed the title ~~fix(cli): Ensure knowledge/ citations and cross-skill references resolve~~ fix(cli): make skill references resolve Aug 20, 2026

MajorLift changed the title ~~fix(cli): make skill references resolve~~ fix(cli): Ensure knowledge/ citations and cross-skill references resolve Aug 20, 2026

Navigation Menu

Uh oh!

Uh oh!

fix(cli): Ensure `knowledge/` citations and cross-skill references resolve - #87

fix(cli): Ensure `knowledge/` citations and cross-skill references resolve#87
MajorLift wants to merge 16 commits into
mainfrom
jongsun/fix/knowledge-installs-once-per-domain

MajorLift commented Jul 30, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

MajorLift Aug 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

MajorLift commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The hazard

What ships

Evidence

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

MajorLift Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MajorLift commented Jul 30, 2026 •

edited

Loading