Proposal: instructions, skills and review agent

.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)

.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)

.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)

.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<char>`-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)

.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)

.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).

.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)

.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)