* Add TFM-aware buildTransitive->build forwarding guidance to msbuild skills
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>
* Clarify ambiguous props/targets glob in forwarder review check
Address review feedback on dotnet/skills#836: spell out ".props/.targets forwarders" instead of the ambiguous `buildTransitive/*.props|targets`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Tighten forwarding-chain guidance to reduce skill token overhead
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>
* Address review nits: AP range and forward-slash path
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>
* Condense AP-13 forwarding note to a cross-reference to reduce token overhead
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>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Address remaining review nits: targets forwarders, single-source wording, quoted MSBuildThisFileDirectory
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* [dotnet-msbuild] Fix AP-13/AP-14 false positives and add source-vs-packed layout guidance
The msbuild-quality-review run on microsoft/testfx surfaced two confidently-stated
false positives that the maintainer rightly pushed back on:
1. The reviewer claimed unguarded `<Import>` forwarders in MSTest's NuGet
`build/<tfm>/` folders point at non-existent paths under `buildTransitive/<tfm>/`.
In reality those folders are produced at pack time by the `.nuspec` `<file
target="...">` mappings, so the forwarders resolve correctly in every restored
package.
2. The reviewer flagged backslashes in `<Import Project="...\...\...">` as a
cross-platform 🔴 error. MSBuild's evaluator normalizes `\` to `/` on
Unix-like systems via `FileUtilities.MaybeAdjustFilePath` /
`ConvertToUnixSlashes` before resolving the path, so existing backslash-style
imports work everywhere — confirmed by years of shipping MSTest to Linux/macOS
users.
This change updates the rubric so the same false positives don't recur across
every consumer of the dotnet-msbuild plugin:
* `msbuild-antipatterns/SKILL.md` AP-13 — adds an explicit "NuGet package
forwarders" exception and tells reviewers to consult `.nuspec` /
`<PackagePath>` before flagging.
* `msbuild-antipatterns/SKILL.md` AP-14 — rewritten to distinguish where
backslashes are a real bug (raw `<Exec>` shell strings, CDATA, non-MSBuild
consumers — keep 🔴) from where they're only style (`<Import>` and other
evaluator-routed paths — 🔵 with a cite to the MSBuild source).
* `extension-points/SKILL.md` — adds a "Source Tree vs Packed Layout" section
documenting the three packaging mechanisms (.nuspec `<file>` mappings, csproj
`<PackagePath>` metadata, SDK pack conventions) that legitimately reshape the
layout, with the cross-check procedure reviewers must run before flagging
"missing-file" imports.
* `agents/msbuild-code-review.agent.md` — Discovery now records the projected
packed layout; Category 4 references the new AP-13/AP-14 nuances; new
"Veracity gate" step downgrades or drops 🔴 findings that would imply
currently-shipping CI is broken.
No fixtures change: the `tests/dotnet-msbuild/msbuild-antipatterns/eval.yaml`
rubric does not enumerate AP-13/AP-14 by id, and the F#-specific scenarios are
unaffected. Markdownlint passes on all edited files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR review feedback
- extension-points: rewrite .nuspec example so the source file is genuinely shared (single buildTransitive/common/MyAdapter.props fan-out instead of three already-per-TFM sources); clarify <file target=> folder-vs-rename semantics.
- extension-points: replace semicolon-list PackagePath example with the unambiguous multi-<None> form, and note in prose that semicolon syntax is also supported. Avoids reader confusion noted in review.
- msbuild-antipatterns AP-13 cross-check: align search scope with extension-points (project directory AND any parent directory) so reviewers don't miss shared mono-repo nuspecs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* AP-13: drop ../ link to extension-points to satisfy skill-validator
Skill-validator rejects file references that traverse out of the skill directory (parent-directory traversal). Replace the markdown link with a plain-text reference to the dotnet-msbuild/extension-points skill — the prose is still discoverable to humans and to the agent that loads the plugin, but no longer trips the validator.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address second round of PR review feedback
- AP-13: replace <tfm> placeholders inside XML attribute strings with concrete TFM examples (net8.0) so the snippets are valid copy-pasteable XML.
- AP-13 & extension-points: bound the .nuspec search to project directory + IMMEDIATE parent only (no unbounded walking); wording now identical in both docs.
- AP-14: add explicit text severity next to severity emoji (🔴 Error / 🔵 Style) for accessibility — screen readers and plain-text copies now convey severity without relying on color/emoji rendering.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
YuliiaKovalova