Add MSBuild target authoring skills (target-authoring, property-patterns, item-management, extension-points)#669
Conversation
New skills: - target-authoring: three-level target chain, DependsOn extension, naming conventions - property-patterns: conditional defaults, composition, path normalization, TFM helpers - item-management: Include/Remove/Update, batching, transforms, FileWrites registration - extension-points: CustomBefore/After hooks, import gating, NuGet build extensions Each skill includes eval.yaml tests with anti-pattern fixtures. Closes dotnet#668
YuliiaKovalova
requested review from
a team,
JanKrivanek and
ViktorHofer
as code owners
|
/evaluate |
There was a problem hiding this comment.
Pull request overview
Adds four new authoring-focused skills to the dotnet-msbuild plugin (target authoring, property patterns, item management, and extension points) along with evaluation scenarios and anti-pattern fixture files to validate the agent’s review guidance.
Changes:
- Added 4 new skills under
plugins/dotnet-msbuild/skills/*documenting canonical MSBuild authoring patterns. - Added 4 new test suites under
tests/dotnet-msbuild/*witheval.yamlscenarios and fixture projects/files containing intentional anti-patterns. - Added supporting fixture assets (Directory.Build.props/targets, schemas, JSON data, and placeholder source files) used by the eval scenarios.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/dotnet-msbuild/skills/target-authoring/SKILL.md | New guidance for structuring custom targets, chaining, Returns vs Outputs, and templates. |
| plugins/dotnet-msbuild/skills/property-patterns/SKILL.md | New guidance for conditional defaults, composition, path handling, and property functions. |
| plugins/dotnet-msbuild/skills/item-management/SKILL.md | New guidance for Include/Remove/Update, batching, transforms, and common pitfalls. |
| plugins/dotnet-msbuild/skills/extension-points/SKILL.md | New guidance for CustomBefore/After hooks, import guards, Directory.Build discovery, and NuGet build extensions. |
| tests/dotnet-msbuild/target-authoring/TargetAuthoring.csproj | Fixture project containing target-authoring anti-patterns for review. |
| tests/dotnet-msbuild/target-authoring/Placeholder.cs | Minimal source file for the target-authoring fixture. |
| tests/dotnet-msbuild/target-authoring/eval.yaml | Eval scenario asserting correct feedback on target-authoring anti-patterns. |
| tests/dotnet-msbuild/property-patterns/PropertyPatterns.csproj | Fixture project for property-patterns eval context. |
| tests/dotnet-msbuild/property-patterns/Placeholder.cs | Minimal source file for the property-patterns fixture. |
| tests/dotnet-msbuild/property-patterns/eval.yaml | Eval scenario asserting correct feedback on property definition anti-patterns. |
| tests/dotnet-msbuild/property-patterns/Directory.Build.props | Fixture props file containing property anti-patterns. |
| tests/dotnet-msbuild/item-management/ItemManagement.csproj | Fixture project containing item-management anti-patterns for review. |
| tests/dotnet-msbuild/item-management/Placeholder.cs | Minimal source file for the item-management fixture. |
| tests/dotnet-msbuild/item-management/eval.yaml | Eval scenario asserting correct feedback on item/batching/generation issues. |
| tests/dotnet-msbuild/item-management/schemas/users.xsd | Fixture schema input for item-management scenarios. |
| tests/dotnet-msbuild/item-management/schemas/orders.xsd | Fixture schema input for item-management scenarios. |
| tests/dotnet-msbuild/item-management/data/users.json | Fixture data input for item-management scenarios. |
| tests/dotnet-msbuild/item-management/data/orders.json | Fixture data input for item-management scenarios. |
| tests/dotnet-msbuild/item-management/Constants.g.cs | Fixture generated-file example used by the item-management scenario. |
| tests/dotnet-msbuild/extension-points/ExtensionPoints.csproj | Fixture project for extension-points eval context. |
| tests/dotnet-msbuild/extension-points/Placeholder.cs | Minimal source file for the extension-points fixture. |
| tests/dotnet-msbuild/extension-points/eval.yaml | Eval scenario asserting correct feedback on import guards and extensibility patterns. |
| tests/dotnet-msbuild/extension-points/Directory.Build.targets | Fixture targets file containing extension-point anti-patterns. |
| tests/dotnet-msbuild/extension-points/Directory.Build.props | Fixture props file defining RepoRoot for the extension-points scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Skill Validation Results
[1] Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps ▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
- target-authoring: Reword DO NOT USE clause to exclude only deep incremental-build diagnostics, not basic Inputs/Outputs usage - property-patterns: Close unclosed XML elements in String Functions snippet (PropertyGroup, TargetFrameworkMoniker) - extension-points: Add missing CustomAfterMySDK property definition to match the import at bottom of example
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
plugins/dotnet-msbuild/skills/extension-points/SKILL.md:150
- The parent Directory.Build.props import example is missing an Exists(...) guard. Without it, the Import can fail when the file isn’t found (and the repo’s own directory-build-organization skill shows the guarded pattern). Consider adding an Exists condition to make the snippet robust and consistent.
```xml
<!-- src/Directory.Build.props -->
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)..\'))" />
**plugins/dotnet-msbuild/skills/property-patterns/SKILL.md:78**
* The IsPathRooted example passes $(MSBuildProjectExtensionsPath) unquoted into the property function. Quote the argument so paths containing spaces/special chars don’t break the function call, and keep function-call quoting consistent within the doc.
- extension-points: Use full MSBuildExtensionsPath expression and consistent Exists() casing in wildcard import example - property-patterns: Quote property arguments in NormalizePath call
|
/evaluate |
|
This looks good but before approving I would like to see an improvement in an evaluation run. My understanding was that the LLM with the public available documentation was good enough. |
|
/evaluate |
Skill Validation Results
[1] Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps ▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Rewrite all 4 eval prompts as natural developer problem descriptions instead of skill-aligned checklists. Softens technique-prescriptive rubric items. Result: overfitting scores drop from 0.15-0.31 to 0.06-0.08 (all green). property-patterns now passes eval.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
plugins/dotnet-msbuild/skills/property-patterns/SKILL.md:110
- The
IsNullOrWhitespaceexample passes$(TargetFrameworkProfile)without quoting it inside the property function call. This can break if the property is empty or contains spaces/special characters; quote it as a string argument so the sample is copy/paste safe.
<!-- IsNullOrWhitespace -->
<TargetFrameworkMoniker
Condition="'$([System.String]::IsNullOrWhitespace($(TargetFrameworkProfile)))' != 'true'">
$(TargetFrameworkIdentifier),Version=$(TargetFrameworkVersion),Profile=$(TargetFrameworkProfile)
</TargetFrameworkMoniker>
- property-patterns: Quote in IsPathRooted call - extension-points: Quote in GetDirectoryNameOfFileAbove - item-management fixture: Use literal semicolon instead of %3B in WriteLinesToFile
Local Eval Results (5 runs, claude-opus-4.6)After iterating on eval prompts, skill descriptions, and content trimming:
Changes made since initial submission:
Remaining ❌ verdicts are purely token-cost penalties (-7% to -9%). The skills add reference material the agent reads, which increases tokens even when quality is unchanged. No quality regressions — all skills score 5.0/5 with and without the skill. |
This implies that the skills don't add any value based on the current set of evaluation tests. I don't know if that's accurate or if the tests need to be better / more complex. |
Each skill now has a second scenario with multi-file setups containing interacting bugs that require cross-file analysis: - target-authoring: SDK .targets + Directory.Build.targets with 3 bugs (target redefinition, fragile BeforeTargets, duplicate DependsOn chain) - property-patterns: nested Directory.Build.props with 5 bugs (import order, unconditional overwrite, unquoted condition, missing trailing slash, NoWarn overwrite) - item-management: csproj with 5 interacting bugs (Include vs Update, cross-product batching, eval-time FileWrites, too-broad Remove glob) - extension-points: Directory.Build.props/targets + NuGet package with 6 bugs (inverted guard, late ImportByWildcard, CustomBefore overwrite, missing Exists guard, target name collision, internal target hook) Timeouts increased to 180s for all scenarios.
Harder multi-file eval scenarios addedAdded a second scenario to each skill's eval.yaml with multi-file setups containing interacting bugs that require cross-file analysis. Each \hard/\ subfolder has 3-6 intentional anti-patterns spread across multiple MSBuild files. Eval results (v4 and v5, 5 runs each, claude-opus-4.6)Best results (v4):
Consistent result (passed in both v4 and v5):
Key observations
Hard scenario designEach scenario has multiple interacting bugs spread across 2-3 files:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
plugins/dotnet-msbuild/skills/extension-points/SKILL.md:139
- The parent-import example uses a Windows-only
..\path insideGetPathOfFileAbove. Since these skills aim to be cross-platform, prefer a normalized/cross-platform form (e.g.../or$([MSBuild]::NormalizePath(...))) and/or include anExists(...)guard like other skills in this repo to avoid import errors when the parent file isn’t present.
```xml
<!-- src/Directory.Build.props -->
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)..\'))" />
</details>
…call in condition
|
/evaluate |
Skill Validation Results
[1] (Isolated) Quality unchanged but weighted score is -11.8% due to: judgment, quality Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps ▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Summary
Adds four new skills to the
dotnet-msbuildplugin covering MSBuild target/props authoring best practices derived from analysis of the MSBuild repo's own.targetsand.propsfiles.Closes #668
New Skills
1.
target-authoringThree-level target chain pattern (Before/Core/After), DependsOn chain extension, DependsOnTargets vs BeforeTargets/AfterTargets guidance, Returns vs Outputs, incremental build with Inputs/Outputs, naming conventions, and a complete target template.
2.
property-patternsConditional defaults, semicolon-delimited composition, path normalization, MSBuild string functions, TFM condition helpers, guard properties, feature gating, and fallback chains.
3.
item-managementInclude/Remove/Update semantics, batching (single-axis vs cross-product pitfall), item transforms, Exclude patterns, conditional item inclusion, PrivateAssets/ExcludeAssets metadata, and FileWrites registration for generated files.
4.
extension-pointsCustomBefore/CustomAfter hooks, wildcard import directories with alphabetic ordering, import gating with control properties, NuGet package build extension layout (build/buildTransitive), Directory.Build discovery and multi-level hierarchy, and the import guard pattern.
Tests
Each skill includes:
Checklist