ci: add lint-skill-entry structural validator - #47
Merged
Conversation
MajorLift
marked this pull request as ready for review
June 17, 2026 13:28
NicolasMassart
requested changes
Jun 18, 2026
NicolasMassart
left a comment
Contributor
There was a problem hiding this comment.
Nice direction — reusing collectSkills / parseFrontmatter is the right shape, since the gate validates exactly what ships. Heads-up: the new workflow is currently failing on this branch and needs a fix before merge.
Adds a CI gate that validates skill contributions against the documented schema, reusing the installer's own parser (collectSkills / parseFrontmatter) so it checks what actually ships rather than a parallel model. - tools/skill-schema.mjs: single source for the frontmatter fields, maturity vocabulary, bundle-dir allowlist, description ceiling, and name pattern. - .github/scripts/lint-skill-entry.mjs: errors on layout/schema violations, warns on advisory issues. On-demand-only contract bans alwaysApply, and the sibling-dir allowlist rejects knowledge/. - .github/workflows/lint-skill-entry.yml: gates PRs on changed skills only, so pre-existing drift never blocks an unrelated change. - test/lint-skill-entry.test.mjs: fixture tests for pass and each violation.
MajorLift
force-pushed
the
jongsun/ci/260609-lint-skill-entry
branch
from
July 22, 2026 16:29
be70ac4 to
d026341
Compare
This was referenced Jul 27, 2026
MajorLift
marked this pull request as draft
July 30, 2026 14:02
… installer Running the validator against `main` turned up three errors, two of which were defects in the repo rather than in the check. `workflows/` held 14 files across two `web3-tools` skills, referenced 22 times from their bodies, and was absent from the installer's bundle list — so every installed copy carried 22 dangling links and nothing reported it. Adding it to `copy_bundle_dirs` makes all 22 resolve. This is the third instance of one root cause, after `pr-validate`'s `hooks/` and domain `knowledge/`: a directory that exists in source and is not in the copy list. So the list stops being written down twice. Two tests now read `tools/install` directly — one asserting `BUNDLE_DIRS` matches the directories it copies, one asserting every key it reads via `frontmatter_value` is declared. The comment asking a human to keep them in sync is now checked. `metadata` was rejected as unknown frontmatter though README documents it as preserved through install; it joins the optional set. Knowledge files get their own declared keys, since they take `domain` rather than the installer-behaviour keys skills use. The `performance` description was 1078 characters against the 1024 operator ceiling; trimmed to 928 without dropping a trigger or a covered topic. Validator now reports 0 errors across 46 skills.
The validator covers the deterministic half of what makes a skill acceptable — layout, name pattern, frontmatter keys, maturity, description length. It says nothing about the two questions that decide whether a skill should exist, and those are currently answered ad hoc in review. The first is whether the thing is a skill at all. Skills shape generation; linters, hooks, and fitness functions enforce. A skill that substitutes for enforcement is unsafe, since an agent can ignore any context it is given. One that restates what a deterministic check already verifies is wasteful, spending context on every invocation to duplicate what CI produces for free. One that teaches the upstream pattern so enforcement rarely fires is the right shape, and should name the layer it pairs with. The second is whether it earns its context budget. Frontmatter for every installed skill loads at agent startup, so a skill that is never selected still costs its description on every run. Ships here rather than separately because the two halves are the same decision: the check handles what it can, and the prose says where to spend review attention instead. Adapted from the review of ADR 0057 (MetaMask/decisions#162).
lint-skill-entry structural validator
…verage Three blocking findings from @NicolasMassart. Filenames were interpolated into the shell. They come from PR contents, so a path with spaces split and a path with metacharacters could alter the command. They are now produced by `git diff -z`, passed through the environment, and split on NUL into an array — never placed on a command line. Requires `fetch-depth: 0` for the base commit, and drops the `tj-actions/changed-files` dependency. Malformed directory shapes passed silently. `collectSkills` only returns directories that already match the layout and parse, so a misplaced file, a `SKILL.md` casing error, or a `skill.md` deleted in the same PR were invisible to it — the run printed `0 skill(s) checked, 0 error(s)` and exited 0. Shape is now checked from the path side, before the collect filter, which is where those cases exist. All four examples in the review are covered. The changed-files branch had no tests, which is how the above reached `main`: every case exercised the no-argument full-audit path while CI runs the other one. `lint()` now forwards paths, and the seven cases from the review are covered — including a filename with a space, and a description at exactly `DESCRIPTION_MAX` versus one over. Verified non-vacuous: the pre-fix linter on the missing-`skill.md` fixture prints `0 skill(s) checked, 0 error(s)` and exits 0; the same input now reports the error.
`tools/**` triggered the workflow but never reached the lint step: the file filter listed only `domains/**`, so a schema change spun CI up, found no changed skills, skipped, and reported green — the tightened rule never applied to what already existed. A change under `tools/` or `.github/scripts/` now runs the full audit instead. `scope` and `mandatory` had no enum validation, so `scope: users` or `mandatory: ture` were silent no-ops that installed differently than intended. Both now warn, matching the `maturity` check without widening the blocking surface. The recommended-section regex used a trailing `\b`, which let `## When To Use Cases` satisfy `When To Use`. Anchoring to end-of-line fixes that but rejects `## Workflows` and `## Workflow (interactive)`, both of which are the section and both of which exist here — so the pattern allows an optional plural and an optional parenthetical, nothing else. The reviewer's case is still rejected; the four false positives anchoring introduced are gone. `collectSkills` was called with `repo` undefined and worked only because the linter never reads `repoApplicable`; it now passes an explicit sentinel. README, CONTRIBUTING, and SKILL_TEMPLATE all documented a 1,536-character description while the validator enforces 1,024. All three now say 1,024, with the reason recorded once: it is the lowest ceiling across operators. `lint:skills` is renamed `audit:skills`, since with no arguments it audits the whole catalogue rather than linting a change. The post-merge failure the review anticipated no longer applies — the full audit is clean. The sibling-directory test used `knowledge/`; `workflows/` was the live failure mode but is now legitimately in the bundle list, so the test asserts the general guarantee instead: a sibling the installer does not copy fails, and every directory in `BUNDLE_DIRS` is accepted.
Code scanning raised three findings on this workflow: `actions/checkout` and `actions/setup-node` unpinned against the blanket hash policy, and checkout persisting credentials. Pinning both to SHAs would satisfy the scanner while leaving this the only workflow here reaching for third-party actions. Every other one uses `MetaMask/action-checkout-and-setup@v3`, which does checkout and Node setup together; it takes `fetch-depth`, which is all this needed from raw checkout. Switching removes the dependency, and the findings with it.
Code scanning applies the blanket hash policy to first-party actions too, so `MetaMask/action-checkout-and-setup@v3` still failed after the switch away from third-party ones. Pinned to 0543b5929698c71e3ccc6ed24eac87825669b5de (v3.5.0), with the tag in a trailing comment so the version stays readable. The other workflows here still float on `@v3` — they predate the check and are outside this change.
6 tasks
MajorLift
marked this pull request as ready for review
July 30, 2026 20:34
MajorLift
added a commit
that referenced
this pull request
Jul 30, 2026
`pr-validate` cost ~9,059 tokens the moment an agent selected it — 5.6× the median of the 26 skills in the open PRs — and a quarter of that was the AEP local-run procedure, which most validations never touch. The skill's own Sufficiency section tells you to prefer a lighter lane; the body charged you for the heavy one regardless. Preflight, run mechanics, and teardown move to `references/aep-local-run.md`, which the body already linked twice and which did not exist. The link was dangling — the same defect class the `knowledge/` guard catches, on a path nothing checks. Publishing keeps the decisions (surface by ownership, post complete once, falsifier-forward, scrub) and points at `references/evidence-publishing.md` for the mechanics it already documents in full. Body 34,431 → 25,736 bytes, so a selected skill is ~6,812 tokens installed rather than ~9,059. Nothing is lost: it sits behind the same progressive disclosure boundary as the other seven references, read when an AEP run is actually warranted. Description trimmed 1,147 → 885 characters. It was over the 1,024 ceiling that #47 enforces, so it would have failed that check on merge.
8 tasks
… limit The 1,024 ceiling was justified as the per-operator minimum, citing OpenCode. That claim has no source and does not survive checking: `tools/install` emits the description verbatim with no truncation anywhere, and six skills over 1,024 — up to 1,344 — install and load in Claude Code today. Nothing observed here rejects or truncates a longer one. It also never bound anything. The 46 skills on `main` have a median description of 45 characters and a maximum of 928, so the limit was enforced against a corpus that never approached it, while blocking four skills in open PRs that do. The cost of enforcing it is real. A description is the discovery surface and the only part of a skill carrying its own trigger cues, so trimming one to fit makes the skill less likely to be selected when it is relevant. Cutting content to satisfy an unverified number trades function for compliance. Raised to 1,536, which is what README and SKILL_TEMPLATE said before they were reconciled downward to match the constant — the reconciliation went the wrong way. The comment now states plainly that this is a repo budget bounding always-on context, and asks for an operator and version before anyone claims a figure is externally imposed. The linter says "budget" rather than "operator ceiling" for the same reason. Restores the `performance` description to its full 1,078 characters, cut to 928 only to satisfy the old number. Two tests keep the story straight: the boundary case derives its length from the constant instead of hardcoding one that silently stops testing the boundary when the budget moves, and a new check fails if README, CONTRIBUTING, or SKILL_TEMPLATE state a number the schema does not enforce.
NicolasMassart
approved these changes
Jul 31, 2026
NicolasMassart
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me.
6 tasks
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.
Summary
Acceptance criteria for skill contributions, in both halves — a CI validator for the deterministic properties, and the two questions in
CONTRIBUTING.mdthat no check can answer. From the ADR 0057 review, where acceptance criteria were raised as an open item.The validator
yarn lint:skillschecks directory layout,skill.mdpath and name pattern, frontmatter keys and maturity values,mms-prefix misuse, repo-overlay names, and description length.tools/skill-schema.mjsis the single source of truth, imported by the linter so the documented and enforced schemas cannot drift.Description budget is 1,536 — a repo budget, not an operator limit.
An earlier draft used 1,024, justified as the per-operator minimum citing OpenCode. That does not hold up:
tools/installemits the description verbatim with no truncation anywhere in the tooling, and six skills over 1,024 — up to 1,344 — install and load in Claude Code today. It also never bound anything, since the 46 skills onmainhave a median description of 45 characters and a maximum of 928.Enforcing it had a real cost. A description is the discovery surface and the only part of a skill carrying its own trigger cues, so trimming one to fit makes the skill less likely to be selected when it is relevant. The budget still exists — always-on context should be bounded — but at a number that reflects a deliberate choice rather than an unverified external claim, and the comment now asks for an operator and version before anyone tightens it.
What running it against
mainturned upThree errors, two of which were defects in the repo rather than in the check:
workflows/never shipped. 14 files across twoweb3-toolsskills, referenced 22 times from their bodies, absent from the installer's bundle list — so every installed copy carried 22 dangling links and nothing reported it. Added tocopy_bundle_dirs; all 22 now resolve, 42 files delivered.This is the third instance of one root cause, after
pr-validate'shooks/and domainknowledge/: a directory that exists in source and is not in the copy list.metadatawas rejected as unknown frontmatter, though README documents it as preserved through install. Now in the optional set.performancedescription was 1,078 chars. Under the 1,536 budget, so it stays as authored — an earlier trim to 928 was reverted along with the ceiling that motivated it.Validator now reports 0 errors across 46 skills.
So the list stops being written twice
Two tests read
tools/installdirectly:BUNDLE_DIRSmust match the directories it copies.frontmatter_valuemust be declared in the schema.The comment asking a human to keep Bash and JS in sync is now checked. Knowledge files also get their own declared keys, since they take
domainrather than the installer-behaviour keys skills use.And the half a linter cannot check
CONTRIBUTING.mdgains two questions:Is it a skill, or does it belong in an enforcement layer? Skills shape generation; linters, hooks, and fitness functions enforce. A skill that substitutes for enforcement is unsafe — an agent can ignore any context it is given. One that restates a deterministic check is wasteful. One that teaches the upstream pattern so enforcement rarely fires is the right shape, and should name the layer it pairs with.
Does it earn its context budget? Frontmatter for every installed skill loads at agent startup, so a skill that is never selected still costs its
descriptionon every run.Keeping the number honest
Two tests, because the failure mode here was documentation drifting from enforcement in both directions:
DESCRIPTION_MAXrather than hardcoding one, so it keeps testing the boundary when the budget moves. The previous hardcoded 1,100 silently stopped exercising anything the moment the ceiling rose.README.md,CONTRIBUTING.md, or.github/SKILL_TEMPLATE.mdstate a figure the schema does not enforce. All three had said 1,536 while the schema said 1,024; reconciling them by editing the docs down was the wrong direction, and this makes that disagreement fail rather than get silently resolved.Test plan
node .github/scripts/lint-skill-entry.mjs— 0 errors, 46 skillstest/lint-skill-entry.test.mjs— 10 pass, including both drift checkstest/cli.test.mjs11 pass,test/unit.test.mjs30 passweb3-tools: 22/22workflows/references resolve, 42 files deliveredNotes