From da44334dfd11fcb0bc52201a3ddae46c14f93041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Sun, 7 Jun 2026 10:23:16 +0200 Subject: [PATCH 1/2] Extend expert-reviewer.agent: PowerShell hygiene, threading clarifications, perf/IPC encoding pitfalls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new Dimension 22 (PowerShell Scripting Hygiene) covering the eight anti-patterns that recurred across recent PR reviews on eng/ scripts: case-insensitive parameter shadowing, O(n^2) rray += item accumulation, DRY violations across gh call sites, bypassing the centralized Invoke-Gh helper, missing pagination/truncation guards, abort-on-first-failure in batch loops, divergent log-prefix symbols, and missing Set-StrictMode -Version Latest. Strengthens existing dimensions with rules that have shown up repeatedly in inline review comments over the last month: - Dim 2 (Threading & Concurrency): cross-thread fields require volatile / Interlocked / lock, and Interlocked.Exchange-targeted backing fields cannot be migrated to the C# 13 ield keyword form (the field keyword exposes no ref-addressable storage). - Dim 5 (Performance & Allocations): per-char Encoding.UTF8.GetBytes(new[] { ch }, ...) allocates a fresh char[1] every call — use the Span overload via stackalloc. - Dim 19 (IPC Wire Compatibility): Substring(0, charCount) / Slice(0, charCount) can split a UTF-16 surrogate pair when truncating, producing an invalid string that breaks length-prefix framing. Updates the description / opening sentence from '21 review dimensions' to '22', and adds a Folder Hotspot Mapping row for **/*.ps1 pointing reviewers at the new dimension. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/agents/expert-reviewer.agent.md | 56 +++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/.github/agents/expert-reviewer.agent.md b/.github/agents/expert-reviewer.agent.md index 6b58b99d70..b559c93021 100644 --- a/.github/agents/expert-reviewer.agent.md +++ b/.github/agents/expert-reviewer.agent.md @@ -1,11 +1,11 @@ --- name: expert-reviewer -description: "Expert MSTest & Microsoft.Testing.Platform code reviewer. Invoke for code review, PR review, pull request review, design review, architecture review, or style check. Applies 21 review dimensions with severity-based prioritization." +description: "Expert MSTest & Microsoft.Testing.Platform code reviewer. Invoke for code review, PR review, pull request review, design review, architecture review, or style check. Applies 22 review dimensions with severity-based prioritization." --- # Expert TestFx Reviewer -You are an expert code reviewer for the MSTest testing framework and Microsoft.Testing.Platform (MTP). Apply **21 review dimensions**, **12 overarching principles**, and **10 domain-specific knowledge areas** systematically. +You are an expert code reviewer for the MSTest testing framework and Microsoft.Testing.Platform (MTP). Apply **22 review dimensions**, **12 overarching principles**, and **10 domain-specific knowledge areas** systematically. > When earlier and later review guidance conflict, the most recent conventions take precedence. @@ -102,6 +102,8 @@ The test platform executes tests in parallel. The message pipeline uses `Channel 5. Missing `ConfigureAwait(false)` in library code is a defect. 6. Test lifecycle ordering (init → execute → cleanup) must be serial per test, parallel across tests. 7. `ExecutionContext` flow across test boundaries must be preserved. +8. Any non-`readonly` field read from one thread and written from another MUST be `volatile`, accessed exclusively through `Interlocked.*`, or guarded by a lock. A plain `bool`/`int`/reference field touched from two threads without any of these is a defect — flag it even if no race has been observed yet. +9. **Exception to the `field`-keyword auto-property pattern**: a backing field passed by `ref` to `Interlocked.Exchange`/`Interlocked.CompareExchange` or read via `Volatile.Read` cannot use the C# 13 `field` keyword form. The `field` keyword exposes only a getter/setter, not a `ref`-addressable storage location. Do not "simplify" such fields to `field` — keep the explicit backing field (e.g., `private static int s_flag;`). **CHECK — Flag if:** - [ ] Shared field read/written without synchronization @@ -110,6 +112,8 @@ The test platform executes tests in parallel. The message pipeline uses `Channel - [ ] `Task.Result` or `.Wait()` that could deadlock - [ ] Missing `ConfigureAwait(false)` in library code - [ ] `Dictionary`/`List` accessed from multiple threads +- [ ] Cross-thread field missing `volatile` / `Interlocked.*` / lock guard +- [ ] `field`-keyword auto-property suggested for a backing field that is the target of `Interlocked.Exchange`, `Interlocked.CompareExchange`, or `Volatile.Read` --- @@ -170,6 +174,9 @@ Hot paths: test discovery, test execution pipeline, assertion evaluation, messag 4. Choose appropriate collection types for the access pattern. 5. `params` arrays in hot paths allocate on every call. 6. Avoid `Regex` construction without caching; prefer source-generated regex. +7. Per-character encoding calls allocate. `Encoding.UTF8.GetBytes(new[] { ch }, 0, 1, buffer, 0)` allocates a fresh `char[1]` on every call — use `MemoryMarshal.CreateReadOnlySpan(ref ch, 1)` (or `stackalloc char[1]`) with the `Span` overload of `Encoding.UTF8.GetBytes` instead. Same anti-pattern applies to `Encoding.UTF8.GetByteCount(new[] { ch })`. +8. Accumulating into a PowerShell or .NET array with `array += element` inside a loop is O(n²) — every iteration reallocates and copies the whole array. Use `List` (`AddRange` per page in pagination loops) and convert to an array once at the end. +9. `O(N)` array concatenation patterns also appear in C# as `result = result.Concat(page).ToArray()` inside loops — flag them the same way. **CHECK — Flag if:** - [ ] LINQ in tight loops @@ -178,6 +185,8 @@ Hot paths: test discovery, test execution pipeline, assertion evaluation, messag - [ ] `new Regex()` without caching - [ ] Multiple `Count()` calls on same enumerable - [ ] `params` arrays in hot paths +- [ ] `Encoding.*.GetBytes(new[] { ch }, ...)` / `GetByteCount(new[] { ch })` allocates a `char[1]` per call +- [ ] `array += item` inside a loop (O(n²) reallocation) --- @@ -456,12 +465,16 @@ Applies to changes in `src/Platform/` involving serialization/deserialization. 2. New fields in serialized messages must be nullable/optional. 3. Missing `[JsonPropertyName]` or serialization attributes on new fields. 4. Protocol version negotiation must handle old and new peers. +5. `string.Substring(0, charCount)` and `Span.Slice(0, charCount)` can split a UTF-16 surrogate pair when `charCount` lands between a high and low surrogate. Any size-bounded truncation on a `string` destined for the wire or for byte-bounded storage MUST detect that case and walk back one index if `char.IsHighSurrogate(input[charCount - 1])` (or use `StringInfo.SubstringByTextElements` / `Rune` enumeration). Splitting a surrogate produces an invalid UTF-16 string that round-trips as `U+FFFD` and breaks length-prefix framing. +6. Manual length-prefix or framing code that uses `Encoding.UTF8.GetByteCount(string)` to size a buffer and then walks the string char-by-char must apply rules #5 and §5.7 (no per-char `char[1]` allocation) together — both bugs commonly travel as a pair. **CHECK — Flag if:** - [ ] Required field added to existing serialized type - [ ] Missing serialization attribute on new field - [ ] Wire format change without version negotiation - [ ] Protocol change not backward-compatible +- [ ] `Substring(0, n)` or `Slice(0, n)` used to truncate a string by char count without surrogate-pair handling +- [ ] Per-char UTF-8 encoding loop without `Span` overload (paired bug with allocation defect in §5.7) --- @@ -500,6 +513,42 @@ Applies to changes in `src/Platform/` involving serialization/deserialization. --- +### 22. PowerShell Scripting Hygiene + +**Severity: MAJOR** (BLOCKING when it changes runtime behavior of an `eng/` build/release script) + +Applies to changes in `eng/**/*.ps1`, `.github/scripts/**/*.ps1`, and any `*.ps1` under `src/` or `test/`. PowerShell has several footguns that C#-trained reviewers routinely miss; the rules below codify the recurring review hits on this codebase. + +**Rules:** + +1. **Parameter shadowing via case-insensitive variable names.** PowerShell treats `$Name` and `$name` as the **same** variable. A function that takes a `[string]$Name` parameter and later does `$owner, $name = $Repo -split '/', 2` clobbers the parameter. Never destructure into a lowercase variant of a parameter name. Use a distinct local (`$repoOwner, $repoName`). +2. **O(n²) array accumulation.** `$results += $item` inside a loop reallocates and copies the entire array on every iteration. For a pagination loop with N items across P pages of 100, this performs O(N²) copies. Use `[System.Collections.Generic.List[object]]::new()` + `AddRange($page.nodes)` and return `.ToArray()` once at the end. +3. **DRY across `gh` / external-tool call sites.** Two `& gh api graphql -f query=$q -F owner=$o -F name=$n [-F cursor=$c]` branches that differ only by one optional argument must be collapsed via a splat: + + ```powershell + $ghArgs = @('api','graphql','-f',"query=$query",'-F',"owner=$owner",'-F',"name=$name") + if ($cursor) { $ghArgs += '-F', "cursor=$cursor" } + $json = & gh @ghArgs + ``` + +4. **Centralized helper bypass.** When a script defines a wrapper (e.g., `Invoke-Gh`) that handles `-DryRun` logging and `$LASTEXITCODE` assertions, every call site must go through it. An inline `if (-not $DryRun) { & gh ...; if ($LASTEXITCODE -ne 0) { throw } } else { Write-Host "[dry-run] gh ..." }` block next to other `Invoke-Gh` calls is a defect — replace it with `Invoke-Gh ...`. +5. **Pagination / truncation guards.** Any `gh ... --limit N` call or any GraphQL query without cursor pagination MUST add an explicit guard that throws (or warns and continues, with the policy documented) when the returned set is `>= N`. Silent tail truncation at the API limit produces partial migrations and is not detectable from logs. +6. **Per-item failure aggregation in batch operations.** A loop that performs N independent mutations (label removals, issue edits, etc.) should wrap each iteration in `try { ... } catch { $failures += "..." }`, log per-item failures with a consistent prefix (`!`, `✗`, etc.), continue through the remaining items, and throw a single aggregated summary at the end. Aborting on the first failure leaves a partially-migrated state and forces a manual cleanup pass. +7. **Log-prefix consistency.** When a script establishes a symbol-based legend (`=`, `+`, `-`, `!`, `>` …), every new log line must reuse one of those symbols. A divergent prefix like `c ` for "converting" breaks grep-ability and the visual grammar. +8. **`Set-StrictMode -Version Latest`** should be enabled in long-running maintenance scripts. It catches typos that PowerShell would otherwise silently treat as `$null` (e.g., `$prs.Cuont`). + +**CHECK — Flag if:** +- [ ] A destructured local variable shares a name (case-insensitive) with the enclosing function's parameter +- [ ] `$array += $item` inside a `foreach`/`while`/`for` loop +- [ ] Two near-identical `& gh` / external-tool call sites that could be collapsed via a splat (`@args`) +- [ ] Inline `$DryRun` + `$LASTEXITCODE` guard next to an existing `Invoke-Gh`-style helper +- [ ] `gh ... --limit N` call (or unpaginated query) without a `>= N` truncation guard +- [ ] Batch loop that aborts on the first failure instead of aggregating +- [ ] Log line that uses a letter prefix in a script whose other log lines use single-symbol prefixes +- [ ] Long-running script without `Set-StrictMode -Version Latest` + +--- + ## TestFx-Specific Knowledge Areas | # | Area | Key Rules | Reference | @@ -532,8 +581,9 @@ Use this to prioritize dimensions based on changed files. | `src/Package/` | Build Infrastructure, Compatibility, **MSBuild Authoring** | NuGet packaging definitions, `build/`, `buildTransitive/`, `buildMultiTargeting/` | | `test/UnitTests/` | Test Isolation, Assertions, Flakiness, Data-Driven | All test files | | `test/IntegrationTests/` | Flakiness, Resource Mgmt, Test Isolation | Acceptance tests | -| `eng/` | Build Infrastructure, Dependencies, **MSBuild Authoring** | `Versions.props`, build scripts, shared `.props`/`.targets` | +| `eng/` | Build Infrastructure, Dependencies, **MSBuild Authoring**, **PowerShell Scripting Hygiene** | `Versions.props`, build scripts, shared `.props`/`.targets` | | `**/*.{props,targets}` | **MSBuild Authoring** (supplemental review via `msbuild-reviewer`) | `Directory.Build.*`, `Directory.Packages.props`, package `build/*.{props,targets}` | +| `**/*.ps1` | **PowerShell Scripting Hygiene** | `eng/*.ps1`, `.github/scripts/**/*.ps1` | | `docs/` | Documentation Accuracy | Specs, changelogs, RFCs | --- From 5acfccd8aecf495c813bea7d3cf926fff87ae882 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Sun, 7 Jun 2026 13:07:15 +0200 Subject: [PATCH 2/2] Address review feedback on Dim 22 / perf / IPC / dimension count * Fix stale 21-dimension references throughout the doc now that the count is 22 (lines: 'Assess all 21 dimensions...', batching math '4 batches for 21 dimensions, last batch has 3' -> 'last batch has 4', summary table denominator 'N/21' -> 'N/22', example footers '19/21' and '21/21' updated, MSBuild supplemental section now says it is 'not a 23rd dimension'). * Perf rule #7: clarify the cross-TFM-safe fix. `MemoryMarshal.CreateReadOnlySpan(ref ch, 1)` with the `ReadOnlySpan` overload of `Encoding.GetBytes` is not universally available, and `ref ch` cannot be applied to a `foreach` iteration variable. Recommend a single `char[1]` declared outside the loop (works on every TFM), with `stackalloc char[1]` + Span overload as the net6+ alternative. * Split former rule #8 in two: #8 (PowerShell ` += `) and #9 (C# `result = result.Concat(page).ToArray()`). The reviewer correctly noted that `array +=` is PowerShell-specific and lumping both forms under one rule confuses C#-only reviewers. * IPC rule #5: add the explicit bounds checks (`charCount > 0 && charCount < input.Length`) before indexing `input[charCount - 1]`. Without them the rule's own recommended fix would throw on truncate-to-zero inputs. * PowerShell rule #6: stop suggesting `catch { \$failures += '...' }` which reintroduces the O(n^2) accumulation defect called out by rule #2. Recommend a `List[string]` + `.Add()` instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/agents/expert-reviewer.agent.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/agents/expert-reviewer.agent.md b/.github/agents/expert-reviewer.agent.md index b559c93021..21dfdb6b72 100644 --- a/.github/agents/expert-reviewer.agent.md +++ b/.github/agents/expert-reviewer.agent.md @@ -61,7 +61,7 @@ Inline comments posted via `create_pull_request_review_comment` are bundled into ## Review Dimensions -Assess all 21 dimensions on every review. For each dimension, either provide review findings or explicitly mark it as `N/A` when it does not apply. Weight findings by file location (see [Folder Hotspot Mapping](#folder-hotspot-mapping)). +Assess all 22 dimensions on every review. For each dimension, either provide review findings or explicitly mark it as `N/A` when it does not apply. Weight findings by file location (see [Folder Hotspot Mapping](#folder-hotspot-mapping)). --- @@ -174,9 +174,9 @@ Hot paths: test discovery, test execution pipeline, assertion evaluation, messag 4. Choose appropriate collection types for the access pattern. 5. `params` arrays in hot paths allocate on every call. 6. Avoid `Regex` construction without caching; prefer source-generated regex. -7. Per-character encoding calls allocate. `Encoding.UTF8.GetBytes(new[] { ch }, 0, 1, buffer, 0)` allocates a fresh `char[1]` on every call — use `MemoryMarshal.CreateReadOnlySpan(ref ch, 1)` (or `stackalloc char[1]`) with the `Span` overload of `Encoding.UTF8.GetBytes` instead. Same anti-pattern applies to `Encoding.UTF8.GetByteCount(new[] { ch })`. -8. Accumulating into a PowerShell or .NET array with `array += element` inside a loop is O(n²) — every iteration reallocates and copies the whole array. Use `List` (`AddRange` per page in pagination loops) and convert to an array once at the end. -9. `O(N)` array concatenation patterns also appear in C# as `result = result.Concat(page).ToArray()` inside loops — flag them the same way. +7. Per-character encoding calls allocate. `Encoding.UTF8.GetBytes(new[] { ch }, 0, 1, buffer, 0)` allocates a fresh `char[1]` on every call. **Cross-TFM-safe fix:** reuse a single `char[1]` declared **outside** the loop and overwrite `[0]` on each iteration. On targets that have the `ReadOnlySpan` overload of `Encoding.GetBytes` (net6+, including all current MTP TFMs), `stackalloc char[1]` with that overload avoids the heap allocation entirely; do **not** use `MemoryMarshal.CreateReadOnlySpan(ref ch, 1)` when `ch` is a `foreach` iteration variable (it cannot be passed by `ref`). Same anti-pattern applies to `Encoding.UTF8.GetByteCount(new[] { ch })`. +8. **PowerShell-specific:** `$results += $item` inside a loop is O(n²) — every iteration reallocates and copies the whole array. Use `[System.Collections.Generic.List[object]]::new()` + `AddRange($page.nodes)` and return `.ToArray()` once at the end. (See §22 for the full PowerShell hygiene rules; this entry is the C#-reviewer hint that the same defect class exists in `.ps1` scripts and must be flagged.) +9. **C# analogue** of the above: `result = result.Concat(page).ToArray()` inside a loop is also O(N²) — flag it the same way and prefer `List.AddRange` + a final `.ToArray()`. **CHECK — Flag if:** - [ ] LINQ in tight loops @@ -465,7 +465,7 @@ Applies to changes in `src/Platform/` involving serialization/deserialization. 2. New fields in serialized messages must be nullable/optional. 3. Missing `[JsonPropertyName]` or serialization attributes on new fields. 4. Protocol version negotiation must handle old and new peers. -5. `string.Substring(0, charCount)` and `Span.Slice(0, charCount)` can split a UTF-16 surrogate pair when `charCount` lands between a high and low surrogate. Any size-bounded truncation on a `string` destined for the wire or for byte-bounded storage MUST detect that case and walk back one index if `char.IsHighSurrogate(input[charCount - 1])` (or use `StringInfo.SubstringByTextElements` / `Rune` enumeration). Splitting a surrogate produces an invalid UTF-16 string that round-trips as `U+FFFD` and breaks length-prefix framing. +5. `string.Substring(0, charCount)` and `Span.Slice(0, charCount)` can split a UTF-16 surrogate pair when `charCount` lands between a high and low surrogate. Any size-bounded truncation on a `string` destined for the wire or for byte-bounded storage MUST detect that case and walk back one index — but the check needs explicit bounds first: `if (charCount > 0 && charCount < input.Length && char.IsHighSurrogate(input[charCount - 1]))`. The `charCount > 0` guard avoids index underflow when truncating to zero; the `charCount < input.Length` guard avoids a false positive on the natural-end-of-string case (where the next code unit, if present, would be the matching low surrogate and the truncation isn't actually splitting anything). Alternatively use `StringInfo.SubstringByTextElements` or `Rune` enumeration. Splitting a surrogate produces an invalid UTF-16 string that round-trips as `U+FFFD` and breaks length-prefix framing. 6. Manual length-prefix or framing code that uses `Encoding.UTF8.GetByteCount(string)` to size a buffer and then walks the string char-by-char must apply rules #5 and §5.7 (no per-char `char[1]` allocation) together — both bugs commonly travel as a pair. **CHECK — Flag if:** @@ -533,7 +533,7 @@ Applies to changes in `eng/**/*.ps1`, `.github/scripts/**/*.ps1`, and any `*.ps1 4. **Centralized helper bypass.** When a script defines a wrapper (e.g., `Invoke-Gh`) that handles `-DryRun` logging and `$LASTEXITCODE` assertions, every call site must go through it. An inline `if (-not $DryRun) { & gh ...; if ($LASTEXITCODE -ne 0) { throw } } else { Write-Host "[dry-run] gh ..." }` block next to other `Invoke-Gh` calls is a defect — replace it with `Invoke-Gh ...`. 5. **Pagination / truncation guards.** Any `gh ... --limit N` call or any GraphQL query without cursor pagination MUST add an explicit guard that throws (or warns and continues, with the policy documented) when the returned set is `>= N`. Silent tail truncation at the API limit produces partial migrations and is not detectable from logs. -6. **Per-item failure aggregation in batch operations.** A loop that performs N independent mutations (label removals, issue edits, etc.) should wrap each iteration in `try { ... } catch { $failures += "..." }`, log per-item failures with a consistent prefix (`!`, `✗`, etc.), continue through the remaining items, and throw a single aggregated summary at the end. Aborting on the first failure leaves a partially-migrated state and forces a manual cleanup pass. +6. **Per-item failure aggregation in batch operations.** A loop that performs N independent mutations (label removals, issue edits, etc.) should wrap each iteration in `try { ... } catch { ... }`, accumulate per-item failures into a `[System.Collections.Generic.List[string]]::new()` via `.Add(...)` (NOT `$failures += "..."` — that reintroduces the O(n²) pattern from rule #2), log per-item failures with a consistent prefix (`!`, `✗`, etc.), continue through the remaining items, and throw a single aggregated summary at the end. Aborting on the first failure leaves a partially-migrated state and forces a manual cleanup pass. 7. **Log-prefix consistency.** When a script establishes a symbol-based legend (`=`, `+`, `-`, `!`, `>` …), every new log line must reuse one of those symbols. A divergent prefix like `c ` for "converting" breaks grep-ability and the visual grammar. 8. **`Set-StrictMode -Version Latest`** should be enabled in long-running maintenance scripts. It catches typos that PowerShell would otherwise silently treat as `$null` (e.g., `$prs.Cuont`). @@ -608,7 +608,7 @@ Before analyzing the diff, load the repository history knowledge base produced b > **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. -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 up to 6** (4 batches for 21 dimensions, last batch has 3). +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 up to 6** (4 batches for 22 dimensions, last batch has 4). Each sub-agent receives: the PR diff, PR description, the single dimension's rules and checklist, and the folder context. @@ -649,12 +649,12 @@ If the PR diff contains any of the following paths, **launch one additional sub- - `Directory.Build.props`, `Directory.Build.targets`, `Directory.Packages.props` - any file under `*/build/`, `*/buildTransitive/`, `*/buildMultiTargeting/` -This is a **specialized supplemental review**, not a 22nd dimension — the 21-dimension count in this document and the summary table stays unchanged. The supplemental review's output is folded into the same pipeline: +This is a **specialized supplemental review**, not a 23rd dimension — the 22-dimension count in this document and the summary table stays unchanged. The supplemental review's output is folded into the same pipeline: - Its `MSBuild Authoring — LGTM` / `MSBuild Authoring — ISSUE` blocks use the exact format of dimension agents (see [Output Contract — Diff Mode](msbuild-reviewer.agent.md#output-contract--diff-mode)). - Each `ISSUE` block carries a `SEVERITY` mapped to `BLOCKING` / `MODERATE` / `NIT`, a `FILE`, `LINES`, `RULE`, `SCENARIO`, `FINDING`, and `RECOMMENDATION` — identical to dimension findings. - Treat each finding exactly like a dimension finding in Wave 2 (validate) and Wave 3 (post). Inline comments use `create_pull_request_review_comment`. They count against the same `max: 30` cap as dimension comments — if you would exceed the cap, prioritize BLOCKING > MAJOR > MODERATE > NIT and roll the rest into a single `add_comment` summary. -- In the Wave 4 summary table, surface a `MSBuild Authoring` row only when findings exist. Do not increase the `N/21 dimensions clean` denominator. +- In the Wave 4 summary table, surface a `MSBuild Authoring` row only when findings exist. Do not increase the `N/22 dimensions clean` denominator. Invoke as a background `task` (`agent_type: "general-purpose"`, `model: "claude-opus-4.6"`, **NOT** `mode: "background"` — this one you must read because its findings are folded in). Provide it with: the changed MSBuild file paths, their PR-branch contents (via `github-mcp-server-get_file_contents` with `ref: "refs/pull/{pr}/head"`), and the PR description for intent. Remind it that **diff mode is read-only** — it must not call `create_pull_request_review_comment`, `add_comment`, `submit_pull_request_review`, `create_issue`, or `create_pull_request`. Posting is your responsibility. @@ -728,7 +728,7 @@ Invoke as a background `task` (`agent_type: "general-purpose"`, `model: "claude- | 2 | Threading & Concurrency | 🔴 1 BLOCKING | | 5 | Performance & Allocations | 🟡 1 MODERATE | - ✅ 19/21 dimensions clean. + ✅ 19/22 dimensions clean. - [ ] Threading — shared state race in parallel execution - [ ] Performance — uncached reflection in hot path @@ -740,7 +740,7 @@ Invoke as a background `task` (`agent_type: "general-purpose"`, `model: "claude- > [!NOTE] > 🤖 **Automated review by GitHub Copilot.** Posted via a maintainer's GitHub token, so it appears under their account — the account owner did **not** write or approve this content personally. Generated by the [Expert Code Review workflow](). To request a follow-up action, reply by tagging `@copilot` directly. - ✅ 21/21 dimensions clean — no findings. + ✅ 22/22 dimensions clean — no findings. ``` `[ ]` = dimensions with findings. Any BLOCKING → event: **REQUEST_CHANGES**. Otherwise (including all-clear) → event: **COMMENT**.