ci: validate cross-skill references in lint-skill-entry - #103
Open
MajorLift wants to merge 4 commits into
Open
Conversation
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.
There was a problem hiding this comment.
Pull request overview
This PR extends the lint-skill-entry CI gate to validate cross-skill references inside skill.md content so that references shown to readers are mechanically resolvable (or explicitly flagged).
Changes:
- Read the full
skill.mdsource alongside frontmatter to support accurate line-number reporting. - Add cross-reference checks for: lane-id usage (error in frontmatter
description, warning in body without a catalog link), private-vault[[snake_case]]wiki links (error), and unresolvable## Relatedskill names (warning). - Cache known skill names for
## Relatedresolution.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| crossReferenceChecks(skill, raw, source, errors, warnings); |
…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.
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.
Problem
Three kinds of cross-skill reference could name something no reader can reach, and nothing checked any of them. All three were live in open PRs when this was written:
B7,C4race-condition-repro,react-render-delta,falsifying-testevidence-catalog.md, not a name. It carries no meaning to a reader who has not opened that file — and a frontmatterdescriptionis plain text, so it cannot link out to one.[[snake_case]]evidence## Relatedreact-render-delta,agent-run-costpr-validateandmemory-leak-hunthad both been renamed;sentry-quotaexisted on no branch at all.The rename cases share a mechanism worth calling out: a rename sweeps the branch that performs it and leaves every branch that referenced the old name pointing at nothing. In one case the stale name survived inside the very PR doing the renaming.
What this adds
lintSkillnow resolves these mechanically.descriptionis an error. It is unresolvable by construction, so there is no valid instance to allow. In the body it is a warning, suppressed when the line links the catalog. The skill that owns the catalog is exempt — it defines them.[[snake_case]]is an error. Matching on the underscore is what keeps JS array literals ([[signer1.address, …]], common inworkflows/snippets) from tripping it.## Relatedname is a warning, not an error. The gate runs on the PR's own branch, where a sibling skill shipping in a concurrent PR does not exist yet. Blocking would fail a PR for a forward reference that resolves on merge.Notes for review
Two things I got wrong first, both preserved as comments so the next person does not repeat them:
collectSkillstakes an array of roots. Passing a bare string iterates its characters, yields zero skills, and makes every check pass vacuously — the run looked clean because it checked nothing.skill.bodyis frontmatter-stripped, so its indices are not the line numbers a reader sees. The offset is derived by locating the body in the source rather than subtracting line counts, which differ by trailing-newline handling.Verified against
main(47 skills, no findings) and against each branch carrying a known defect, where it reports the exact file lines.