Enforce description size limits and add spec conformance checks#238
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds agentskills.io specification conformance checks to the SkillProfiler in the skill validator, building on the description size limit enforcement from PR #224. It introduces name validation rules, compatibility field length checks, body line count warnings, and file reference depth warnings. A new Compatibility field is threaded through the model layer and discovery pipeline, and a CI workflow plus PowerShell validation script enforce description size limits centrally.
Changes:
- Adds name, compatibility, body line count, and file reference depth validation to
SkillProfiler.AnalyzeSkill(), withdescription_too_longas a blocking failure in the validate command (even in--verdict-warn-onlymode). - Introduces
eng/validate-descriptions.ps1with CI workflow and PowerShell integration tests for per-skill (1024 chars) and per-plugin aggregate (15K chars) description size limits. - Adds
Compatibilityfield toSkillInfo,RawFrontmatter, andSkillDiscovery, plus updates CODEOWNERS to removenuget-trusted-publishingand@dotnet/appmodelfromdotnet-aot-compat.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
eng/skill-validator/src/Services/SkillProfiler.cs |
Adds name validation, description length check, compatibility length check, body line count check, file reference depth check, and two new generated regexes |
eng/skill-validator/src/Models/Models.cs |
Adds optional Compatibility field to SkillInfo record |
eng/skill-validator/src/Services/EvalSchema.cs |
Adds Compatibility property to RawFrontmatter for YAML deserialization |
eng/skill-validator/src/Services/SkillDiscovery.cs |
Threads Compatibility from parsed frontmatter into SkillInfo construction |
eng/skill-validator/src/Commands/ValidateCommand.cs |
Adds description_too_long as a non-warnable blocking failure and early-return verdict |
eng/skill-validator/tests/SkillProfileTests.cs |
17 new unit tests for name, description, compatibility, body lines, and file reference depth validation |
eng/validate-descriptions.ps1 |
New script for validating per-skill and per-plugin aggregate description sizes |
eng/tests/validate-descriptions.tests.ps1 |
6 PowerShell integration tests for the description validation script |
.github/workflows/validate-descriptions.yml |
New CI workflow to run description validation on PRs/pushes affecting plugins |
agentic-workflows/dotnet-msbuild/build.ps1 |
Delegates description size checks to the central validation script |
.github/CODEOWNERS |
Removes nuget-trusted-publishing entries, removes @dotnet/appmodel from dotnet-aot-compat |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d959b32 to
a4d6011
Compare
Add infrastructure to reject skills with oversized descriptions, per issue dotnet#222: - SkillProfiler: check description length, flag DescriptionTooLong - ValidateCommand: block oversized descriptions (survives --verdict-warn-only) - New eng/validate-descriptions.ps1: validates per-skill and per-plugin aggregate limits - New CI workflow: runs validation on PRs touching plugins/ - build.ps1: adds size checks to local build validation - Unit tests for all new SkillProfiler behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review feedback: the option description now lists description_too_long alongside execution errors and --require-evals as conditions that remain fatal even when --verdict-warn-only is enabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Validate name rules (max 64 chars, lowercase+hyphens, no edge/consecutive hyphens, must match directory), compatibility field max 500 chars, body max 500 lines, and file reference depth. Scan of existing skills found 2 violations: - dotnet-msbuild/msbuild-antipatterns: 582 lines (max 500) - dotnet-msbuild/directory-build-organization: cross-skill ref 3 levels deep Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix MakeSkill SkillMdPath to derive from resolved path (SkillProfileTests.cs) - Fix RepoRoot in build.ps1 to be actual repo root (was agentic-workflows/) - Fix body line count off-by-one: trim trailing newlines before splitting - Fix file ref depth: normalize paths, handle .. traversals, strip #anchor, compute depth from directory segments only - Add tests for parent traversal, anchor fragments, ./ normalization, and trailing newline body count Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a4d6011 to
a072630
Compare
JanKrivanek
left a comment
There was a problem hiding this comment.
LGTM.
I'd prefer this to be part of regular validation run
ViktorHofer
left a comment
There was a problem hiding this comment.
Love it. As Jan already commented on, I would also remove implement this purely in the skill-validator and avoid the powershell scripts and the additional workflow.
Move the per-plugin aggregate description limit (15K chars) from the standalone PowerShell script into ValidateCommand, where it runs after skill discovery. This eliminates the separate validate-descriptions.ps1, its CI workflow, its PowerShell tests, and the delegation from build.ps1. Also rename FailureKind from "description_too_long" to the generic "spec_conformance_failure" so future spec checks reuse the same kind. Deleted: - eng/validate-descriptions.ps1 - eng/tests/validate-descriptions.tests.ps1 - .github/workflows/validate-descriptions.yml Added: - ValidateCommand.CheckAggregateDescriptionLimits() - ValidateCommand.DerivePluginName() - 6 C# tests for aggregate limit + DerivePluginName Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Now that dotnet#247 has merged and fixed the two existing violations (msbuild-antipatterns 582 lines, directory-build-organization cross-ref), these spec conformance checks can be blocking. Also replaces the DescriptionTooLong bool with a general Errors list on SkillProfile, so all spec conformance violations (description length, body lines, file ref depth) flow through the same mechanism. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The lock file diffs were side effects of running build.ps1 to verify the validation changes — they picked up upstream content changes unrelated to this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
OK @ViktorHofer should be ready for re-review. |
|
When this is merged, I will open PR for a followup for agent/plugin spec validation holes. |
Adds description size limit enforcement and agentskills.io spec conformance checks to SkillProfiler. (Supersedes #224, which has been closed.)
Description size limits (from #224, closes #222)
Per-skill limit (1024 chars):
ErrorslistFailureKind = "spec_conformance_failure"(survives--verdict-warn-only)Per-plugin aggregate limit (15,000 chars):
ValidateCommand.CheckAggregateDescriptionLimits()groups discovered skills by plugin and checks aggregate sizeevaluation.ymlpipeline — no separate workflow neededCurrent state: all plugins pass (max skill: 769 chars, max plugin: 5,765 chars).
Spec conformance checks (new)
Blocking errors:
Warnings:
Other fixes
$RepoRootinbuild.ps1(was resolving toagentic-workflows/instead of the repo root after the dotnet-msbuild subfolder reorg)DescriptionTooLongbool with a generalErrorslist onSkillProfile, so all spec conformance violations flow through the same mechanismTests: 9 C# unit tests for description limits (per-skill + aggregate); 21 C# unit tests for spec conformance checks.