diff --git a/.github/agents/expert-reviewer.md b/.github/agents/expert-reviewer.md index f0fbd3bf2fc..7496846848f 100644 --- a/.github/agents/expert-reviewer.md +++ b/.github/agents/expert-reviewer.md @@ -106,6 +106,7 @@ Hot paths: `Evaluator.cs`, `Expander.cs`, file I/O operations. 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. +5. Verify test assertions actually validate the claimed behavior — weak assertions (e.g., "not empty", "no crash") can pass even with incorrect output. **CHECK — Flag if:** - [ ] New behavior has no test coverage @@ -113,6 +114,7 @@ Hot paths: `Evaluator.cs`, `Expander.cs`, file I/O operations. - [ ] Test method names are opaque (e.g., `Test1`, `TestBug`) - [ ] Tests depend on implicit environment state - [ ] Only the happy path is tested +- [ ] Test assertions are too weak to catch incorrect behavior (would pass with wrong output) --- @@ -227,12 +229,18 @@ See `../../documentation/wiki/Microsoft.Build.Framework.md`. 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. +5. For bug fix PRs, read the original issue and feature PR discussions to understand the design intent. Verify the fix aligns with it. +6. When code works around an API limitation (try/catch chains, TOCTOU patterns, fallback sequences), check whether a better API exists in already-referenced packages that would eliminate the workaround. +7. When a pattern is borrowed from another codebase or context, verify its assumptions still hold in the new context. **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 +- [ ] Fix contradicts design intent established in original feature discussions +- [ ] Workaround for an API limitation when a better API is available in existing dependencies +- [ ] Pattern borrowed from a different context without validating its assumptions apply here --- @@ -460,6 +468,8 @@ See `../../documentation/High-level-overview.md` and `../../documentation/Built- 3. Validate fixes against original repro steps. 4. Validate inputs early — fail fast with clear errors. 5. Imagine exotic scenarios that could break assumptions. Like a red teamer trying to break MSBuild with weird project files or build environments. +6. When a fix handles N=2 participants (e.g., two concurrent writers), verify it also works for N=3+. Fixes that close one race window often leave a wider one. +7. Verify the fix addresses the root cause, not just a symptom. Patching over a structural issue (e.g., adding retries around a TOCTOU) may need revisiting if a cleaner solution exists. **CHECK — Flag if:** - [ ] New code path doesn't handle null/empty inputs @@ -467,6 +477,8 @@ See `../../documentation/High-level-overview.md` and `../../documentation/Built- - [ ] Boundary conditions not considered (off-by-one, max-length, empty) - [ ] Input validation missing at public API boundaries - [ ] Code relies on assumptions that exotic inputs could break +- [ ] Fix only handles the 2-participant case but fails with more concurrent actors +- [ ] Fix patches a symptom when the root cause could be addressed --- @@ -550,6 +562,8 @@ Use this to prioritize dimensions based on changed files. 1. Map changed files to the [Folder Hotspot Mapping](#folder-hotspot-mapping). +1b. **Historical context** (for bug fix and follow-up PRs): Read the linked issue and the original feature PR discussions. Identify design intent, constraints, and reviewer-established principles. Feed this context to every dimension agent so they can evaluate whether the fix aligns with the original design, not just whether the code compiles. + 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. diff --git a/.github/instructions/tasks.instructions.md b/.github/instructions/tasks.instructions.md index 08ebfb794c3..94de668819d 100644 --- a/.github/instructions/tasks.instructions.md +++ b/.github/instructions/tasks.instructions.md @@ -30,6 +30,7 @@ Built-in tasks ship with MSBuild and cannot be independently versioned. * Use `FileUtilities` helpers — do not roll custom path manipulation. * Support UNC paths, long paths (> 260 chars), and cross-platform separators. +* The `Microsoft.IO.Redist` package is referenced and backports .NET Core file APIs to .NET Framework. Check `Microsoft.IO.File`, `Microsoft.IO.Path`, `Microsoft.IO.Directory` for better overloads before writing workarounds (e.g., `Microsoft.IO.File.Move(src, dst, overwrite: true)` eliminates TOCTOU patterns around `File.Move` + `File.Replace`). ## Multithreaded Task Migration