Add TFM-aware buildTransitive->build forwarding guidance to msbuild skills#836
Add TFM-aware buildTransitive->build forwarding guidance to msbuild skills#836YuliiaKovalova wants to merge 6 commits into
Conversation
…kills buildTransitive/*.props should forward through the corresponding build/*.props (ownership chain buildTransitive -> build -> shared) rather than importing buildMultiTargeting/ directly. When build/ is packed per-TFM (build/<tfm>/), the forwarder must include the TFM segment and derive it from the file own folder, not $(TargetFramework) (NuGet nearest-match can serve a different asset folder), otherwise transitive consumers hit MSB4019. Updates extension-points (new Forwarding chain section), msbuild-antipatterns AP-13, and the msbuild-code-review agent. Lesson learned from microsoft/testfx#9431. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
YuliiaKovalova
requested review from
a team,
JanKrivanek and
ViktorHofer
as code owners
There was a problem hiding this comment.
Pull request overview
Adds guidance to the dotnet-msbuild skill set to prevent a real-world packaging/build break: when build/ assets are packed per-TFM, buildTransitive/<tfm>/ forwarders must include the <tfm> segment and derive it from the forwarder’s own folder (not $(TargetFramework)), preserving the intended ownership chain buildTransitive → build → shared.
Changes:
- Documented the recommended forwarding chain (
buildTransitive/→build/→ shared) and the TFM-aware forwarding pattern, including a concrete MSBuild derivation expression. - Extended AP-13 (“Import Without Exists() Guard”) with a TFM-aware forwarding note to prevent false positives and real restore-time asset mismatches.
- Added a Category-4 code review checklist item to ensure reviewers check for correct transitive forwarding behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plugins/dotnet-msbuild/skills/msbuild-antipatterns/SKILL.md | Adds AP-13 guidance on TFM-aware buildTransitive/<tfm>/ → build/<tfm>/ forwarding to avoid MSB4019. |
| plugins/dotnet-msbuild/skills/extension-points/SKILL.md | Introduces a “Forwarding chain” section with rationale, examples, and a safe <tfm> derivation expression. |
| plugins/dotnet-msbuild/agents/msbuild-code-review.agent.md | Adds a Category-4 review check for correct forwarding chain and TFM derivation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback on dotnet#836: spell out ".props/.targets forwarders" instead of the ambiguous `buildTransitive/*.props|targets`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the review feedback in d38fb2e: replaced the ambiguous |
|
👋 @YuliiaKovalova — this PR has 1 unresolved review thread(s). When you're ready, please address the feedback and push an update; the triage bot will pick up the next state automatically. (Add the |
github-actions
Bot
added
pr-state/ready-for-eval
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 |
Condense the new extension-points Forwarding chain section (+26 -> +11 lines) and the AP-13 note by dropping the redundant non-TFM example and self-evident derivation explanation, keeping the chain rule, MSB4019 cause, and the TFM derivation expression. Lower token footprint addresses the skill-validator weighted-score token penalty without losing substance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Eval analysis + quality iterationI downloaded the eval artifacts (run 28239731107) and analyzed The failing verdicts are noise/overhead-driven, not content regressionsThe pairwise A/B judgments (the more reliable signal — direct comparison of baseline vs skilled output) favor the skill on both "failing" scenarios:
What I changedEven though the dips are noise, the RecommendationGiven CV of 81–102%, a single run is dominated by LLM non-determinism. Re-running with more runs should stabilize the signal (the guide suggests 7–10 for noisy scenarios). /evaluate --runs 7 |
Update the additional anti-patterns range to AP-16 through AP-22 (the reference doc now includes AP-22), and use a forward-slash build/MyPackage.props in the forwarding-chain prose to match the build/<tfm>/ convention used in the section. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed both review nits in 0b7f356: updated the range to "AP-16 through AP-22" (the reference doc now includes AP-22), and switched the prose to a forward-slash |
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 |
…verhead The full TFM-forwarding guidance lives in extension-points; AP-13 only needs a concise pointer. Removes duplication and trims the msbuild-antipatterns skill footprint flagged by the skill-validator token penalty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Re: the three F#
|
|
|
||
| ### Forwarding chain: `buildTransitive/` → `build/` → shared | ||
|
|
||
| Forward `buildTransitive/*.props` through the sibling `build/*.props` (chain `buildTransitive → build → shared`) instead of importing `buildMultiTargeting/` directly — this keeps a clear ownership chain and makes explicit that transitive consumers get a subset of what direct consumers see. |
|
|
||
| ```xml | ||
| <!-- buildTransitive/<tfm>/MyPackage.props --> | ||
| <Import Project="$(MSBuildThisFileDirectory)..\..\build\$([System.IO.Path]::GetFileName($([System.IO.Path]::GetDirectoryName($(MSBuildThisFileDirectory)))))\MyPackage.props" /> |
|
|
||
| Before flagging an unguarded `<Import>` inside a `build/` or `buildTransitive/` folder, **resolve it against the packed layout** — read every `*.nuspec` in the project directory **and its immediate parent directory** (shared nuspecs are common in mono-repos; do not walk further up), and any `<PackagePath>` metadata on `<None>`/`<Content>` items in the `.csproj`. Only flag if the target path is missing from **both** the source tree *and* the projected package layout. The `dotnet-msbuild/extension-points` skill — *Source tree vs packed layout* — documents the full cross-check procedure. | ||
|
|
||
| **Forwarding `buildTransitive/` → `build/`:** forward through the sibling `build/*` file (not directly to `buildMultiTargeting/`); when `build/` is per-TFM (`build/<tfm>/`), include the TFM segment derived from the file's own folder (not `$(TargetFramework)`), or transitive consumers hit `MSB4019`. See the `extension-points` skill — *Forwarding chain* — for the rule and derivation expression. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
|
||
| ### Forwarding chain: `buildTransitive/` → `build/` → shared | ||
|
|
||
| Forward `buildTransitive/*.props` through the sibling `build/*.props` (chain `buildTransitive → build → shared`) instead of importing `buildMultiTargeting/` directly — this keeps a clear ownership chain and makes explicit that transitive consumers get a subset of what direct consumers see. |
|
|
||
| ```xml | ||
| <!-- buildTransitive/<tfm>/MyPackage.props --> | ||
| <Import Project="$(MSBuildThisFileDirectory)..\..\build\$([System.IO.Path]::GetFileName($([System.IO.Path]::GetDirectoryName($(MSBuildThisFileDirectory)))))\MyPackage.props" /> |
github-actions
Bot
added
waiting-on-author
Summary
Captures the lesson learned in microsoft/testfx#9431 about TFM-aware
buildTransitive/→build/forwarding.A NuGet build-extension
buildTransitive/*.propsshould forward through the correspondingbuild/*.props(ownership chainbuildTransitive → build → shared) rather than importingbuildMultiTargeting/directly. The subtlety:build/*.propsare frequently packed per-TFM underbuild/<tfm>/, whilebuildMultiTargeting/is not. So a forwarder living inbuildTransitive/<tfm>/must include the TFM segment — and must derive<tfm>from the file's own folder, not$(TargetFramework), because NuGet nearest-match can serve a consumer a different asset folder. Dropping the segment resolves to a non-existent package-rootbuild/Pkg.propsand fails withMSB4019for transitive consumers.Changes
buildTransitive/→build/→ shared" section, including the TFM-segment requirement and the derive-from-own-folder expression.Reference: microsoft/testfx#9431 (Rule E-1).