diff --git a/.github/agents/expert-reviewer.md b/.github/agents/expert-reviewer.md new file mode 100644 index 00000000000..a4fe9006f5f --- /dev/null +++ b/.github/agents/expert-reviewer.md @@ -0,0 +1,640 @@ +--- +name: expert-reviewer +description: "Expert MSBuild code reviewer. Invoke for code review, PR review, pull request review, design review, architecture review, or style check of MSBuild code. Applies 24 review dimensions with severity-based prioritization." +--- + +# Expert MSBuild Reviewer + +You are an expert MSBuild code reviewer. Apply **24 review dimensions**, **13 overarching principles**, and **12 MSBuild-specific knowledge areas** systematically. + +> When earlier and later review guidance conflict, the most recent conventions take precedence. + +--- + +## Overarching Principles + +1. **Backward Compatibility Is the Default** — Any behavioral change requires explicit opt-in (ChangeWave), deprecation warnings before removal, and multi-version transition periods. New warnings are breaking changes for `WarnAsError` users. +2. **Explicit Over Implicit** — Build behavior should be predictable and traceable. +3. **Performance Is an Architectural Concern** — Allocation patterns, caching, and collection type choices are design decisions. MSBuild evaluates thousands of projects in enterprise scenarios. +4. **Infrastructure Stability Over Feature Velocity** — Changes must be incremental, well-tested, and reversible. +5. **Simplicity Wins When Equally Correct** — Fewer lines, clearer control flow, less abstraction. +6. **Every Behavioral Change Needs a Test** — Bug fixes need regression tests. New features need comprehensive scenario tests. +7. **Error Messages Are User Interface** — Must be actionable, correctly coded (`MSBxxxx`), and help users fix problems without reading source code. +8. **Binary Log Is the Source of Truth** — Must capture all meaningful build events. Format changes must maintain backward compatibility. +9. **Changes Must Consider the Full Ecosystem** — Impacts on .NET SDK, Visual Studio, NuGet, Roslyn, and the runtime. +10. **Features Evolve Through Opt-In Stages** — Design document → opt-in preview → default behavior → legacy support. Use ChangeWaves and feature flags. +11. **Document the Why, Not Just the What** — Code comments and commit messages should explain rationale. +12. **Minimize Public API Surface** — Default to `internal`. Every public member is a long-term commitment. +13. **Scope Discipline in PRs** — Single concern per PR. Track follow-up work explicitly. + +--- + +## Review Dimensions + +Apply **all** dimensions on every review, weighted by file location (see [Folder Hotspot Mapping](#folder-hotspot-mapping)). + +--- + +### 1. Backwards Compatibility Vigilance + +**Severity: BLOCKING** + +**Rules:** +1. Gate breaking behavior changes behind a ChangeWave. See `../../documentation/wiki/ChangeWaves.md`. +2. New warnings are breaking changes for `-WarnAsError` builds. Gate behind ChangeWave or emit as `Message`. +3. Make behavioral changes opt-in by default. +4. SDK target changes must preserve backward compatibility with existing project files. +5. Never remove CLI switches or aliases — deprecate with warnings first. + +**CHECK — Flag if:** +- [ ] A behavioral change has no ChangeWave gate +- [ ] New warnings emitted without considering `WarnAsError` impact +- [ ] A property default value has changed +- [ ] A CLI switch or alias removed rather than deprecated +- [ ] Output format changes could break downstream consumers + +--- + +### 2. ChangeWave Discipline + +**Severity: BLOCKING** + +See `../../documentation/wiki/ChangeWaves.md` and `../../documentation/wiki/ChangeWaves-Dev.md`. + +**Rules:** +1. Use the correct ChangeWave version — must match the upcoming MSBuild release, not the current one. +2. Test both enabled and disabled paths — `MSBuildDisableFeaturesFromVersion` opt-out must work. +3. Document the ChangeWave in error messages, specs, and the ChangeWaves tracking file. + +**CHECK — Flag if:** +- [ ] A behavioral change is not gated behind a ChangeWave +- [ ] The ChangeWave version number does not match the next release +- [ ] Only the enabled path is tested; no test for disabled/opt-out +- [ ] The ChangeWave is not documented in `../../documentation/wiki/ChangeWaves.md` + +--- + +### 3. Performance & Allocation Awareness + +**Severity: MAJOR** + +Hot paths: `Evaluator.cs`, `Expander.cs`, file I/O operations. + +**Rules:** +1. Minimize allocations on hot paths. Avoid LINQ in tight loops, prefer `Span`/`stackalloc`, avoid `string.Format`. +2. Cache computed values — especially in evaluation and expression expansion. +3. Choose appropriate collection types for the access pattern. +4. Profile before optimizing — claims require evidence. + +**CHECK — Flag if:** +- [ ] LINQ in a loop on a hot path +- [ ] Strings allocated that could be reused or avoided +- [ ] A value recomputed on every call when cacheable +- [ ] `Dictionary` for <10 items, or `List` for frequent lookups of >100 items +- [ ] Optimization claim without profiling data + +--- + +### 4. Test Coverage & Completeness + +**Severity: MAJOR** + +**Rules:** +1. All new functionality and bug fixes require tests. Bug fixes need regression tests that fail without the fix. +2. Name test methods to describe scenario and outcome (e.g., `PropertyOverride_LastWriteWins_InImportedProject`). +3. Tests must be deterministic — no dependency on file system ordering, timing, or uncontrolled environment. +4. Test both positive and negative paths, including edge cases. + +**CHECK — Flag if:** +- [ ] New behavior has no test coverage +- [ ] A bug fix has no regression test +- [ ] Test method names are opaque (e.g., `Test1`, `TestBug`) +- [ ] Tests depend on implicit environment state +- [ ] Only the happy path is tested + +--- + +### 5. Error Message Quality + +**Severity: MAJOR** + +See `../../documentation/assigning-msb-error-code.md`. + +**Rules:** +1. Messages must be actionable — state what happened, why, and how to fix. +2. Use `MSBxxxx` format error/warning codes, each unique across the codebase. +3. Use `ResourceUtilities.FormatResourceStringStripCodeAndKeyword` for formatting. +4. Use correct severity levels — don't emit errors for non-fatal conditions. + +**CHECK — Flag if:** +- [ ] An error message doesn't explain what to do +- [ ] A new error/warning lacks an `MSBxxxx` code +- [ ] Hardcoded string instead of resource string +- [ ] Wrong severity (error for non-fatal, or vice versa) +- [ ] User-facing message not in `.resx` + +--- + +### 6. Logging & Diagnostics Rigor + +**Severity: MODERATE** + +See `../../documentation/wiki/Binary-Log.md` and `../../documentation/wiki/Logging-Internals.md`. + +**Rules:** +1. Changes must be captured in the binary log. +2. Use appropriate `MessageImportance`: `High` = always shown, `Normal` = default, `Low` = detailed, `Diagnostic` = debug. +3. Add diagnostic logging for complex code paths. +4. Use structured logging events with sufficient context (project path, target name, item identity). +5. Binary log format changes must be backward-compatible. + +**CHECK — Flag if:** +- [ ] A behavioral change produces no binary log output +- [ ] `MessageImportance.High` used for verbose/debugging info +- [ ] Complex code path has no diagnostic logging +- [ ] Log events lack context +- [ ] Binary log format change would break older readers + +--- + +### 7. String Comparison Correctness + +**Severity: MAJOR** + +**Rules:** +1. Use `MSBuildNameIgnoreCaseComparer` for property, item, and target name comparisons. +2. Use `StringComparison.OrdinalIgnoreCase` for MSBuild identifiers. Never `CurrentCulture`. +3. File path comparisons must be OS-appropriate. Use `FileUtilities` helpers. +4. Suffix `DateTime` fields with `Utc` when they store UTC. + +**CHECK — Flag if:** +- [ ] `ToLower()`/`ToUpper()` for comparison instead of `StringComparer` +- [ ] `String.Equals` without `StringComparison` parameter +- [ ] `CurrentCulture` used for MSBuild names +- [ ] File path comparison ignores OS case sensitivity +- [ ] `DateTime` field lacks `Utc` suffix + +--- + +### 8. API Surface Discipline + +**Severity: MAJOR** + +See `../../documentation/wiki/Microsoft.Build.Framework.md`. + +**Rules:** +1. Default to `internal`. Only `public` with strong justification. +2. Use interfaces for extensibility points over `abstract class`. +3. New public API additions must be in `PublicAPI.Unshipped.txt`. +4. Never remove public API members — deprecate with `[Obsolete]`. +5. XML doc comments on all public members. + +**CHECK — Flag if:** +- [ ] A member is `public` without justification +- [ ] New public API missing from `PublicAPI.Unshipped.txt` +- [ ] Public member has no XML doc comment +- [ ] Public API member removed instead of deprecated +- [ ] `abstract class` where `interface` would suffice + +--- + +### 9. MSBuild Target Authoring Conventions + +**Severity: MAJOR** + +**Rules:** +1. Use `DependsOnTargets` for predecessors. Use `BeforeTargets`/`AfterTargets` sparingly. +2. Use proper MSBuild conditions — `'$(Prop)' == ''` patterns. +3. Respect SDK import ordering: `.props` before user code, `.targets` after. See `../../documentation/ProjectReference-Protocol.md`. +4. Incremental build targets require precise `Inputs`/`Outputs` declarations. + +**CHECK — Flag if:** +- [ ] `BeforeTargets`/`AfterTargets` where `DependsOnTargets` is clearer +- [ ] Target could break SDK import ordering contract +- [ ] `Inputs`/`Outputs` missing on incremental-build target +- [ ] Conditions reference properties not yet defined at evaluation point + +--- + +### 10. Design Before Implementation + +**Severity: MAJOR** + +**Rules:** +1. Discuss design tradeoffs before implementation for non-trivial features. +2. Complex features require a written spec — see `../../documentation/specs/`. +3. Make incremental, reviewable commits. Large monolithic PRs are rejected. +4. Follow established design patterns. Don't introduce new patterns without discussion. + +**CHECK — Flag if:** +- [ ] Large feature PR with no linked spec +- [ ] New architectural pattern introduced without discussion +- [ ] Single PR mixes multiple unrelated concerns +- [ ] Design trade-offs not articulated + +--- + +### 11. Cross-Platform Correctness + +**Severity: MAJOR** + +**Rules:** +1. Use cross-platform APIs. No hardcoded backslashes. Use `Path.Combine`, `FileUtilities`. +2. Handle .NET Framework vs .NET Core differences explicitly. +3. Handle UNC paths, long paths (`\\?\`), and symlinks. +4. Build output must not differ based on build machine OS. + +**CHECK — Flag if:** +- [ ] Hardcoded path separators +- [ ] Windows-only APIs without cross-platform fallback +- [ ] File path case sensitivity not considered +- [ ] Code assumes `\r\n` newlines +- [ ] `.NET Framework`-only API without `#if` guard + +--- + +### 12. Code Simplification + +**Severity: MODERATE** + +**Rules:** +1. Remove unnecessary conditions, flatten nested logic, collapse redundant branches. +2. Prefer simpler implementations when equally correct. +3. Use early returns, guard clauses, `switch` expressions for clear control flow. +4. Use existing helpers (`FileUtilities`, `MSBuildNameIgnoreCaseComparer`, `ResourceUtilities`). +5. Remove dead code proactively. + +**CHECK — Flag if:** +- [ ] >3 levels of nesting where guard clauses would flatten +- [ ] Custom implementation for something shared utilities provide +- [ ] Dead code or unused variables +- [ ] Complex expression replaceable by pattern match or ternary +- [ ] `if/else` chain replaceable by `switch` expression + +--- + +### 13. Concurrency & Thread Safety + +**Severity: BLOCKING** + +See `../../documentation/wiki/Nodes-Orchestration.md`. + +**Rules:** +1. Shared mutable state must be thread-safe (`ConcurrentDictionary`, `Interlocked`, explicit locking). +2. Synchronize access to state from multiple threads or nodes. +3. Handle in-proc vs out-of-proc node differences — behavior must be correct in both. +4. Consider IPC concurrency: message ordering, reentrancy, serialization. + +**CHECK — Flag if:** +- [ ] Shared field read/written without synchronization +- [ ] `static` mutable field without thread-safety analysis +- [ ] Code assumes single-threaded execution on parallel build paths +- [ ] In-proc/out-of-proc behavior differences untested + +--- + +### 14. Naming Precision + +**Severity: NIT** + +**Rules:** +1. Use clear, descriptive names. Avoid abbreviations unless universally understood (e.g., `PRE` for `ProjectRootElement`). +2. Be consistent with surrounding code naming. +3. Test methods: `MethodUnderTest_Scenario_ExpectedResult`. + +**CHECK — Flag if:** +- [ ] Ambiguous names (`data`, `result`, `temp`, `flag`) +- [ ] Naming inconsistent with adjacent code +- [ ] Boolean parameter meaning unclear +- [ ] Test method names don't describe what they test + +--- + +### 15. SDK Integration Boundaries + +**Severity: MAJOR** + +See `../../documentation/ProjectReference-Protocol.md`. + +**Rules:** +1. SDK changes must respect evaluation/execution phase separation. +2. Property defaults must not override user-specified values — use `Condition="'$(Prop)' == ''"`. +3. Project reference protocol changes require cross-stack coordination (MSBuild, SDK, NuGet, VS). +4. NuGet restore and Build must not run in the same evaluation. +5. Evaluation-time decisions in SDK targets have long-term architectural impact. + +**CHECK — Flag if:** +- [ ] SDK property default overwrites user values (missing `Condition`) +- [ ] Evaluation/execution concerns mixed +- [ ] Project reference protocol change not coordinated with SDK/NuGet +- [ ] Restore and Build conflated +- [ ] Design-time build contract violated + +--- + +### 16. Idiomatic C# Patterns + +**Severity: NIT** + +**Rules:** +1. Use modern C# features where supported: expression-bodied members, pattern matching, `using` declarations. +2. Match surrounding code style — MSBuild conventions may differ from general .NET guidance. +3. Handle nullability explicitly with annotations and `is null`/`is not null`. +4. Track .NET version constraints with `// TODO` for unavailable APIs. + +**CHECK — Flag if:** +- [ ] `using` block where surrounding code uses `using` declarations +- [ ] `== null` where `is null` is the codebase convention +- [ ] Nullable `!` suppression without explanation +- [ ] Older C# pattern where newer is idiomatic in codebase + +--- + +### 17. File I/O & Path Handling + +**Severity: MAJOR** + +**Rules:** +1. Use `FileUtilities` helpers for path normalization, comparison, and manipulation. +2. Handle UNC paths and long paths (`\\?\`). Test with deeply nested directories. +3. Globbing patterns must handle excludes correctly. See `../../documentation/WhenGlobbingReturnsOriginalFilespec.md`. + +**CHECK — Flag if:** +- [ ] Custom path normalization instead of `FileUtilities` +- [ ] File path comparison using `==` instead of OS-appropriate comparison +- [ ] UNC/long paths not considered +- [ ] Globbing doesn't account for exclude patterns + +--- + +### 18. Documentation Accuracy + +**Severity: MODERATE** + +**Rules:** +1. Code comments should explain _why_, not just _what_. +2. XML doc comments on public and complex internal code. +3. Use `learn.microsoft.com` URLs, not `docs.microsoft.com`. +4. Specs need problem statements, non-goals, and concrete examples. See `../../documentation/specs/`. + +**CHECK — Flag if:** +- [ ] `docs.microsoft.com` URL instead of `learn.microsoft.com` +- [ ] Complex method with no doc comment +- [ ] Spec lacks problem statement, non-goals, or examples +- [ ] Documentation inaccurate vs actual behavior +- [ ] Design decision undocumented + +--- + +### 19. Build Infrastructure Care + +**Severity: MAJOR** + +See `../../documentation/wiki/Bootstrap.md`. + +**Rules:** +1. Dependency versions must be pinned via Darc/Maestro. Manual edits to `eng/Versions.props` require justification. +2. Verify compatibility with all build entry points: Arcade CLI, VS, `dotnet build`, bootstrap. +3. CI/CD pipeline changes require validation before merge. +4. Follow VS servicing and branching conventions. + +**CHECK — Flag if:** +- [ ] `eng/Versions.props` manually edited without Darc justification +- [ ] CI YAML change not validated +- [ ] Bootstrap build compatibility not verified +- [ ] Change works in one build entry point but may fail in others + +--- + +### 20. Scope & PR Discipline + +**Severity: MODERATE** + +**Rules:** +1. Track follow-up work explicitly — create issues for deferred improvements. +2. Don't mix refactoring with behavioral changes in the same PR. +3. Cross-reference related issues and PRs. +4. Address reviewer concerns before merging. + +**CHECK — Flag if:** +- [ ] PR contains unrelated changes (formatting mixed with logic) +- [ ] Follow-up work mentioned but no issue created +- [ ] PR lacks references to related issues/specs +- [ ] Reviewer feedback unresolved at merge time + +--- + +### 21. Evaluation Model Integrity + +**Severity: BLOCKING** + +See `../../documentation/High-level-overview.md` and `../../documentation/Built-in-Properties.md`. + +**Rules:** +1. Respect evaluation order: environment → global properties → project properties (in file order with imports) → item definitions → items. +2. Understand how `Directory.Build.props`/`Directory.Build.targets` are injected: they are imported via `Microsoft.Common.props`/`Microsoft.Common.targets`, which may themselves be imported by SDK props/targets. +3. Evaluation-time decisions have long-term architectural impact — extremely hard to reverse. +4. Undefined metadata and empty-string metadata must be treated equivalently. + +**CHECK — Flag if:** +- [ ] Change alters property evaluation order +- [ ] Changes rely on incorrect assumptions about `Directory.Build.props`/`.targets` import ordering +- [ ] Evaluation-time side effects introduced +- [ ] Undefined metadata treated differently from empty-string + +--- + +### 22. Correctness & Edge Cases + +**Severity: MAJOR** + +**Rules:** +1. Verify edge cases: empty collections, null values, concurrent access, very large inputs, Unicode paths. +2. Verify behavior matches documented semantics. +3. Validate fixes against original repro steps. +4. Validate inputs early — fail fast with clear errors. + +**CHECK — Flag if:** +- [ ] New code path doesn't handle null/empty inputs +- [ ] Bug fix doesn't include original repro as test +- [ ] Boundary conditions not considered (off-by-one, max-length, empty) +- [ ] Input validation missing at public API boundaries + +--- + +### 23. Dependency Management + +**Severity: MODERATE** + +**Rules:** +1. Minimize unnecessary references. Each dependency is a compatibility constraint. +2. Use Darc/Maestro for dependency updates. Manual bumps require justification. +3. Binding redirect changes have high downstream impact — test thoroughly. + +**CHECK — Flag if:** +- [ ] New package reference without justification +- [ ] `eng/Versions.props` or `eng/Version.Details.xml` edited without Darc context +- [ ] Binding redirects changed without impact analysis + +--- + +### 24. Security Awareness + +**Severity: BLOCKING** + +**Rules:** +1. Security-relaxing parameters (e.g., `TrustServerCertificate`) must require explicit user opt-in. +2. Task type loading must unify to currently-running MSBuild assemblies. +3. Consider path traversal, symlink following, and temp file safety. + +**CHECK — Flag if:** +- [ ] Security-relaxing flag applied unconditionally +- [ ] Task assembly loading accepts untrusted paths without validation +- [ ] File operations exploitable via symlinks or path traversal +- [ ] Credentials or tokens logged/stored insecurely + +--- + +## MSBuild-Specific Knowledge Areas + +| # | Area | Key Rules | Docs | +|---|------|-----------|------| +| 1 | **Name Comparisons** | `MSBuildNameIgnoreCaseComparer` for property/item/target names. `OrdinalIgnoreCase` for identifiers. Never `CurrentCulture`. | — | +| 2 | **ChangeWave Mechanism** | Gate behind correct version. Test opt-out. Document in ChangeWaves file. | `../../documentation/wiki/ChangeWaves.md` | +| 3 | **Breaking Change Sensitivity** | New warnings break `WarnAsError`. Never remove CLI switches. Changed defaults break projects. | — | +| 4 | **Evaluation Order** | Properties: last-write wins. Conditions at point of appearance. | `../../documentation/High-level-overview.md` | +| 5 | **Target Ordering** | `DependsOnTargets` for predecessors. Incremental builds need `Inputs`/`Outputs`. | `../../documentation/wiki/Target-Maps.md` | +| 6 | **Binary Log** | All events captured. Format backward-compatible. Correct `MessageImportance`. | `../../documentation/wiki/Binary-Log.md` | +| 7 | **Node Architecture** | Test in-proc and out-of-proc. Shared state thread-safe. IPC serialization correct. | `../../documentation/wiki/Nodes-Orchestration.md` | +| 8 | **Error/Warning Codes** | `MSBxxxx` format. Unique. `ResourceUtilities` formatting. Actionable messages. | `../../documentation/assigning-msb-error-code.md` | +| 9 | **SDK-MSBuild Boundary** | SDK props before user code. Defaults must not override. Restore ≠ Build. | `../../documentation/ProjectReference-Protocol.md` | +| 10 | **VS Servicing Model** | Track VS version branches. Side-by-side installation compatibility. | — | +| 11 | **Resource String Patterns** | Embedded error codes at start. Changes semi-breaking. `.resx` for user-facing. | — | +| 12 | **Task Loading Model** | Tasks unify to running MSBuild assemblies. Loading differs Framework vs Core. | `../../documentation/wiki/Tasks.md` | + +--- + +## Folder Hotspot Mapping + +Use this to prioritize dimensions based on changed files. + +| Folder | Priority Dimensions | Hot Files | +|--------|---------------------|-----------| +| `src/Build/BackEnd/` | Concurrency, Performance, Correctness, Logging | `BuildManager.cs`, `NodeProviderOutOfProcBase.cs` | +| `src/Build/Evaluation/` | Evaluation Model, Performance, String Comparison, Compat | `Evaluator.cs`, `Expander.cs`, `IntrinsicFunctions.cs` | +| `src/Build/Logging/` | Logging, Design, API Surface | `ParallelConsoleLogger.cs`, `BuildEventArgsWriter.cs` | +| `src/Build/Construction/` | API Surface, Compat, Evaluation Model | `SolutionFile.cs`, `ProjectRootElement.cs` | +| `src/Build/Graph/` | Correctness, Performance, Simplification | `ProjectGraph.cs`, `GraphBuilder.cs` | +| `src/Build/Instance/` | API Surface, Performance, Correctness | `TaskRegistry.cs`, `ProjectInstance.cs` | +| `src/Build/Resources/` | Error Messages, Compat, ChangeWave | `Strings.resx` | +| `src/Tasks/` | Target Conventions, Cross-Platform, Compat | — | +| `src/Shared/` | Performance, Cross-Platform, File I/O | — | +| `src/MSBuild/` | Error Messages, Compat, Logging | — | +| `src/Framework/` | API Surface, Compat, Cross-Platform | — | +| `documentation/` | Documentation Accuracy, Design | specs | +| `eng/` | Build Infrastructure, Dependency Management | `Versions.props` | + +--- + +## Review Workflow + +### Wave 1: Find + +1. Map changed files to the [Folder Hotspot Mapping](#folder-hotspot-mapping). + +2. Launch **one sub-agent per dimension** (`task` tool, `agent_type: "general-purpose"`, `model: "claude-opus-4.6"`). Each agent evaluates exactly one dimension against the full PR diff. Run in **parallel batches of 6** (4 batches for 24 dimensions). + + Each sub-agent receives: the PR diff, PR description, the single dimension's rules and checklist, and the folder context. + + Include verbatim in every sub-agent prompt: + + > You evaluate **one dimension only**: $DimensionName. + > + > Report `$DimensionName — LGTM` when the dimension is genuinely clean. + > + > Report an ISSUE only when you can construct a **concrete failing scenario**: a specific thread interleaving, a specific null input, a specific call sequence that triggers the bug. No hypotheticals. + > + > Read the **PR diff**, not main — new files and methods only exist in the PR branch. + > + > **Concurrency**: identify every thread that reads/writes shared state. Map the timeline. Show overlapping unsynchronized access. + > **Correctness**: construct the exact input that fails (e.g., "null projectFileNames → NRE at .Length"). + > **Compatibility**: name the specific behavioral change and who it breaks. + > + > ``` + > $DimensionName — LGTM + > ``` + > ``` + > $DimensionName — ISSUE + > SEVERITY: BLOCKING | MAJOR | MODERATE | NIT + > FILE: path/to/file.cs + > LINES: 100-120 + > SCENARIO: + > FINDING: + > RECOMMENDATION: + > ``` + +### Wave 2: Validate + +3. For each non-LGTM finding, launch a validation agent that **proves or disproves it** using: + + - **Code flow tracing**: Read full source from the PR branch (`github-mcp-server-get_file_contents` with `ref: "refs/pull/{pr}/head"`). Trace callers, callees, locks, thread boundaries. + - **Build and test**: Build the PR branch locally. Run existing tests. Check coverage of the claimed scenario. + - **Proof-of-concept test**: Write a minimal test that demonstrates the issue — include in PR feedback as evidence. + - **Thread timeline**: For concurrency issues, write the interleaving step-by-step: + ``` + T=0 Thread-A: writes field X (line N) + T=1 Thread-A: yields, decrements counter + T=2 Main: starts Thread-B + T=3 Thread-B: writes field X (line M) ← unsynchronized + T=4 Thread-A: restores field X ← stomps Thread-B + ``` + + Output per finding: + ``` + VERDICT: CONFIRMED | DISPUTED + EVIDENCE: + TEST_SNIPPET: + ``` + + Confirm only with concrete evidence. Dispute if a lock, blocking call, or control flow prevents the scenario. **Never validate against `main`.** + +4. For borderline findings, run the same validation on 3 models (`claude-opus-4.6`, `gpt-5.2-codex`, `gemini-3-pro-preview`). Keep findings confirmed by ≥2/3. + +### Wave 3: Post + +5. Post **inline review comments** at the exact file and line via GitHub MCP or `gh` CLI. Format: + + ```markdown + **[$SEVERITY] $DimensionName** + + $Scenario that triggers the bug. + + **Thread timeline:** + T=0 Thread-A: ... + T=1 Thread-B: ... ← race + + **Proof-of-concept test:** + [Fact] + public void ConcurrentTasks_SharedState_NotCorrupted() { ... } + + **Recommendation:** $Fix. + ``` + +6. Post design-level concerns (not tied to a line) as a single PR comment — one bullet each. + +### Wave 4: Summary + +7. Post the summary table as the review body: + + ```markdown + | # | Dimension | Verdict | + |---|-----------|---------| + | 1 | Backwards Compatibility | ✅ LGTM | + | 13 | Concurrency | 🔴 2 MAJOR | + + - [x] Backwards Compat + - [ ] Concurrency — shared state race + ``` + + `[x]` = LGTM or NITs only. `[ ]` = MAJOR or BLOCKING. + All `[x]` → **APPROVE**. Any BLOCKING → **REQUEST_CHANGES**. Otherwise → **COMMENT**. diff --git a/.github/instructions/backend-execution.instructions.md b/.github/instructions/backend-execution.instructions.md new file mode 100644 index 00000000000..a88cd36cc4a --- /dev/null +++ b/.github/instructions/backend-execution.instructions.md @@ -0,0 +1,48 @@ +--- +applyTo: "src/Build/BackEnd/**" +--- + +# BackEnd & Execution Engine Instructions + +MSBuild's multi-process execution engine: `BuildManager`, node communication, scheduler, result caching, and task execution. + +## Concurrency & Thread Safety (Critical) + +* All shared mutable state must be synchronized — tasks run across in-proc and out-of-proc nodes concurrently. +* `BuildManager.cs` is the most complex file — `BeginBuild`/`EndBuild`/`ResetCaches` sequences must remain correct. +* Test in **both** in-proc and out-of-proc scenarios — they differ in state isolation, type loading, and serialization. +* Lock ordering must be consistent to prevent deadlocks. Document acquisition order when introducing new locks. + +## Node Communication & IPC + +* Never change the IPC packet format without versioning — old nodes must communicate with new ones during rolling updates. +* IPC message ordering matters — race conditions cause intermittent, hard-to-reproduce failures. +* Task host node (`NodeProviderOutOfProcTaskHost.cs`) has additional isolation constraints for type loading. + +## Scheduler Correctness + +* Changes can cause deadlocks, starvation, or incorrect parallelism. +* Yield/unyield semantics must be preserved — tasks that yield allow their node to process other requests. + +## Result Caching + +* Results are cached by `(project path, global properties, targets)` — changes to cache key computation break incremental builds. +* Cache coherence between nodes is critical — stale results cause incorrect builds. +* See [Results Cache](../../documentation/wiki/Results-Cache.md) and [Cache Flow](../../documentation/wiki/CacheFlow.png). + +## SDK Resolution + +* `SdkResolverService.cs` resolves SDK references during evaluation — changes affect every SDK-style project. +* SDK resolution must not have side effects that persist across evaluations. + +## BuildManager Lifecycle + +* `BeginBuild` → submissions → `EndBuild` is the required sequence. Handle reentrant calls and out-of-order events gracefully. +* `ResetCaches` must not lose in-flight results. + +## Related Documentation + +* [Nodes Orchestration](../../documentation/wiki/Nodes-Orchestration.md) +* [Results Cache](../../documentation/wiki/Results-Cache.md) +* [Logging Internals](../../documentation/wiki/Logging-Internals.md) +* [Threading spec](../../documentation/specs/threading.md) diff --git a/.github/instructions/buildcheck.instructions.md b/.github/instructions/buildcheck.instructions.md new file mode 100644 index 00000000000..91ef76013da --- /dev/null +++ b/.github/instructions/buildcheck.instructions.md @@ -0,0 +1,49 @@ +--- +applyTo: "src/Build/BuildCheck/**,src/Framework/BuildCheck/**" +--- + +# BuildCheck Instructions + +MSBuild's analyzer infrastructure enabling built-in and third-party build analyzers. + +## Analyzer Authoring + +* Analyzers must not throw — wrap logic in try/catch and report failures via infrastructure. +* Must be stateless between projects or explicitly manage state via context. Shared mutable state causes concurrency bugs in multi-node builds. +* Built-in analyzers serve as examples for third-party authors — keep them clean and idiomatic. + +## Diagnostic Codes + +* Every diagnostic must have a unique code — see [Codes](../../documentation/specs/BuildCheck/Codes.md). +* Codes are permanent — once assigned, cannot be reused or reassigned. +* Messages must be actionable: state what was detected and what to do. + +## Severity Handling + +* Escalation chain: `Suggestion` → `Warning` → `Error`. +* Users configure severity per-analyzer in `.editorconfig` or MSBuild properties. +* Default to `Suggestion` or `Warning` — `Error` blocks builds and requires high confidence. + +## Acquisition & Extensibility + +* Third-party analyzers load via NuGet packages — changes to the loading pipeline affect the ecosystem. +* Analyzer interfaces in `Framework/BuildCheck/` are public API — version contract interfaces, breaking changes require a new version. + +## Performance Impact + +* Analyzers must not measurably slow down builds. +* Avoid per-item/per-property callbacks for analyzers that only need project-level data. +* Cache results when the same check runs across projects with identical configuration. + +## Architecture + +* Engine logic in `src/Build/BuildCheck/`, contracts in `src/Framework/BuildCheck/`. +* Data flow: evaluation/execution → BuildCheck infrastructure → analyzer → diagnostic output. +* Cross-node remoting must be handled correctly — see [architecture doc](../../documentation/specs/BuildCheck/BuildCheck-Architecture.md). + +## Related Documentation + +* [BuildCheck Architecture](../../documentation/specs/BuildCheck/BuildCheck-Architecture.md) +* [BuildCheck Feature Spec](../../documentation/specs/BuildCheck/BuildCheck.md) +* [Custom BuildCheck Analyzers](../../documentation/specs/BuildCheck/CustomBuildCheck.md) +* [Diagnostic Codes](../../documentation/specs/BuildCheck/Codes.md) diff --git a/.github/instructions/cli.instructions.md b/.github/instructions/cli.instructions.md new file mode 100644 index 00000000000..e4552a4ae4d --- /dev/null +++ b/.github/instructions/cli.instructions.md @@ -0,0 +1,48 @@ +--- +applyTo: "src/MSBuild/**" +--- + +# MSBuild CLI Instructions + +Command-line entry point (`XMake.cs`), argument parsing, server mode, and CLI-specific logic. + +## CLI Switch Stability (Critical) + +* **Never remove or rename existing switches or aliases** — build scripts worldwide depend on them. +* New switches must not conflict with existing switches or their abbreviations. +* Switch abbreviation rules must be preserved — existing shortest-unique prefixes must continue to work. + +## XMake.cs Entry Point + +* Exit codes must remain stable — scripts check specific exit codes. +* Startup performance matters — avoid unnecessary initialization on the critical path. +* Top-level error handling must catch and report all exceptions with actionable messages. + +## Server Mode + +* Server mode keeps processes alive between builds — state leaks cause intermittent failures. +* Ensure all per-build state is properly reset between builds. +* See [threading spec](../../documentation/specs/threading.md) for concurrency constraints. + +## Command-Line Parsing + +* Backward compatible — existing valid command lines must work identically. +* Boolean switches: `-switch`, `-switch:true`, `-switch:false` — handle all forms. +* Response file (`@file`) processing must maintain ordering and nesting semantics. + +## CLI Behavior Compatibility + +* Default verbosity, output format, and behavior must not change without a [ChangeWave](../../documentation/wiki/ChangeWaves.md). +* The set of automatically-forwarded properties to child nodes must remain stable. +* Changes to project discovery (`.sln` vs `.slnx` handling) require careful compatibility analysis. + +## Error Messages + +* CLI-level errors (bad arguments, missing project files) must be immediately actionable. +* Include the `/help` pointer when appropriate. + +## Related Documentation + +* [MSBuild Environment Variables](../../documentation/wiki/MSBuild-Environment-Variables.md) +* [ChangeWaves](../../documentation/wiki/ChangeWaves.md) +* [MSBuild apphost spec](../../documentation/specs/msbuild-apphost.md) diff --git a/.github/instructions/evaluation.instructions.md b/.github/instructions/evaluation.instructions.md new file mode 100644 index 00000000000..aa0053585da --- /dev/null +++ b/.github/instructions/evaluation.instructions.md @@ -0,0 +1,43 @@ +--- +applyTo: "src/Build/Evaluation/**" +--- + +# Evaluation Engine Instructions + +The evaluation engine (`Evaluator.cs`, `Expander.cs`, `LazyItemEvaluator.cs`) is MSBuild's hottest code path. Every project load passes through here. + +## Evaluation Model Integrity + +* Strict evaluation order: environment → global properties → project properties (file order with imports) → item definitions → items. Never alter this order. +* Conditions are evaluated at the point they appear, not deferred. +* Undefined metadata and empty-string metadata must be treated equivalently. +* Property precedence is last-write-wins within the import chain. +* Gate evaluation behavior changes behind a [ChangeWave](../../documentation/wiki/ChangeWaves.md). + +## Expander Safety + +* `Expander.cs` is called millions of times per evaluation — every allocation counts. +* Cache expanded values when the same expression is expanded repeatedly. + +## IntrinsicFunctions + +* New intrinsic functions are permanent public API — can never be removed once shipped. +* Validate all inputs; called from user-authored MSBuild with arbitrary arguments. +* Security-sensitive functions (file I/O, registry, environment) must check for opt-in. +* Test edge cases: null, empty strings, very long strings, culture-sensitive formatting. + +## Condition Evaluation + +* Condition parsing is allocation-sensitive — prefer `Span`-based parsing. +* Boolean conditions must short-circuit correctly. +* String comparisons in conditions use MSBuild semantics (case-insensitive for identifiers). + +## ProjectRootElementCache + +* Cache invalidation bugs cause stale evaluations or memory leaks — test eviction scenarios. +* Thread safety is critical — the cache is accessed from multiple nodes. + +## Related Documentation + +* [ChangeWaves](../../documentation/wiki/ChangeWaves.md) +* [Target Maps](../../documentation/wiki/Target-Maps.md) diff --git a/.github/instructions/framework.instructions.md b/.github/instructions/framework.instructions.md new file mode 100644 index 00000000000..141a21dff39 --- /dev/null +++ b/.github/instructions/framework.instructions.md @@ -0,0 +1,47 @@ +--- +applyTo: "src/Framework/**" +--- + +# MSBuild Framework Instructions + +`Microsoft.Build.Framework` defines MSBuild's public API contracts: interfaces, base types, event args, and extensibility points. Referenced by every task and logger author. + +## API Surface Discipline (Critical) + +* Default to `internal`. Every `public` member is a permanent commitment. +* New public API **must** be recorded in `PublicAPI.Unshipped.txt`. +* Add XML doc comments to all public members. +* Seal classes unless explicitly designed for inheritance. Prefer interfaces for extensibility. + +## Public API Compatibility + +* Never remove or change signatures of existing public members — add new overloads. +* Interface additions require default interface method implementations to avoid breaking implementors. +* Binary compatibility matters — task assemblies compiled against older Framework versions must continue to work. + +## Event Args & Build Events + +* Event args are serialized in binary logs — adding fields requires backward-compatible serialization. See [Binary Log](../../documentation/wiki/Binary-Log.md). +* New event types must integrate with `IEventSource`, forwarding loggers, and binary log reader/writer. +* `MessageImportance` levels: `High` = user-critical, `Normal` = standard, `Low` = verbose. + +## BuildCheck Contracts + +* Analyzer interfaces define the contract with third-party analyzers — treat as public API. +* See [BuildCheck Architecture](../../documentation/specs/BuildCheck/BuildCheck-Architecture.md). + +## Serialization Stability + +* Types serialized across IPC or persisted in binary logs must maintain format stability. +* When adding fields, handle the case where the field is missing (backward compat with older writers). +* Use `ITranslatable` for IPC serialization; follow existing patterns. + +## Interface Design + +* `IBuildEngine` versions follow a progression pattern — new task capabilities go in the next numbered interface. +* Ensure new interfaces are implemented on `TaskExecutionHost`. + +## Related Documentation + +* [Microsoft.Build.Framework](../../documentation/wiki/Microsoft.Build.Framework.md) +* [BuildCheck specs](../../documentation/specs/BuildCheck/BuildCheck.md) diff --git a/.github/instructions/globbing.instructions.md b/.github/instructions/globbing.instructions.md new file mode 100644 index 00000000000..0c6c813aa64 --- /dev/null +++ b/.github/instructions/globbing.instructions.md @@ -0,0 +1,38 @@ +--- +applyTo: "src/Build/Globbing/**" +--- + +# Globbing Instructions + +Resolves file patterns (e.g., `**/*.cs`) for item includes/excludes. Runs during evaluation — performance-critical. + +## Glob Pattern Correctness + +* Semantics: `*` matches within a directory, `**` across directories, `?` a single character. +* Include and exclude patterns must use the same matching rules. +* Handle edge cases: empty patterns, only-wildcard patterns, literal special characters, trailing separators. +* Relative vs absolute patterns must produce consistent results regardless of working directory. + +## Performance + +* Minimize filesystem enumeration — prune directory traversal early using glob structure. +* Cache results when the same pattern is evaluated multiple times (common with `**/*.cs` across imports). +* Avoid allocating intermediate string collections — use lazy evaluation where possible. + +## Exclude Patterns + +* Excludes are applied after includes, not during. +* `Remove` with globs must use the same matching engine. +* Test with nested excludes (e.g., `**/*.cs` include with `**/obj/**` exclude). + +## Evaluation-Time Behavior + +* Globs resolve at evaluation time — filesystem state at that point determines the item list. +* Changes affect all SDK-style projects (implicit `**/*.cs` includes). +* Gate behavioral changes behind a [ChangeWave](../../documentation/wiki/ChangeWaves.md). + +## Cross-Platform + +* Glob patterns must work with both `\` and `/` separators. +* Case sensitivity must follow OS filesystem conventions. +* Symlink traversal must be safe (no infinite loops). diff --git a/.github/instructions/logging.instructions.md b/.github/instructions/logging.instructions.md new file mode 100644 index 00000000000..029b04087f7 --- /dev/null +++ b/.github/instructions/logging.instructions.md @@ -0,0 +1,43 @@ +--- +applyTo: "src/Build/Logging/**" +--- + +# Logging Infrastructure Instructions + +Binary logger, console loggers, terminal logger, and event forwarding infrastructure. + +## Binary Log Format Stability + +* `.binlog` format must maintain backward compatibility — older readers must handle newer logs. +* `BuildEventArgsWriter.cs`/`BuildEventArgsReader.cs` are the serialization boundary — field additions must be versioned. +* Always write new fields at the end of existing records. Readers must handle missing fields. +* Test round-trip: write → read → compare for all modified event types. + +## Terminal Logger (FancyLogger) + +* Must handle terminal width changes, very narrow terminals, and non-TTY output (piped to file). +* Concurrent project builds must render without corruption. +* ANSI escape sequences must be cross-platform compatible. + +## Console Logger + +* `ParallelConsoleLogger.cs` handles multi-project console output. +* `MessageImportance` filtering: `High` always shows, `Normal` at normal verbosity, `Low` at detailed. +* Never change the default output format without a [ChangeWave](../../documentation/wiki/ChangeWaves.md) — build log parsers depend on it. + +## Build Event Handling + +* Event forwarding between nodes must preserve ordering and completeness. +* Central loggers see all events; distributed loggers see only their node's events. See [Logging Internals](../../documentation/wiki/Logging-Internals.md). + +## Diagnostics Completeness + +* Behavioral changes must produce corresponding binary log entries. +* Error/warning events must include file, line, and column when available. +* Prefer structured events over string messages for programmatic consumption. + +## Related Documentation + +* [Binary Log](../../documentation/wiki/Binary-Log.md) +* [Logging Internals](../../documentation/wiki/Logging-Internals.md) +* [Providing Binary Logs](../../documentation/wiki/Providing-Binary-Logs.md) diff --git a/.github/instructions/shared-code.instructions.md b/.github/instructions/shared-code.instructions.md new file mode 100644 index 00000000000..7ff95fdf661 --- /dev/null +++ b/.github/instructions/shared-code.instructions.md @@ -0,0 +1,35 @@ +--- +applyTo: "src/Shared/**" +--- + +# Shared Code Instructions + +Code in `src/Shared/` is linked (compiled) into multiple MSBuild assemblies. A bug here breaks multiple assemblies simultaneously. + +## Cross-Assembly Impact + +* Compiled into `Microsoft.Build`, `Microsoft.Build.Tasks`, `Microsoft.Build.Utilities`, and the CLI. +* Test changes against all consuming assemblies, not just one. +* `#if` conditional compilation is used extensively — verify behavior for all target configurations (`FEATURE_*`, `RUNTIME_TYPE_NETCORE`, etc.). + +## IPC Packet Stability + +* `NodePacketTranslator` and `ITranslatable` implementations define the wire format between MSBuild nodes. +* Never change packet layout without versioning — old out-of-proc nodes must communicate with new in-proc nodes. +* Serialization must handle missing fields gracefully (forward compatibility). +* Test IPC round-trip for all modified packet types. + +## FileUtilities Safety + +* Use `FileUtilities.cs` instead of raw `System.IO.Path` calls. +* Must handle: UNC paths, long paths (> MAX_PATH), trailing separators, relative paths, embedded `.`/`..` segments. +* Avoid unnecessary file system calls — slow and can fail on network paths. + +## Cross-Platform Correctness + +* Handle .NET Framework vs .NET Core differences with appropriate `#if` guards. +* Environment variable access patterns differ across platforms — use the shared helpers. + +## Related Documentation + +* [Contributing Code](../../documentation/wiki/Contributing-Code.md) diff --git a/.github/instructions/targets.instructions.md b/.github/instructions/targets.instructions.md new file mode 100644 index 00000000000..df1aa50b834 --- /dev/null +++ b/.github/instructions/targets.instructions.md @@ -0,0 +1,47 @@ +--- +applyTo: "src/Tasks/**/*.targets,src/Tasks/**/*.props" +--- + +# Targets & Props Authoring Instructions + +`.targets` and `.props` files define MSBuild's default build logic. Changes here affect every .NET project. + +## Target Ordering + +* Use `DependsOnTargets` for required predecessors — primary ordering mechanism. +* Use `BeforeTargets`/`AfterTargets` sparingly — harder to reason about and debug. +* Test execution order with project references and multi-targeting scenarios. +* New targets must hook into the correct extensibility point (e.g., `BuildDependsOn`, `CompileDependsOn`). + +## Condition Patterns + +* Use `Condition="'$(PropertyName)' == ''"` to set defaults without overriding user values. +* `.props` files set defaults; `.targets` files should not override user-set properties. +* Conditions are evaluated in file order — cannot reference properties set later. +* Always use single-quotes for MSBuild string comparisons in conditions. + +## Backwards Compatibility + +* Changing property defaults breaks projects that relied on the previous default. +* Removing or renaming a target is a breaking change — may be referenced by `DependsOnTargets` in user projects. +* Adding items to existing item types can change build output unexpectedly. +* Gate behavioral changes behind a [ChangeWave](../../documentation/wiki/ChangeWaves.md). + +## Incremental Build Correctness + +* `Inputs`/`Outputs` declarations must be precise: + - `Inputs`: all files that affect the target's output. + - `Outputs`: all files the target produces. + - Missing inputs → stale builds. Missing outputs → unnecessary rebuilds. +* See [Rebuilding when nothing changed](../../documentation/wiki/Rebuilding-when-nothing-changed.md). + +## SDK Target Interaction + +* SDK `.props` import before user project content; SDK `.targets` import after. +* Property defaults in SDK props must use `Condition="'$(Prop)' == ''"` so user values win. +* Do not assume execution order between SDK targets and user-authored targets. + +## Related Documentation + +* [ChangeWaves](../../documentation/wiki/ChangeWaves.md) +* [Target Maps](../../documentation/wiki/Target-Maps.md) diff --git a/.github/instructions/tasks.instructions.md b/.github/instructions/tasks.instructions.md new file mode 100644 index 00000000000..ed006eb5dde --- /dev/null +++ b/.github/instructions/tasks.instructions.md @@ -0,0 +1,43 @@ +--- +applyTo: "src/Tasks/**/*.cs" +--- + +# Built-in Tasks Instructions + +Built-in tasks ship with MSBuild and cannot be independently versioned. + +## Backwards Compatibility + +* Gate behavioral changes behind a [ChangeWave](../../documentation/wiki/ChangeWaves.md). +* Never remove or rename `[Output]` properties — downstream targets depend on them by name. +* Adding new `[Required]` properties is a breaking change for existing `UsingTask` declarations. +* New optional parameters must default to preserving existing behavior. + +## Task Authoring Patterns + +* Extend `Task` or `ToolTask`. Use `TaskLoggingHelper` for logging, not `Console.WriteLine`. +* Validate inputs early in `Execute()` — fail fast with `Log.LogError` using MSBxxxx codes. +* `[Output]` properties must be set before returning `true`. +* All user-facing strings go in `.resx` files; use `ResourceUtilities.FormatResourceStringStripCodeAndKeyword` for formatting. + +## ResolveAssemblyReference (RAR) + +* Most complex built-in task — changes require extensive testing across framework targeting scenarios. +* RAR performance is critical — it runs for every project and can dominate build time. +* See [RAR docs](../../documentation/wiki/ResolveAssemblyReference.md) and [core scenarios](../../documentation/specs/rar-core-scenarios.md). + +## Path Handling + +* Use `FileUtilities` helpers — do not roll custom path manipulation. +* Support UNC paths, long paths (> 260 chars), and cross-platform separators. + +## Multithreaded Task Migration + +* Tasks must not hold locks across yield points. +* Shared static state is a concurrency hazard in multi-process builds. + +## Related Documentation + +* [Contributing Tasks](../../documentation/wiki/Contributing-Tasks.md) +* [Tasks](../../documentation/wiki/Tasks.md) +* [Task Isolation](../../documentation/specs/task-isolation-and-dependencies.md) diff --git a/.github/skills/backwards-compatibility/SKILL.md b/.github/skills/backwards-compatibility/SKILL.md new file mode 100644 index 00000000000..a485eda2e1e --- /dev/null +++ b/.github/skills/backwards-compatibility/SKILL.md @@ -0,0 +1,88 @@ +--- +name: assessing-breaking-changes +description: 'Guides assessment of backward compatibility for MSBuild changes. Consult when modifying behavior, adding warnings or errors, changing defaults, altering target ordering, removing or deprecating features, deciding whether a change needs a ChangeWave, reviewing blast radius of behavioral changes, or when a PR introduces user-visible output differences.' +argument-hint: 'Describe the change and its potential compatibility impact.' +--- + +# Backward Compatibility in MSBuild + +Backward compatibility is the default — any change that could alter existing build behavior must be explicitly justified. + +This skill covers **how to evaluate compatibility risk**. For the mechanics of ChangeWave implementation, see the [changewaves skill](../changewaves/SKILL.md). + +## Core Philosophy + +1. **Existing builds must not break.** If a project built successfully yesterday, it must build successfully today with identical semantics. +2. **New warnings are breaking changes.** Builds that use `-WarnAsError` or `true` will fail if you introduce a new warning. Always gate new warnings behind a ChangeWave or emit them as `Message` importance instead. +3. **Output changes are behavioral changes.** Even "improvements" to output formatting, file paths, or diagnostic text can break downstream consumers that parse MSBuild output. +4. **Removal is nearly impossible.** Never remove CLI switches, public APIs, or property names. Deprecate with warnings first, then gate removal behind a ChangeWave after multiple release cycles. + +## Blast Radius Checklist + +Before merging any behavioral change, evaluate: + +| Question | If Yes | +|----------|--------| +| Does this change what gets built or how? | ChangeWave required | +| Does this add a new warning? | ChangeWave required (WarnAsError impact) | +| Does this change a property default? | ChangeWave required (existing .csproj files depend on defaults) | +| Does this alter target execution order? | Test with real-world solutions; likely ChangeWave | +| Does this change console or binlog output format? | Consider downstream tool impact | +| Does this affect only internal code paths with no user-visible effect? | No ChangeWave needed | +| Is this a pure bug fix restoring documented behavior? | Usually no ChangeWave; use judgment on blast radius | + +## When ChangeWave Is NOT Needed + +- Internal refactoring with no observable behavior change +- Performance improvements that don't change semantics +- New opt-in features gated by a new property or switch +- Bug fixes that restore clearly-intended behavior with limited blast radius + +For detailed ChangeWave mechanics, see [ChangeWaves-Dev.md](../../../documentation/wiki/ChangeWaves-Dev.md) and [ChangeWaves.md](../../../documentation/wiki/ChangeWaves.md). + +## The Warnings-as-Errors Rule + +This is the most commonly missed compatibility concern: + +```xml + +true +``` + +Any new `MSBxxxx` warning you add will **break these builds**. Options: +1. **Gate behind ChangeWave** — preferred for genuinely important warnings +2. **Use `MessageImportance.Low` or `Normal`** — for informational diagnostics +3. **Add as an error from the start** — if the condition is always wrong + +## Deprecation Protocol + +1. Add a deprecation warning (behind ChangeWave if broad impact) +2. Document the deprecation in release notes +3. Maintain the old behavior for at least two major .NET versions +4. Only remove after the ChangeWave has rotated out + +## Compatibility Test Matrix + +When testing backward compatibility, verify: + +- **Multi-targeting projects** — `net472;net8.0` +- **Solution builds with mixed project types** — C#, F#, VB, C++/CLI +- **Incremental builds** — second build should be a no-op +- **Design-time builds** — Visual Studio calls different target contracts +- **Cross-platform** — path separator differences, case sensitivity on Linux +- **WarnAsError builds** — explicitly test with `true` + +## Decision Framework + +``` +Is the change user-visible? +├── No → Ship it (no ChangeWave needed) +└── Yes + ├── Is it a new opt-in feature? → Ship it (no ChangeWave needed) + └── Does it change existing behavior? + ├── Bug fix with low blast radius? → Ship it, add regression test + └── Behavioral change or new warning? + └── Gate behind ChangeWave, test opt-out path +``` + +Document the compatibility decision in your PR description so reviewers can validate it. diff --git a/.github/skills/binary-log-considerations/SKILL.md b/.github/skills/binary-log-considerations/SKILL.md new file mode 100644 index 00000000000..c540b62451a --- /dev/null +++ b/.github/skills/binary-log-considerations/SKILL.md @@ -0,0 +1,130 @@ +--- +name: maintaining-binary-log-compatibility +description: 'Guides changes to MSBuild binary log infrastructure. Consult when modifying BinaryLogger or BinaryLogReplayEventSource, adding new BuildEventArgs types, changing event serialization/deserialization, modifying ProjectImportsCollector, adjusting message importance levels, or making changes that affect .binlog content. Also applies when verifying that behavioral changes are properly reflected in binary log output.' +argument-hint: 'Describe the binary log change or event serialization concern.' +--- + +# Binary Log Considerations + +The binary log (`.binlog`) is MSBuild's primary diagnostic format and source of truth for build analysis. It captures the complete build event stream with full fidelity. Format changes have strict compatibility requirements. + +For general binary log usage, see [Binary-Log.md](../../../documentation/wiki/Binary-Log.md). + +## Core Principles + +1. **The binlog captures everything.** All meaningful build events must be represented. If your change alters build behavior, it must be observable in the binlog. +2. **Format changes must be backward-compatible.** Older versions of MSBuild Structured Log Viewer and other tools must be able to read binlogs produced by newer MSBuild, at minimum gracefully degrading. +3. **Forward compatibility matters too.** Newer viewers should handle binlogs from older MSBuild without crashing, even if some events are unrecognized. + +## Architecture Overview + +``` +Build Engine + → BuildEventArgs (structured events) + → BinaryLogger (serializes to .binlog) + → ProjectImportsCollector (embeds project/targets files) + +Replay: + .binlog file + → BinaryLogReplayEventSource (deserializes) + → Any ILogger (console, structured log viewer, analyzers) +``` + +Key source files: +- `src/Build/Logging/BinaryLogger/BinaryLogger.cs` — the logger itself +- `src/Framework/BuildEventArgs.cs` — base class for all build events +- `src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs` — captures imported files + +## Adding New Build Event Types + +When adding a new `BuildEventArgs` subclass: + +1. **Define the new event class** inheriting from the appropriate base (`BuildMessageEventArgs`, `BuildWarningEventArgs`, etc.) +2. **Add serialization support** — implement `WriteToStream` and `CreateFromStream` methods +3. **Increment the binary log version** if the new event type changes the format +4. **Add a new record type constant** in the binary logger's record type enum +5. **Handle the unknown-type case** in the replay source — older readers must skip gracefully + +### Serialization Compatibility Rules + +- **Never remove fields** from existing event args serialization +- **New fields must be appended** to the end of the serialization stream +- **Use version checks** when reading — if the binlog version is older, use defaults for new fields +- **Nullable fields** should serialize a presence flag before the value + +```csharp +// Pattern for backward-compatible field addition +if (logVersion >= newFieldVersion) +{ + writer.Write(newField); +} + +// Reading with backward compatibility +if (logVersion >= newFieldVersion) +{ + newField = reader.ReadString(); +} +else +{ + newField = defaultValue; +} +``` + +## Message Importance Levels + +Importance controls what appears in console output, but **everything goes to binlog** regardless of importance. + +| Level | Use For | Console Verbosity | +|-------|---------|------------------| +| `High` | Critical user-facing information | Minimal and above | +| `Normal` | Standard build progress | Normal and above | +| `Low` | Detailed diagnostic information | Detailed and above | +| `Diagnostic` | Internal debugging | Diagnostic only | + +### Rules + +- Default to `Normal` for user-relevant information +- Use `Low` for information useful when debugging but noisy in normal builds +- `High` is reserved for important warnings/status — use sparingly +- Never skip logging because "it's too verbose" — log at `Low` instead + +## ProjectImportsCollector + +The `ProjectImportsCollector` embeds all imported `.props`, `.targets`, and project files into the binlog. This enables the "preprocessed view" in log viewers. + +### Considerations + +- `MSBUILDLOGIMPORTS=1` (or the `/bl` switch) enables import collection +- Imported file content is captured at evaluation time — reflects the actual content used +- Large import chains increase binlog size — this is acceptable for diagnostic completeness +- Sensitive content in imported files will be embedded — document this for users + +## Changes That Affect Binlog Content + +When modifying MSBuild behavior, verify binlog impact: + +| Change Type | Binlog Consideration | +|------------|---------------------| +| New property set during evaluation | Appears in `PropertyInitialValue` or `PropertyReassignment` events | +| New target added | Produces `TargetStarted`/`TargetFinished` events | +| Changed task behavior | Task output items/properties captured in `TaskFinished` | +| New warning/error | Captured as `BuildWarningEventArgs`/`BuildErrorEventArgs` | +| Modified import chain | Changes which files `ProjectImportsCollector` captures | + +## Testing Binlog Changes + +- **Round-trip test**: Write a binlog, replay it, verify all events are reconstructed +- **Version compatibility test**: Verify older replay sources handle new events gracefully +- **Content verification**: Assert specific events appear in the binlog for behavioral changes +- **Size regression**: Monitor binlog size for unexpectedly large increases +- Use `BinaryLogReplayEventSource` in tests to verify binlog content programmatically + +## Checklist + +- [ ] New event types have `WriteToStream`/`CreateFromStream` implementations +- [ ] Binary log format version incremented if format changed +- [ ] Backward compatibility: older readers skip/degrade gracefully +- [ ] Forward compatibility: newer readers handle old format +- [ ] Importance levels set correctly for new messages +- [ ] Behavioral changes produce observable binlog events +- [ ] Round-trip test passes (serialize → deserialize → verify) diff --git a/.github/skills/changewaves/SKILL.md b/.github/skills/changewaves/SKILL.md index b6afe26e7cc..a159646d565 100644 --- a/.github/skills/changewaves/SKILL.md +++ b/.github/skills/changewaves/SKILL.md @@ -6,7 +6,9 @@ argument-hint: 'Add, query, or remove changewaves and changewave checks.' # Managing MSBuild Change Waves -A Change Wave is an opt-out flag that groups risky features together. Users disable features by setting the environment variable `MSBUILDDISABLEFEATURESFROMVERSION` to the wave version. This skill covers the full lifecycle: creating a wave, conditioning code on it, testing, documenting, and retiring. +A Change Wave is an opt-out flag that groups risky features together. Users disable features by setting the environment variable `MSBUILDDISABLEFEATURESFROMVERSION` to the wave version. This skill covers the **how** — the full lifecycle: creating a wave, conditioning code on it, testing, documenting, and retiring. + +For the **when** — deciding whether a change is a breaking change and whether it needs a ChangeWave at all — see [assessing-breaking-changes](../backwards-compatibility/SKILL.md). ## Decide Whether a Change Wave Is Appropriate diff --git a/.github/skills/error-and-warning-authoring/SKILL.md b/.github/skills/error-and-warning-authoring/SKILL.md new file mode 100644 index 00000000000..6668bad1e68 --- /dev/null +++ b/.github/skills/error-and-warning-authoring/SKILL.md @@ -0,0 +1,135 @@ +--- +name: authoring-errors-and-warnings +description: 'Guides authoring of MSBuild errors, warnings, and diagnostic messages. Consult when adding new MSBxxxx codes, writing or modifying user-facing diagnostic text, deciding between error/warning/message severity, working with Strings.resx resource files, formatting paths in error output, or evaluating whether a new warning could break WarnAsError builds.' +argument-hint: 'Describe the error/warning being added or modified.' +--- + +# Error and Warning Authoring in MSBuild + +Error messages are MSBuild's primary user interface. They must help developers fix problems without reading source code. + +For the mechanics of error code assignment, see [assigning-msb-error-code.md](../../../documentation/assigning-msb-error-code.md). + +## Error Message Quality Rules + +Every error message must answer three questions: + +1. **What happened?** — State the problem clearly +2. **Why?** — Provide context (file, property, expected value) +3. **What should the user do?** — Give actionable guidance + +```xml + +MSB4999: Invalid configuration. + + +MSB4999: The project "{0}" specifies TargetFramework "{1}" which is not installed. Install the SDK for "{1}" or update the TargetFramework in the project file. +``` + +## Severity Decision Framework + +``` +Is the condition always wrong (invalid input, impossible state)? +├── Yes → ERROR (build should fail) +└── No + ├── Could this cause subtle build correctness issues? + │ ├── Yes, likely → WARNING (but see WarnAsError impact below) + │ └── Yes, maybe → MESSAGE at Normal importance + └── Is this purely informational? + └── Yes → MESSAGE at Low importance +``` + +### The WarnAsError Constraint + +**New warnings are breaking changes** for builds using: +- `-WarnAsError` (CLI) +- `true` (project) +- `MSBxxxx` (specific codes) + +Before adding a new warning, consider: +1. **Is the warning important enough to justify breaking WarnAsError builds?** If yes, gate behind a ChangeWave (see [changewaves skill](../changewaves/SKILL.md)). +2. **Could this be a Message instead?** Messages never break builds but still appear in binary logs. +3. **Should this be an error from the start?** If the condition is always wrong, skip the warning stage. + +## MSB Error Code Assignment + +### Code Ranges + +| Range | Area | +|-------|------| +| `MSB1xxx` | Command-line handling | +| `MSB3xxx` | Tasks (`Microsoft.Build.Tasks.Core.dll`) | +| `MSB4xxx` | Engine (`Microsoft.Build.dll`) | +| `MSB5xxx` | Shared code across assemblies | +| `MSB6xxx` | Utilities (`Microsoft.Build.Utilities`) | + +### Assignment Process + +1. Open the `Strings.resx` file for the appropriate assembly +2. Find the "Next message code" comment at the bottom of the file +3. Search the repo to confirm the code is not already in use +4. Use the code; update the comment to the next available number + +See [assigning-msb-error-code.md](../../../documentation/assigning-msb-error-code.md) for detailed steps. + +## Resource String Format + +```xml + + MSBxxxx: Clear description with {0} placeholders for runtime values. + {StrBegin="MSBxxxx: "}{0} is the project file path. {1} is the property name. + +``` + +### Rules + +- Resource name uses `FeatureArea.DescriptiveName` convention +- Value starts with `MSBxxxx: ` (code, colon, space) +- Comment documents the `{StrBegin}` marker and explains each `{N}` placeholder +- Comments guide translators — mention untranslatable tokens + +## Consuming Error Resources in Code + +Use the standard formatting method that extracts and applies the error code: + +```csharp +// For errors +Log.LogErrorWithCodeFromResources("Copy.Error", sourceFile, destFile, ex.Message); + +// For warnings +Log.LogWarningWithCodeFromResources("ResolveAssemblyReference.Conflict", assemblyName); + +// For engine-level errors (not in tasks) +ProjectFileErrorUtilities.ThrowInvalidProjectFile( + elementLocation, + "InvalidProjectFile", + arg1, arg2); +``` + +**Never** construct error messages by string concatenation — always use resource strings for localization support. + +## Localization Requirements + +- Error **codes** (`MSBxxxx`) are never localized +- Error **text** is localized into all supported languages +- After adding/modifying resource strings, run a full build to generate `.xlf` placeholder translations +- Use `` elements to help translators understand context +- See [Localization.md](../../../documentation/wiki/Localization.md) for the full localization process + +## Path Formatting in Error Messages + +- Show paths relative to the project directory when possible +- Use the path exactly as the user specified it (don't normalize slashes) +- Include file and line number via `IElementLocation` when available — this enables IDE click-to-navigate +- For paths that could contain spaces, ensure they're quoted in the message + +## Checklist for New Errors/Warnings + +- [ ] Error code assigned from correct `Strings.resx` range +- [ ] "Next message code" comment updated in the resx +- [ ] Message answers: what happened, why, what to do +- [ ] `{StrBegin}` comment and placeholder documentation added +- [ ] Severity chosen (error vs warning vs message) with WarnAsError considered +- [ ] If warning: ChangeWave gating evaluated +- [ ] Full build run to generate `.xlf` files +- [ ] Test verifies the error is produced with correct code and text diff --git a/.github/skills/msbuild-performance/SKILL.md b/.github/skills/msbuild-performance/SKILL.md new file mode 100644 index 00000000000..b2654587ffd --- /dev/null +++ b/.github/skills/msbuild-performance/SKILL.md @@ -0,0 +1,102 @@ +--- +name: optimizing-msbuild-performance +description: 'Guides performance optimization for MSBuild engine code. Consult when working on hot paths in evaluation or execution, reducing allocations, choosing collection types, handling strings efficiently, modifying Expander.cs or Evaluator.cs, using Span/stackalloc, caching values, or profiling build performance. Also applies when reviewing PRs for performance regression.' +argument-hint: 'Describe the performance-sensitive code area or optimization goal.' +--- + +# MSBuild Performance Guidelines + +MSBuild evaluates and builds thousands of projects in enterprise solutions. Performance is an architectural concern, not an afterthought. + +## Guiding Principle + +**Profile before optimizing; measure, do not guess.** Use BenchmarkDotNet, the evaluation profiler (`/profileevaluation`), or ETW traces to identify actual bottlenecks before optimizing. + +See [evaluation-profiling.md](../../../documentation/evaluation-profiling.md) and [General_perf_onepager.md](../../../documentation/specs/proposed/General_perf_onepager.md) for profiling tools. + +## Allocation Awareness on Hot Paths + +The evaluation and execution engines process millions of operations per build. Unnecessary allocations cause GC pressure that compounds across large solutions. + +### Rules + +1. **Avoid LINQ in hot paths.** `Where`, `Select`, `Any`, `First` all allocate enumerator objects and delegate closures. Use `foreach` loops instead. + + ```csharp + // BAD — allocates iterator + delegate + var match = items.FirstOrDefault(i => i.Name == name); + + // GOOD — zero allocations + foreach (var item in items) + { + if (item.Name == name) { /* found */ break; } + } + ``` + +2. **Avoid string allocations in formatting.** Use `string.Concat`, interpolation with `Span`, or `StringBuilder` reuse — not `string.Format` on hot paths. + +3. **Prefer `Span` and `stackalloc` for short-lived buffers** when parsing or slicing strings. Avoid `Substring` when you only need to compare or inspect a portion. + +4. **Cache computed values.** If a value is computed in a loop but doesn't change, hoist it out. If it's expensive and reused across calls, use a field or `Lazy`. + +## String Comparison Rules + +MSBuild property, item, and target names are **case-insensitive**. Getting this wrong causes subtle bugs and perf issues. + +| Scenario | Use | +|----------|-----| +| Property/item/target names | `MSBuildNameIgnoreCaseComparer` | +| General MSBuild identifiers | `StringComparison.OrdinalIgnoreCase` | +| Dictionary keys for MSBuild names | `MSBuildNameIgnoreCaseComparer` as comparer | +| File paths on Windows | `StringComparison.OrdinalIgnoreCase` | +| File paths on Linux | `StringComparison.Ordinal` | + +**Never** use `ToLower()`/`ToUpper()` for comparisons — they allocate a new string every time. +**Never** use `CurrentCulture` for MSBuild identifiers — build behavior must not vary by locale. + +## Collection Type Selection + +| Access Pattern | Recommended Type | +|---------------|-----------------| +| Small fixed set (< ~8 items) | Array or `ReadOnlySpan` | +| Build-once, read-many | `ImmutableArray` (not `ImmutableList`) | +| Keyed lookup, many items | `Dictionary` with appropriate comparer | +| Keyed lookup, few items (< 5) | Linear scan over array (cache-friendly, avoids dict overhead) | +| Concurrent reads, rare writes | `ImmutableDictionary` or snapshot pattern | +| Ordered iteration needed | `List` or array; avoid `HashSet` if order matters | + +**`ImmutableList` is almost never the right choice** — it has O(log n) access vs O(1) for `ImmutableArray`. + +## Hot Path Identification + +These areas are performance-critical and require extra scrutiny: + +- **`Expander.cs`** — property/item/metadata expansion during evaluation +- **`Evaluator.cs`** — project evaluation orchestration +- **`ItemSpec.cs` / `LazyItemEvaluator.cs`** — item evaluation and globbing +- **`TaskExecutionHost.cs`** — task parameter marshaling +- **File I/O paths** — `FileMatcher.cs`, glob operations, project loading + +### Inlining Considerations + +For extremely hot methods, consider: +- `[MethodImpl(MethodImplOptions.AggressiveInlining)]` for small methods called millions of times +- Avoiding virtual dispatch in inner loops +- Using `struct` enumerators to avoid heap allocation + +## Evaluation Performance Specifics + +- **Import chain depth** affects evaluation time linearly. Minimize unnecessary imports. +- **Glob patterns** are evaluated per-project. Overly broad globs (`**/*`) are expensive. +- **Condition evaluation** should use short-circuit logic — put cheap checks first. +- **Property functions** (`$([System.IO.Path]::...)`) are interpreted and slower than built-in operations. + +## Anti-Patterns + +| Anti-Pattern | Why It's Bad | Fix | +|-------------|-------------|-----| +| `items.Count() > 0` | Enumerates entire collection | `items.Any()` or check `.Count` property | +| `string.Format` in log messages at Low importance | Allocates even if message is filtered | Use structured logging or guard with verbosity check | +| `new List(enumerable).ToArray()` | Double allocation | `enumerable.ToArray()` directly | +| `dict.ContainsKey(k) then dict[k]` | Double lookup | `dict.TryGetValue(k, out var v)` | +| Regex in a loop without `RegexOptions.Compiled` | Reinterprets pattern each time | Compile or use static `Regex` field | diff --git a/.github/skills/reviewing-msbuild-code/SKILL.md b/.github/skills/reviewing-msbuild-code/SKILL.md new file mode 100644 index 00000000000..1795ab95e70 --- /dev/null +++ b/.github/skills/reviewing-msbuild-code/SKILL.md @@ -0,0 +1,8 @@ +--- +name: reviewing-msbuild-code +description: "Reviews MSBuild code changes using a 24-dimension methodology. Activates for code review, PR review, pull request analysis, design review, architecture review, code quality assessment, or style check of MSBuild code. Covers backwards compatibility, ChangeWave discipline, performance, allocation awareness, test coverage, error message quality, logging, string comparison, API surface, target authoring, cross-platform correctness, code simplification, concurrency, naming, SDK integration, evaluation model integrity, correctness, dependency management, security, and build infrastructure." +--- + +# MSBuild Code Review + +Invoke `@expert-reviewer` for 24-dimension MSBuild code review. The agent contains the full methodology, principles, dimension rules with checklists, folder hotspot mapping, and 4-wave workflow. diff --git a/.github/skills/sdk-msbuild-integration/SKILL.md b/.github/skills/sdk-msbuild-integration/SKILL.md new file mode 100644 index 00000000000..b1814982bd6 --- /dev/null +++ b/.github/skills/sdk-msbuild-integration/SKILL.md @@ -0,0 +1,121 @@ +--- +name: integrating-sdk-and-msbuild +description: 'Guides work on the SDK-MSBuild integration boundary. Consult when authoring or modifying SDK targets, working on dotnet CLI to MSBuild invocation, handling project-reference protocol, coordinating cross-repo changes with dotnet/sdk, debugging property resolution or import ordering, working on restore/build/publish/pack target chains, or dealing with Directory.Build.props/targets interaction.' +argument-hint: 'Describe the SDK integration scenario or cross-repo coordination need.' +--- + +# SDK-MSBuild Integration Patterns + +MSBuild operates as a component within the .NET SDK. This boundary is the most complex integration point in the .NET build stack, spanning MSBuild (engine), SDK (target implementations), NuGet (restore), and Roslyn (compilation). + +## The Evaluation Boundary + +Understanding MSBuild's evaluation order is critical for SDK target authoring: + +``` +1. Environment variables +2. Global properties (from CLI: -p:Foo=Bar) +3. Project-level properties (file order, with imports): + ┌─ Sdk.props (SDK defaults) + ├─ Directory.Build.props (user overrides BEFORE project) + ├─ properties (the .csproj itself) + ├─ Directory.Build.targets (user overrides AFTER project) + └─ Sdk.targets (SDK target definitions) +4. Item definitions +5. Items (including SDK default globs) +``` + +### Key Import Order Rules + +- **SDK props import BEFORE user project** — SDK defaults can be overridden by the user +- **SDK targets import AFTER user project** — SDK targets see user-specified properties +- **`Directory.Build.props` is imported from `Microsoft.Common.props` as an early user extension point after core defaults are computed** — use it for solution-wide customization +- **Property defaults set in SDK must not override user-specified values** — always use `Condition="'$(Prop)' == ''"` + +```xml + +Library + + +Library +``` + +## Restore and Build Separation + +**Restore and Build must never run in the same evaluation.** The restore phase generates `.g.props` and `.g.targets` files that must be imported during evaluation — but they don't exist until restore completes. + +- `dotnet build` implicitly runs restore then build as **separate invocations** +- `dotnet build --no-restore` skips restore, assuming it already happened +- Running both targets in one invocation (`/t:Restore;Build`) is a known anti-pattern that causes intermittent failures + +## Project Reference Protocol + +The project-reference protocol spans MSBuild, SDK, and NuGet. It is the most complex integration boundary. + +### How It Works + +1. Outer build dispatches to `_GetProjectReferenceTargetFrameworkProperties` to determine inner build parameters +2. Inner build runs with the resolved `TargetFramework` (singular) for each referenced project +3. `GetTargetPath` returns the output assembly for the referencing project to consume + +### Rules + +- Protocol changes must be coordinated across MSBuild, SDK, and NuGet teams +- Multi-targeting projects (``) dispatch multiple inner builds +- The outer build must not assume a single target framework +- `SetTargetFramework` is how the outer build communicates the chosen framework to inner builds + +## Target Authoring in SDK Context + +### Extension Points + +SDK provides well-known extension points for targets: + +| Extension Point | Use For | +|----------------|---------| +| `$(BuildDependsOn)` | Adding to the Build chain | +| `$(CompileDependsOn)` | Pre-compilation steps | +| `$(PublishDependsOn)` | Publish pipeline additions | +| `$(PackDependsOn)` | NuGet pack pipeline additions | +| `BeforeTargets="Build"` | Use sparingly; prefer `DependsOnTargets` | + +### Ordering Rules + +1. **Use `DependsOnTargets` for required predecessors** — it's explicit and predictable +2. **`BeforeTargets`/`AfterTargets` should be used sparingly** — they create implicit ordering that's hard to debug +3. **Incremental build targets need precise `Inputs` and `Outputs`** — incorrect declarations cause either rebuild-every-time or stale-output bugs +4. **Test with multi-targeting** — target chains execute once per `TargetFramework` in the inner build + +## Cross-Repo Coordination + +Changes that touch the MSBuild-SDK boundary often require coordinated PRs: + +1. **MSBuild engine change** → may need SDK target updates +2. **SDK target change** → may need MSBuild API additions +3. **NuGet restore change** → affects both MSBuild evaluation and SDK targets + +### Coordination Protocol + +- File an issue in both repos describing the cross-cutting change +- Land the MSBuild change first (lower in the stack) +- Update SDK to consume the new MSBuild via dependency flow +- Test end-to-end with the SDK's MSBuild integration tests + +## Design-Time Builds + +Visual Studio uses design-time builds with different target contracts: + +- Design-time builds call `ResolveProjectReferences` but not `Build` +- They set `$(DesignTimeBuild)=true` and `$(BuildingProject)=false` +- Targets that should not run during design-time must check these properties +- Design-time builds must be fast — avoid expensive I/O or compilation + +## Common Integration Bugs + +| Symptom | Likely Cause | +|---------|-------------| +| Property has wrong value | Import ordering — check if SDK prop overrides user setting | +| Target runs in wrong order | Missing `DependsOnTargets` declaration | +| Build works, restore fails | Evaluation-time dependency on restore-generated files | +| Works single-target, fails multi-target | Target assumes single `$(TargetFramework)` | +| CLI build works, VS build fails | Design-time build target contract violation |