diff --git a/.github/skills/multithreaded-task-migration/SKILL.md b/.github/skills/multithreaded-task-migration/SKILL.md index 3f97e8adf8f..b098e2c49ce 100644 --- a/.github/skills/multithreaded-task-migration/SKILL.md +++ b/.github/skills/multithreaded-task-migration/SKILL.md @@ -1,22 +1,18 @@ --- name: multithreaded-task-migration -description: Guide for migrating MSBuild tasks to the multithreaded mode support. Use this when asked to convert tasks to thread-safe versions, implement IMultiThreadableTask, or add TaskEnvironment support to tasks. +description: Guide for migrating MSBuild tasks to multithreaded mode support, including compatibility red-team review. Use this when converting tasks to thread-safe versions, implementing IMultiThreadableTask, adding TaskEnvironment support, or auditing migrations for behavioral compatibility. --- # Migrating MSBuild Tasks to Multithreaded API -This skill guides you through migrating MSBuild tasks to support multithreaded execution by implementing `IMultiThreadableTask` and using `TaskEnvironment`. - -## Overview - -MSBuild's multithreaded execution model requires tasks to avoid global process state (working directory, environment variables). Thread-safe tasks declare this capability by annotating with `MSBuildMultiThreadableTask` and use `TaskEnvironment` provided by `IMultiThreadableTask` for safe alternatives. +MSBuild's multithreaded execution model requires tasks to avoid global process state (working directory, environment variables). Thread-safe tasks declare this capability via `MSBuildMultiThreadableTask` and use `TaskEnvironment` from `IMultiThreadableTask` for safe alternatives. ## Migration Steps ### Step 1: Update Task Class Declaration -a. add the attribute -b. AND implement the interface if it's necessary to use TaskEnvironment APIs. +a. Ensure the task implementing class is decorated with the `MSBuildMultiThreadableTask` attribute. +b. Implement `IMultiThreadableTask` **only if** the task needs `TaskEnvironment` APIs (path absolutization, env vars, process start). If the task has no file/environment operations (e.g., a stub class), the attribute alone is sufficient. ```csharp [MSBuildMultiThreadableTask] @@ -27,18 +23,13 @@ public class MyTask : Task, IMultiThreadableTask } ``` +**Note**: `[MSBuildMultiThreadableTask]` has `Inherited = false` — it must be on each concrete class, not just the base. + ### Step 2: Absolutize Paths Before File Operations -**Critical**: All path strings must be absolutized with `TaskEnvironment.GetAbsolutePath()` before use in file system APIs. This ensures paths resolve relative to the project directory, not the process working directory. +All path strings must be absolutized with `TaskEnvironment.GetAbsolutePath()` before use in file system APIs. This resolves paths relative to the project directory, not the process working directory. ```csharp -// BEFORE - File.Exists uses process working directory for relative paths (UNSAFE) -if (File.Exists(inputPath)) -{ - string content = File.ReadAllText(inputPath); -} - -// AFTER - Absolutize first, then use in file operations (SAFE) AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath(inputPath); if (File.Exists(absolutePath)) { @@ -46,145 +37,57 @@ if (File.Exists(absolutePath)) } ``` -`GetAbsolutePath()` throws for null/empty inputs. See [Exception Handling in Batch Operations](#exception-handling-in-batch-operations) for handling strategies. - The [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) struct: -- Has `Value` property returning the absolute path string -- Has `OriginalValue` property preserving the input path -- Is implicitly convertible to `string` for File/Directory API compatibility +- `Value` — the absolute path string +- `OriginalValue` — preserves the input path (use for error messages and `[Output]` properties) +- Implicitly convertible to `string` for File/Directory API compatibility +- `GetCanonicalForm()` — resolves `..` segments and normalizes separators (see [Sin 5](#sin-5-canonicalization-mismatch)) -**CAUTION**: `FileInfo` can be created from relative paths - only use `FileInfo.FullName` if constructed with an absolute path. - -#### Note: -If code previously used `Path.GetFullPath()` for canonicalization (resolving `..` segments, normalizing separators), call `AbsolutePath.GetCanonicalForm()` after absolutization to preserve that behavior. Do not simply replace `Path.GetFullPath` with `GetAbsolutePath` if canonicalization was the intent. You can replace `Path.GetFullPath` behavior by combining both: - -```csharp -AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath(inputPath).GetCanonicalForm(); -``` -The goal is MAXIMUM compatibility so think about these edge cases so it behaves the same as before. +**CAUTION**: `GetAbsolutePath()` throws `ArgumentException` for null/empty inputs. See [Sin 3](#sin-3-null-coalescing-that-changes-control-flow) and [Sin 6](#sin-6-exception-type-change) for compatibility implications. ### Step 3: Replace Environment Variable APIs -```csharp -// BEFORE (UNSAFE) -string value = Environment.GetEnvironmentVariable("VAR"); -Environment.SetEnvironmentVariable("VAR", "value"); - -// AFTER (SAFE) -string value = TaskEnvironment.GetEnvironmentVariable("VAR"); -TaskEnvironment.SetEnvironmentVariable("VAR", "value"); -``` +| BEFORE (UNSAFE) | AFTER (SAFE) | +|--------------------------------------------------|----------------------------------------------------| +| `Environment.GetEnvironmentVariable("VAR");` | `TaskEnvironment.GetEnvironmentVariable("VAR");` | +| `Environment.SetEnvironmentVariable("VAR", "v");` | `TaskEnvironment.SetEnvironmentVariable("VAR", "v");` | ### Step 4: Replace Process Start APIs -```csharp -// BEFORE (UNSAFE - inherits process state) -var psi = new ProcessStartInfo("tool.exe"); - -// AFTER (SAFE - uses task's isolated environment) -var psi = TaskEnvironment.GetProcessStartInfo(); -psi.FileName = "tool.exe"; -``` +| BEFORE (UNSAFE - inherits process state) | AFTER (SAFE - uses task's isolated environment) | +|----------------------------------------------------|-----------------------------------------------------| +| `var psi = new ProcessStartInfo("tool.exe");` | `var psi = TaskEnvironment.GetProcessStartInfo();` | +| | `psi.FileName = "tool.exe";` | ## Updating Unit Tests -**Every test creating a task instance must set TaskEnvironment.** Use `TaskEnvironmentHelper.CreateForTest()`: - -```csharp -// BEFORE -var task = new Copy -{ - BuildEngine = new MockEngine(true), - SourceFiles = sourceFiles, - DestinationFolder = new TaskItem(destFolder), -}; - -// AFTER -var task = new Copy -{ - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), - BuildEngine = new MockEngine(true), - SourceFiles = sourceFiles, - DestinationFolder = new TaskItem(destFolder), -}; -``` - -### Testing Exception Cases - -Tasks must handle null/empty path inputs properly. - -```csharp -[Fact] -public void Task_WithNullPath_Throws() -{ - var task = CreateTask(); - - Should.Throw(() => task.ProcessPath(null!)); -} -``` +Every test creating a task instance must set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()`. ## APIs to Avoid -### Critical Errors (No Alternative) -- `Environment.Exit()`, `Environment.FailFast()` - Return false or throw instead -- `Process.GetCurrentProcess().Kill()` - Never terminate process -- `ThreadPool.SetMinThreads/MaxThreads` - Process-wide settings -- `CultureInfo.DefaultThreadCurrentCulture` (setter) - Affects all threads -- `Console.*` - Interferes with logging - -### Requires TaskEnvironment -- `Environment.CurrentDirectory` → `TaskEnvironment.ProjectDirectory` -- `Environment.GetEnvironmentVariable` → `TaskEnvironment.GetEnvironmentVariable` -- `Environment.SetEnvironmentVariable` → `TaskEnvironment.SetEnvironmentVariable` -- `Path.GetFullPath` → `TaskEnvironment.GetAbsolutePath` -- `Process.Start`, `ProcessStartInfo` → `TaskEnvironment.GetProcessStartInfo` - -### File APIs Need Absolute Paths -- `File.*`, `Directory.*`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader`, `StreamWriter` -- All path parameters must be absolute - -### Potential Issues (Review Required) -- `Assembly.Load*`, `LoadFrom`, `LoadFile` - Version conflicts -- `Activator.CreateInstance*` - Version conflicts +| Category | APIs | Alternative | +|---|---|---| +| **Forbidden** | `Environment.Exit`, `FailFast`, `Process.Kill`, `ThreadPool.SetMin/MaxThreads`, `Console.*` | Return false, throw, or use `Log` | +| **Use TaskEnvironment** | `Environment.CurrentDirectory`, `Get/SetEnvironmentVariable`, `Path.GetFullPath`, `ProcessStartInfo` | See Steps 2-4 | +| **Need absolute paths** | `File.*`, `Directory.*`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader/Writer` | Absolutize first (File System APIs) | +| **Review required** | `Assembly.Load*`, `Activator.CreateInstance*` | Check for version conflicts | ## Practical Notes ### CRITICAL: Trace All Path String Usage -**You MUST trace every path string variable through the entire codebase** to find all places where it flows into file system operations - including helper methods, utility classes, and third-party code that may internally use File APIs. +Trace every path string through all method calls and assignments to find all places it flows into file system operations — including helper methods that may internally use File System APIs. -Steps: 1. Find every path string (e.g., `item.ItemSpec`, function parameters) -2. **Trace downstream**: Follow the variable through all method calls and assignments +2. Trace downstream through all method calls 3. Absolutize BEFORE any code path that touches the file system -4. Use `OriginalValue` for user-facing output (logs, errors) - -```csharp -// WRONG - LockCheck internally uses File APIs with non-absolutized path -string sourceSpec = item.ItemSpec; // sourceSpec is string -string lockedMsg = LockCheck.GetLockedFileMessage(sourceSpec); // BUG! Trace the call! - -// CORRECT - absolutized path passed to helper -AbsolutePath sourceFile = TaskEnvironment.GetAbsolutePath(item.ItemSpec); -string lockedMsg = LockCheck.GetLockedFileMessage(sourceFile); - -// For error messages, preserve original user input -Log.LogError("...", sourceFile.OriginalValue, ...); -``` +4. Use `OriginalValue` for user-facing output (logs, errors) — see [Sin 2](#sin-2-error-message-path-inflation) ### Exception Handling in Batch Operations -**Important**: `GetAbsolutePath()` throws on null/empty inputs. In batch processing scenarios (e.g., iterating over multiple files), an unhandled exception will abort the entire batch. Tasks must catch and handle these exceptions appropriately to avoid cutting short processing of valid items: +In batch processing (iterating over files), `GetAbsolutePath()` throwing on one bad path aborts the entire batch. Match the original task's error semantics: ```csharp -// WRONG - one bad path aborts entire batch -foreach (ITaskItem item in SourceFiles) -{ - AbsolutePath path = TaskEnvironment.GetAbsolutePath(item.ItemSpec); // throws, batch stops! - ProcessFile(path); -} - -// CORRECT - handle exceptions, continue processing valid items bool success = true; foreach (ITaskItem item in SourceFiles) { @@ -195,50 +98,194 @@ foreach (ITaskItem item in SourceFiles) } catch (ArgumentException ex) { - Log.LogError($"Invalid path '{item.ItemSpec}': {ex.Message}"); + Log.LogError("Invalid path '{0}': {1}", item.ItemSpec, ex.Message); success = false; - // Continue processing remaining items } } return success; ``` -Consider the task's error semantics: should one invalid path fail the entire task immediately, or should all items be processed with errors collected? Match the original task's behavior. - ### Prefer AbsolutePath Over String -When working with paths, stay in the `AbsolutePath` world as much as possible rather than converting back and forth to `string`. This reduces unnecessary conversions and maintains type safety: +Stay in the `AbsolutePath` world — it's implicitly convertible to `string` where needed. Avoid round-tripping through `string` and back. + +### TaskEnvironment is Not Thread-Safe + +If your task spawns multiple threads internally, synchronize access to `TaskEnvironment`. Each task *instance* gets its own environment, so no synchronization between tasks is needed. + +## References + +- [Thread-Safe Tasks Spec](https://github.com/dotnet/msbuild/blob/main/documentation/specs/multithreading/thread-safe-tasks.md) +- [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) +- [`TaskEnvironment`](https://github.com/dotnet/msbuild/blob/main/src/Framework/TaskEnvironment.cs) +- [`IMultiThreadableTask`](https://github.com/dotnet/msbuild/blob/main/src/Framework/IMultiThreadableTask.cs) + +--- + +# Compatibility Red-Team Playbook + +After migration, review for behavioral compatibility. **Every observable difference is a bug until proven otherwise.** + +Observable behavior = `Execute()` return value, `[Output]` property values, error/warning message content, exception types, files written, and which code path runs. + +## The 6 Deadly Compatibility Sins + +Real bugs found during MSBuild task migrations. Every one shipped in initial "passing" code with green tests. + +### Sin 1: Output Property Contamination + +Absolutized values leak into `[Output]` properties that users/other tasks consume. ```csharp -// AVOID - unnecessary conversions -string path = TaskEnvironment.GetAbsolutePath(input).Value; -AbsolutePath again = TaskEnvironment.GetAbsolutePath(path); // redundant! - -// PREFER - stay in AbsolutePath -AbsolutePath path = TaskEnvironment.GetAbsolutePath(input); -// Use path directly - it's implicitly convertible to string where needed -File.ReadAllText(path); +// BROKEN: ManifestPath was "bin\Release\app.manifest", now "C:\repo\bin\Release\app.manifest" +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(Path.Combine(OutputDirectory, name)); +ManifestPath = abs; // implicit string conversion! + +// CORRECT: separate original form from absolutized path +string originalPath = Path.Combine(OutputDirectory, name); +AbsolutePath outputPath = TaskEnvironment.GetAbsolutePath(originalPath); +ManifestPath = originalPath; // [Output]: original form +document.Save((string)outputPath); // file I/O: absolute path ``` -### TaskEnvironment is Not Thread-Safe +**Detect**: For every `[Output]` property, trace backward — is it ever assigned from an `AbsolutePath`? -If your task spawns multiple threads internally, you must synchronize access to `TaskEnvironment`. However, each task instance gets its own environment, so no synchronization with other tasks is needed. +### Sin 2: Error Message Path Inflation -## Checklist +Error messages show absolutized paths instead of the user's original input. -- [ ] Task is annotated with `MSBuildMultiThreadableTask` attribute and implements `IMultiThreadableTask` if TaskEnvironment APIs are required -- [ ] All environment variable access uses `TaskEnvironment` APIs -- [ ] All process spawning uses `TaskEnvironment.GetProcessStartInfo()` -- [ ] All file system APIs receive absolute paths -- [ ] All helper methods receiving path strings are traced to verify they don't internally use File APIs with non-absolutized paths -- [ ] No use of `Environment.CurrentDirectory` -- [ ] All tests set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` -- [ ] Tests verify exception behavior for null/empty paths -- [ ] No use of forbidden APIs (Environment.Exit, etc.) +```csharp +// BROKEN: "Cannot find 'C:\repo\app.manifest'" instead of "Cannot find 'app.manifest'" +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(path); +Log.LogError("Cannot find '{0}'", abs); // implicit conversion! -## References +// CORRECT: use OriginalValue +Log.LogError("Cannot find '{0}'", abs.OriginalValue); +``` + +**Detect**: Search every `Log.LogError`/`LogWarning`/`LogMessage` — is any argument an `AbsolutePath`? + +### Sin 3: Null Coalescing That Changes Control Flow + +Adding `?? ""` silently swallows an exception the old code relied on for error handling. + +```csharp +// BEFORE: Path.GetDirectoryName("C:\") → null → Path.Combine(null, x) → ArgumentNullException +// → task fails with an exception / error logged → Execute() returns false -- [Thread-Safe Tasks Spec](https://github.com/dotnet/msbuild/blob/main/documentation/specs/multithreading/thread-safe-tasks.md) - Full specification for multithreaded task support -- [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) - Struct for representing absolute paths -- [`TaskEnvironment`](https://github.com/dotnet/msbuild/blob/main/src/Framework/TaskEnvironment.cs) - Thread-safe environment APIs for tasks -- [`IMultiThreadableTask`](https://github.com/dotnet/msbuild/blob/main/src/Framework/IMultiThreadableTask.cs) - Interface for multithreaded task support +// BROKEN: ?? "" added "for safety" +string dir = Path.GetDirectoryName(fileName) ?? string.Empty; +// Path.Combine("", x) succeeds silently → no error → Execute() returns TRUE! +``` + +**Detect**: For every `??` you added, ask: "What happened when this was null before?" If it threw and was caught → your `??` is a bug. + +### Sin 4: Try-Catch Scope Mismatch + +`GetAbsolutePath()` inside a try block leaves the absolutized value out of scope in the catch block. Helper methods in the catch (like `LockCheck`) then use the original non-absolute path. + +```csharp +// CORRECT: hoist above try so catch can use it too +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); +try { + WriteFile(abs); +} catch (Exception ex) { + string lockMsg = LockCheck.GetLockedFileMessage(abs); // absolute → correct file + Log.LogError("Failed: {0}", OutputManifest.ItemSpec, ...); // original → user-friendly +} +``` + +**Detect**: For every `GetAbsolutePath` inside a try, check if the catch block needs the absolutized value. + +### Sin 5: Canonicalization Mismatch + +`GetAbsolutePath` does NOT canonicalize. `Path.GetFullPath` does TWO things: absolutize AND canonicalize (`..` resolution, separator normalization). If the old code used `Path.GetFullPath` for dictionary keys, comparisons, or display, you must add `.GetCanonicalForm()`: + +```csharp +// GetAbsolutePath("foo/../bar") → "C:\repo\foo/../bar" (NOT canonical) +// Path.GetFullPath("foo/../bar") → "C:\repo\bar" (canonical) + +// BROKEN for dictionary keys — "C:\repo\foo\..\bar" ≠ "C:\repo\bar" +var map = items.ToDictionary(p => (string)TaskEnvironment.GetAbsolutePath(p.ItemSpec), ...); + +// CORRECT +var map = items.ToDictionary( + p => (string)TaskEnvironment.GetAbsolutePath(p.ItemSpec).GetCanonicalForm(), + StringComparer.OrdinalIgnoreCase); +``` + +**Detect**: Find every `Dictionary`/`HashSet`/`ToDictionary` using path keys, and every place the old code called `Path.GetFullPath`. If canonicalization mattered, add `.GetCanonicalForm()`. + +### Sin 6: Exception Type Change + +Old code threw `FileNotFoundException` for missing files; new code throws `ArgumentException` from `GetAbsolutePath("")` before reaching the file check. Custom catch blocks filtering by exception type may be bypassed. (`ExceptionHandling.IsIoRelatedException` catches `ArgumentException`, but task-specific handlers might not.) + +**Detect**: For every `GetAbsolutePath`, check what the old code threw for null/empty and whether the calling code has type-specific catch blocks. + +## Red-Team Audit Protocol + +### Phase 1: Trace Every Changed Line + +For each modified line: What was the exact runtime value before? After? Where does it flow (outputs, logs, file paths, dictionary keys)? Does each destination produce identical observable behavior? + +### Phase 2: Null/Empty/Edge Input Analysis + +| Input | `GetAbsolutePath` | Old behavior | Match? | +|---|---|---|---| +| `null` | `ArgumentException` | Varies | ❓ | +| `""` | `ArgumentException` | Varies | ❓ | +| `"C:\"` (root) | Valid | Valid | ✅ usually | +| `"."` | `"C:\repo\."` (not canonical) | `"C:\repo"` if GetFullPath | ❌ maybe | +| `"foo\..\bar"` | `"C:\repo\foo\..\bar"` | `"C:\repo\bar"` if GetFullPath | ❌ maybe | +| Already absolute | Pass-through | Pass-through | ✅ | + +`Path.GetDirectoryName` → `Path.Combine` chains: +| Input | `GetDirectoryName` returns | `Path.Combine(result, x)` | +|---|---|---| +| `"C:\"` | `null` | Throws `ArgumentNullException` | +| `""` | `""` (.NET Fx) / `null` (.NET Core+) | Works / Throws ⚠️ | +| `"file.resx"` (no dir) | `""` | Works | + +Verify behavior on **both** `net472` and `net10.0`. + +### Phase 3: Downstream Impact + +1. **Output properties**: What consumes this task's `[Output]`? Does it compare, display, or use as a path? +2. **Written files**: Does file content change? (e.g., XML with embedded paths) +3. **Helper methods**: Do `LockCheck`, `ManifestWriter`, etc. internally resolve relative paths? +4. **Error codes**: MSBuild error codes (MSBxxxx) must be identical. + +### Phase 4: Concurrency + +1. Two tasks with different `ProjectDirectory` values don't interfere +2. No writes to static fields (shared across threads) +3. All file operations use absolutized paths + +## Compatibility Test Matrix + +``` +[Task] × [Input Type] × [Assertion] + +Inputs: relative path, absolute path, null, empty, ".." segments, root "C:\", + forward slashes, trailing separator, UNC path, 260+ char path + +Assertions: Execute() return value, [Output] exact string, error message content, + exception type, file location, file content +``` + +## Sign-Off Checklist + +- [ ] `[MSBuildMultiThreadableTask]` on every concrete class (not just base — `Inherited=false`) +- [ ] `IMultiThreadableTask` only on classes that use `TaskEnvironment` APIs +- [ ] Every `[Output]` property: exact string value matches pre-migration +- [ ] Every `Log.LogError`/`LogWarning`: path in message matches pre-migration (use `OriginalValue`) +- [ ] Every `GetAbsolutePath` call: null/empty exception behavior matches old code path +- [ ] Every dictionary/set with path keys: canonicalization preserved (`GetCanonicalForm()`) +- [ ] Every try-catch: absolutized value available in catch block where needed +- [ ] Every `??` or `?.` added: verified it doesn't swallow a previously-thrown exception +- [ ] No `AbsolutePath` leaks into user-visible strings unintentionally +- [ ] Helper methods traced for internal File API usage with non-absolutized paths +- [ ] All tests set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` +- [ ] Cross-framework: tested on both net472 and net10.0 +- [ ] Concurrent execution: two tasks with different project directories produce correct results +- [ ] No forbidden APIs (`Environment.Exit`, `Console.*`, etc.)