test(ci): Enforce that knowledge/ installations resolve - #87
Open
MajorLift wants to merge 4 commits into
Open
Conversation
`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.
knowledge/ once per domain, not once per skillThe 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.
knowledge/ references resolve on disk
knowledge/ references resolve on diskknowledge/ installations resolve
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.
MajorLift
marked this pull request as ready for review
July 30, 2026 18:03
This was referenced Jul 30, 2026
There was a problem hiding this comment.
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>.mdor 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. indomains/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 bareknowledge/<file>.mdoccurrences.
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.
Comment on lines
+298
to
+300
| 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`); |
Comment on lines
+285
to
+287
| 'bash', | ||
| [INSTALL, '--target', target, '--repo', 'core', '--source', source], | ||
| { encoding: 'utf8' }, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Guards a property that breaks silently: every
knowledge/…reference in an installed skill must resolve on disk.The hazard
Domain
knowledge/is copied beside every skill in the domain, so an installed skill body reaches it asknowledge/<file>.md. In this repo the same file sits two levels above the skill. The two layouts disagree:So a repo-relative citation (
../../knowledge/x.md) is correct in the source tree and broken in the delivered output — and nothing reports it. Neither does the reverse. Any change to where the installer places knowledge has the same effect on every skill that cites it: the reference dangles, the skill still installs, and the agent quietly cannot find what the body points at.domains/perpsalone carries 13 such citations.What ships
A guard (
test/cli.test.mjsL244–L309, commit-pinned) — installs a domain and asserts that everyknowledge/…reference in each emitted skill resolves on disk, across all three operator outputs.Its fixture carries a real consumer: a skill body that cites a knowledge file the way shipped skills do. This is the load-bearing part. A fixture without a citation cannot exhibit a placement regression at all — every assertion in it passes while delivered references break.
A corpus check — the guard above proves the installer places knowledge correctly. It says nothing about whether the skills in this repo cite files their own domain actually ships. A second test sweeps every
knowledge/…citation indomains/and fails on any that its domain cannot deliver.It found six that already dangle, all one bug: four skills in
coding,perps, andpr-workflowciteknowledge/testing-layers.md, which lives indomains/testing/. Cross-domain knowledge has no delivery mechanism, so those references cannot resolve for any consumer. They are listed inKNOWN_UNRESOLVEDso this lands green and blocks new breakage instead of merging red; a companion test fails if a listed citation starts resolving, so the list can only shrink. Tracked in #88.Docs — README and CONTRIBUTING state the rule the layout difference forces: cite knowledge by name, or by the installed-relative path, never a repo-relative one.
Evidence
A guard that cannot fire is not a guard. Run against an installer that places knowledge somewhere other than beside the skill, it fails with the dangling reference named:
Notes
Appendix — why not a symlink
Knowledge is copied beside every skill, so a domain with K files and N skills delivers K×N —
perpsships 108 where 27 would do. A per-skill symlink into one shared directory is the obvious fix. It does not hold up, on three counts.It deduplicates nothing that matters. A recursive walk following links sees the content once per skill plus once at the shared location:
Same N× duplication a copy produces, for an agent reading the tree. The symlink saves disk — the cheap resource, regenerated every sync — and saves nothing in the context window, the scarce one.
Relocation strands it. The link is relative, so copying an installed skill anywhere else breaks it:
Copying a single
mms-<skill>/directory elsewhere is an ordinary thing to do.Windows fails as a wrong-type file, not an error. Git stores symlinks as mode
120000; where symlink creation is unavailable — no Developer Mode, no elevation,core.symlinks=false— git checks out a regular file containing the target path.knowledgebecomes a file, soknowledge/alpha.mdis untraversable whileexistsSyncon the parent returns true. Not hypothetical for every consumer:coredoes not ignore the install target, so output there is committable and can reach a Windows contributor through git.metamask-extensiondoes ignore it.This repo ships no symlinks —
git ls-files -sreports no mode-120000entries — so it would be a new artifact class in the delivered tree.Conclusion. Copying is right: the installed tree is generated, the duplication costs only disk, and every alternative trades that for a silent failure mode. If deduplication is revisited, rewriting citations as the installer emits each skill is the safer direction — it changes text, not filesystem semantics, and the guard here would catch it going wrong.