Skip the document-level coverage thresholds on a SearchRoot-scoped coverage runner run

docs/features/active/2026-09-28-coverage-runner-scoped-threshold-and-format-928/code-review.2026-09-29T10-00.md

@@ -0,0 +1,60 @@
+# Code Review: coverage-runner-scoped-threshold-and-format (Issue #928)
+
+- Timestamp label: 2026-09-29T10-00 (assigned without a clock read; see the policy audit header)
+- Branch: bug/coverage-runner-scoped-threshold-and-format-928 at a17ca5bcf (caller-supplied) against merge base 177b6d78e
+- Files reviewed: scripts/vscode/Invoke-MSTestWithCoverage.Scope.ps1 (new), scripts/vscode/Invoke-MSTestWithCoverage.ps1 (modified), tests/scripts/vscode/Invoke-MSTestWithCoverage.Scope.Tests.ps1 (new); read in full from the worktree. Context read: Invoke-MSTestWithCoverage.Threshold.ps1, the three sibling entry-point suites' import blocks, `.github/workflows/_pester.yml`, `.github/workflows/_mstest-coverage.yml` lines 78 to 102.
+- Method: static reading only (Bash withheld). No command executed.
+
+## Executive Summary
+
+The implementation is small, readable and matches the plan's Production and Test Specifications. The predicate is pure and correctly handles the equivalence cases the AC preamble names (omitted root, `.`, `.\`, trailing separator, letter case) and the two misclassification traps a naive prefix comparison would fall into (a sibling whose name extends the root, and the parent directory). The entry-point edit leaves the unscoped path byte-identical in statement text and order, emits one actionable warning on a scoped run, and does not add any opt-out surface. The test file avoids temporary files and TestDrive, mocks only wrapper seams, and proves the RED-first partition.
+
+One Blocking finding (CR-1) carries over from the coverage gate: the scoped-arm `Write-Warning` at entry-point line 408 is executed by a passing test but is not credited by the CI-equivalent breakpoint coverage run, because each test file executes its own parsed copy of the entry point and breakpoints bind to the first copy. This is a structural property of where the conditional lives, and the recommended fix is structural: move the gate into the path-loaded part file behind one unconditional call. One Non-blocking finding (CR-2) and eight Informational notes follow.
+
+Blocking count: 1.
+
+## Findings Table
+
+| Severity | File | Location | Finding | Recommendation | Rationale | Evidence |
+|---|---|---|---|---|---|---|
+| Blocking | scripts/vscode/Invoke-MSTestWithCoverage.ps1 | lines 404 to 414 (conditional block); line 408 | The scoped arm's `Write-Warning` is uncredited by the agreed coverage route: AC6's changed-line clause and baseline clause fail (94.46% vs 94.49%; entry point 89.15% vs 89.68%). The line is executed (It 10 asserts the warning text and passes). Cause: the conditional lives in the entry point, which every suite imports as a separate `ParseFile` copy; Pester 5.6.1 line breakpoints bind to the first copy that runs (`Invoke-MSTest.RunSettings.Tests.ps1`), which never takes the scoped arm. | Relocate the gate into the path-loaded part file: add to `Invoke-MSTestWithCoverage.Scope.ps1` a function (approved verb, singular noun, for example `Assert-CoberturaCoverageThresholdForRun -CoberturaXml -RepoRoot -ResolvedSearchRoot`) whose body is the current conditional verbatim (warning in the scoped arm, the two assertion statements in the same order in the unscoped arm); replace lines 404 to 414 with one call. Add three direct unit cases for the new function (scoped: one warning, no throw; unscoped below line floor: line message; unscoped below branch floor: branch message). First confirm, with the executor's two-file diagnostic shape (AssemblyDiscovery then the new file), that part-file lines are credited from a later-sorting file. Fallback only if that premise fails: maintainer-ratified measurement exception transcribed into issue.md beside AC6. | The rule file makes changed-line coverage regression blocking, and AC6 is explicit. The relocation follows the Coverage Exclusion Policy's own guidance (logic in testable modules, thin wiring in entry points), keeps D3's "unchanged in text and order" property for the two assertion statements, keeps every AC1 to AC5 test valid without edits, and removes the dependence on test-file sort order. Options 1 (change the instrument) and 2 (edit the 499-line first-binding sibling) are rejected; see policy-audit G-1. | evidence/qa-gates/p2-t3-test-coverage.iter1.2026-09-29T09-23.md (FILE_LINE rows, three ordered diagnostic runs); evidence/other/p2-t13-reduced-audit-handoff.2026-09-29T09-27.md (fourth run, profiler route); test file line 19 and sibling import blocks; entry-point line 313 path dot-source of the part file, which reads 5/5 |
+| Non-blocking | scripts/vscode/Invoke-MSTestWithCoverage.Scope.ps1 | lines 33 to 43 | The contract documents both inputs as absolute paths, but nothing enforces it: `[IO.Path]::GetFullPath` silently resolves a relative or empty-after-trim input against the process working directory, so a future caller passing a relative path would get an answer that depends on ambient state. | Add `[ValidateNotNullOrEmpty()]` to both parameters and a fail-fast guard `if (-not [IO.Path]::IsPathRooted($RepoRoot)) { throw ... }` (same for the search root), with one negative test each. Can be folded into the CR-1 remediation of the same file. | Fail fast and explicitly (General Code Change Policy section 3); the rule file asks tests and scripts not to rely on implicit working-directory assumptions. Low risk today because the only caller passes `Resolve-Path` output and a `Join-Path` on it. | Part file lines 41 to 43; entry point lines 323 to 324 |
+| Informational | scripts/vscode/Invoke-MSTestWithCoverage.Scope.ps1; scripts/vscode/Invoke-MSTestWithCoverage.ps1 | part file lines 3 to 6; entry point lines 311 to 312 | Two production comments justify the dot-source placement by the per-batch change-budget cap ("a fourth production file ... over the three-file cap"). That is a process reason that will age: once the helpers chain is next edited the natural home is a sixth dot-source line in Helpers.ps1. | When Helpers.ps1 next changes, move the dot-source there and delete both comments. If CR-1 is remediated, rewrite the part-file header comment to describe the gate function rather than the budget. | Comments should state design reasons that stay true; a budget constraint is true only for this batch. | Read of both files |
+| Informational | scripts/vscode/Invoke-MSTestWithCoverage.ps1 | lines 1 to 13 and 284 to 291 | AC4 is satisfied by the function-level help (`.PARAMETER SearchRoot`), which is what the test reads through the AST. The script-level `param` block has no comment-based help, so `Get-Help .\Invoke-MSTestWithCoverage.ps1` does not surface the scoped-run behavior to a command-line user. The `.PARAMETER SearchRoot` text also carries a redundant clause ("; they are skipped only on a scoped run"). | Follow-up: add a script-level help block that points at the function help, and drop the redundant clause. | Documentation reaches the caller who actually runs the script. Plan D5 chose the function help deliberately, so this is not a defect against the plan. | Read of the entry point |
+| Informational | scripts/vscode/Invoke-MSTestWithCoverage.ps1 | line 466 (file length) | The entry point is at 466 lines, 34 below the 500-line ceiling. | Route the next entry-point addition to a part file; the CR-1 remediation reduces the entry point by about 8 lines. | File-size policy. | P2-T5 arithmetic (439 + 29 - 2 = 466) and Read |
+| Informational | tests/scripts/vscode/Invoke-MSTestWithCoverage.Scope.Tests.ps1 | lines 165 to 173 (It 9) | It 9 carries two assertions (`Should -Not -Throw` and `Should -Invoke Set-Content -Times 4 -Exactly`). Both describe the same behavior (the scoped run completes the full pipeline), so this is acceptable, but a failure of the second assertion would be reported under a name that says only "completes without error". | Optional: split the `Set-Content` count into its own It, or rename It 9 to mention the four writes. | One behavior per It (rule file); clear failure attribution. | Read of the test file |
+| Informational | tests/scripts/vscode/Invoke-MSTestWithCoverage.Scope.Tests.ps1; scripts/vscode/Invoke-MSTestWithCoverage.Scope.ps1 | fixture root line 30; predicate line 45 | Path semantics are Windows-only: the fixtures use a drive-letter root and the comparison is case-insensitive unconditionally. Consistent with the vswhere-bound script and every sibling suite; noted so a future cross-platform port knows the assumption. | None now. | Determinism across hosts. | Read of both files |
+| Informational | tests/scripts/vscode/Invoke-MSTestWithCoverage.Helpers.Tests.ps1 (pre-existing, not in this diff) | lines 41, 42, 104, 124 | Literal fixture paths embed the developer account name under a user-profile prefix. Outside this change's footprint. | Promote a hygiene issue: replace with the neutral drive-letter fixture root the sibling suites use. | Host-identifier hygiene (AC7's rule applies to committed evidence; the same principle applies to committed test fixtures). | Grep over tests/scripts/vscode for the account name |
+| Informational | tests/scripts/vscode (four entry-point suites, pre-existing) | import blocks | The breakpoint-binding under-crediting is not specific to this change: any entry-point line reached only by a suite that sorts after `Invoke-MSTest.RunSettings.Tests.ps1` records no hit under the CI route. | Promote a potential-feature entry: convert the four suites to `. $script:coverageScript` (the `InvocationName` guard at entry-point line 464 makes path dot-sourcing safe and lets all files share PowerShell's per-path compiled-script cache), or evaluate `CodeCoverage.UseBreakpoints = $false` in `_pester.yml` as a separate workflow change with a green run. | Measurement fidelity of the CI gate; the executor's diagnostics establish the mechanism. | P2-T3 diagnostic runs; sibling import blocks; hook and workflow reads |
+| Informational | docs/features/active/.../evidence | P2-T7 to P2-T12 labels | Timestamp labels were assigned without a clock read and renamed to 09-27 afterwards; disclosed by the executor in P2-T13. | None; labels are not used for ordering. Future runs should read the clock per artifact. | Evidence conventions. | evidence/other/p2-t13-reduced-audit-handoff.2026-09-29T09-27.md |
+
+## Detailed Notes
+
+### Predicate correctness (Invoke-MSTestWithCoverage.Scope.ps1)
+
+- `GetFullPath` collapses `.` and `..` segments, so `<repo>\.` and `<repo>\.\` normalise to `<repo>`; `TrimEnd` of both separator characters then removes a trailing separator from either side, so `<repo>\` equals `<repo>`. A drive root trims to `C:`-style text on both sides consistently, so root-versus-root still compares equal and root-versus-subdirectory compares unequal (It 8).
+- `[string]::Equals(..., OrdinalIgnoreCase)` rather than `-eq` is the right choice: the comparison is explicit and culture-independent.
+- A search root of `<subdir>\..` resolves to the repository root and is treated as unscoped, so the full gate applies; that is the correct outcome for a run that covers the whole tree.
+- The `-not` on the equality is the only branch in the file; Pester does not measure branches, and both outcomes are covered by It 1 to 8.
+
+### Entry-point edit (Invoke-MSTestWithCoverage.ps1)
+
+- The conditional sits after post-processing and before the first-party report, so a scoped run still prints `Get-CoberturaFirstPartyCoverageReport`, writes the JaCoCo projection and the trx summary, and runs `Assert-JacocoProjectionReconciliation`. That is the intended behavior: only the two document-level floor assertions are skipped.
+- The warning names both the resolved search root and the repository root, which makes an accidental scoped run recognisable in the log. `Write-Warning` is the established non-fatal channel in this script (the trx-summary branch at line 441 uses it too).
+- The CI workflow invokes `-SearchRoot .` (`_mstest-coverage.yml` line 95) and the VS Code task does the same, so both remain unscoped; `Join-Path <repo> '.'` normalises to the repository root through the predicate. The 80 and 75 literals in Threshold.ps1 (lines 52, 54, 122, 124) are unchanged.
+- The `.DESCRIPTION` paragraph restates the floors as "80 percent line and 75 percent branch", matching the script; the repository rules state 85 for line coverage. That discrepancy predates this change (policy-audit G-4).
+
+### Test design (Invoke-MSTestWithCoverage.Scope.Tests.ps1)
+
+- The guarded part-file import (`if (Test-Path -LiteralPath $script:scopeScript) { . $script:scopeScript }`) is what let the expect-fail run report eight discrete `CommandNotFoundException` failures instead of one block-setup error; the comment above it says so. This is a good pattern for RED-first with a new part file.
+- Mocking `Invoke-DotnetCoverageExe` beneath the real `Invoke-DotnetCoverageCollection` means the AC2 case exercises the real exit-code check (`throw "MSTest with coverage failed with exit code 7"`) and the real derived-settings write and finally-block removal, both answered by mocks. `Should -Invoke ConvertTo-KoverageCoberturaXml -Times 0` then proves the run stopped before post-processing.
+- The two `Get-Content` parameter-filtered mocks are ordered after the unfiltered default, which is the Pester precedence the sibling suites rely on; the filters key on `$LiteralPath`, matching the production calls at lines 238 and 437.
+- `Should -Invoke Write-Warning -Times 1 -Exactly` without a filter is the assertion that proves "exactly one warning" across the whole run, including the trx-summary branch that would otherwise add a second warning; the filtered assertion then proves the content. Both are needed and both are present.
+- The fixture drive root `<fixture-root>` is the same neutral value the sibling suites use; it is not a host path.
+
+### Toolchain evidence read
+
+- Format: hashes identical before and after (P1-T7, P2-T1); liveness proven (P0-T5).
+- Analyze: 0 findings on all three changed files (P2-T2 runs C, D, E).
+- Test: 334/334 (P2-T3); RED partition 11/3 (P1-T3) then 14/14 (P1-T6).
+- Coverage: see CR-1 and the policy audit section 5.