Add structural validation checks ported from dotnet/skills#19
Add structural validation checks ported from dotnet/skills#19danmoseley wants to merge 3 commits intodotnet:mainfrom
Conversation
Port 5 static validation checks to pr-validation.yml: - Reference scanning: unknown domains, http-not-https, pipe-to-shell, script-no-SRI - External dependency checker: scripts, INVOKES, #tool: refs, MCP servers - Duplicate skill name detection across plugins - Aggregate description length warning per plugin - Bundled asset size check (>5MB = error) Add config files for self-serve allowlisting: - eng/known-domains.txt: URL domain allowlist for reference scanning - eng/allowed-external-deps.txt: external dependency allowlist All checks are fork-safe (no tokens/secrets needed). New checks produce warnings (non-blocking) except duplicate names and oversized assets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Ports additional static structural validation checks (from dotnet/skills) into this repo’s PR validation workflow, and introduces allowlist config files under eng/ so contributors can manage warnings without requiring secrets (fork-safe).
Changes:
- Add
eng/known-domains.txtandeng/allowed-external-deps.txtas line-based allowlists for URL/reference scanning and external dependency warnings. - Extend
.github/workflows/pr-validation.ymlwith new validations: reference scanning, external dependency checks, duplicate skill name detection, aggregate description-length warning, and bundled asset size enforcement. - Update workflow path filters to run validation when
eng/**changes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
eng/known-domains.txt |
Adds a domain/path allowlist used by PR validation reference scanning. |
eng/allowed-external-deps.txt |
Adds an allowlist for external-dependency-related warnings (scripts, INVOKES, MCP servers, etc.). |
.github/workflows/pr-validation.yml |
Implements the new validation checks and loads the new eng/ allowlists; updates triggers to include eng/**. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- MCP server check now uses already-parsed $pluginJson instead of re-reading and swallowing errors in a try/catch - Fix tool-ref format in allowed-external-deps.txt (NAME -> SKILL-NAME) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Non-built-in #tool: references in SKILL.md content | ||
| foreach ($toolMatch in [regex]::Matches($skillContent, '#tool:[\w/]+')) { | ||
| $toolName = $toolMatch.Value.Substring(6) | ||
| if (-not $BuiltInTools.Contains($toolName)) { | ||
| $depKey = "tool-ref:$($skillName):$($toolMatch.Value)" | ||
| if (-not $allowedDeps.Contains($depKey)) { | ||
| $warnings.Add("$sCtx — non-built-in tool reference '$($toolMatch.Value)' — verify this is intentional. (allow: $depKey)") |
There was a problem hiding this comment.
The #tool: matcher regex (#tool:[\w/]+) will stop at - or . characters, so references like #tool:mcp-binlog-tool would be detected as #tool:mcp and produce an incorrect allowlist key/warning. Expand the character class to include hyphens (and any other valid tool-name chars used in this repo) so tool refs are parsed reliably.
| # ── Reference scanning (per skill) ────────────────── | ||
| # Scan SKILL.md and all reference/*.md files for URL issues | ||
| if ($knownDomains.Count -gt 0) { | ||
| $scanFiles = @($skillMdPath) | ||
| $refsDir = Join-Path $skillDir.FullName 'references' | ||
| if (Test-Path $refsDir) { | ||
| $scanFiles += Get-ChildItem -Path $refsDir -Filter '*.md' | ForEach-Object { $_.FullName } | ||
| } |
There was a problem hiding this comment.
This reference scanning block only scans SKILL.md + skills/<skill>/references/*.md, but the PR description/config comments mention scanning “skill/agent content”. If agents (*.agent.md) are intended to be covered, add scanning for files under the plugin’s configured agents directory as well (and keep the behavior consistent with the allowlist in eng/known-domains.txt).
|
@copilot address feedback push to this PR |
|
Why not just invoke skill-validator? |
|
I didn't want any of the AI based validation -- setting up the PATs and refreshing regularly, it's slow, etc. Is there a way to get the other parts without copying the code? |
|
Yes. Just invoke "skill-validator check" instead of "skill-validator eval". The binary is available directly from the "SkillValidator Nightly" release tag. I.e. https://github.com/dotnet/skills/releases/download/skill-validator-nightly/skill-validator-linux-x64.tar.gz |
Port 5 static (non-AI) validation checks from dotnet/skills to pr-validation.yml. All checks run as inline PowerShell with no tokens or secrets, so they work on fork PRs.
New checks
http://instead ofhttps://, pipe-to-shell patterns (curl | bash)#tool:refs, MCP servers not on the allowlistConfig files
Two new files under
eng/let PR authors self-serve additions:eng/known-domains.txt— URL domain allowlist for reference scanning. Entries with/match path prefixes; bare domains match the domain and subdomains.eng/allowed-external-deps.txt— Allowlist for external dependencies (scripts, MCP servers, INVOKES, etc). Format:type:skill-name:detail.Validation results
Note
This PR description was generated with the help of Copilot.