Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 56 additions & 64 deletions .claude/agent-memory/feature-review/MEMORY.md

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
name: 441-review-residuals-and-494-handoff
description: "#441/#478 review (epic build-ci-coverage-gate-fidelity wave 0): PASS 0 blocking; residuals NF-1 (uncovered max-hits line 220) + NF-2 (poshqc zero-coverage artifact); corrected repo line-rate 85.0317% handoff owned by #494"
metadata:
type: project
---

Review of `bug/cobertura-coverage-arithmetic-441` (2026-08-10T23-35, base edf3d34c): PASS, 0 blocking, all 20 spec ACs independently verified. Facts later epic-sibling reviews (#457 wave 1, #494 wave 2, #512) will need:

- **Threshold margin handoff:** the corrected package-filtered line rate for the #424 sample is **62345 valid / 53013 covered = 0.850317 (85.0317%)** vs the uniform 85% floor — a 0.03 pp margin. Pre-fix inflated figure was 0.856453. #494 owns any threshold decision; #441 changed no threshold (verified empty diff on CLAUDE.md/.claude/rules/coverage.config). If #494's review sees a threshold edit, this is its provenance.
- **Coverage-arithmetic oracle now available:** post-fix `Get-CoberturaCoverageSummary` reproduces a raw dotnet-coverage document's own root attributes exactly (79957/56124/23109/13472 on the #424 baseline). Any future review can use generator parity as a cheap correctness probe.
- **Residual NF-1:** `Helpers.ps1:220` (`$existing.Hits = $hits`, the later-entry-larger-hits dedup update) has no covering test; recommended fixture rides a later change (#529/#530 work). If a later diff touches `Get-CoberturaClassLineSummary`, check whether the gap got closed.
- **Residual NF-2:** filed as a recommendation only — the `run_poshqc_test` zero-coverage capture defect (see [[poshqc-bundled-coverage-artifact-reads-zero]]) was NOT promoted to an issue by the reviewer (no promotion tools in-session); the orchestrator was asked to file it or fold into #512. Verify it did not evaporate.
- Follow-ups #529 (package rates stale), #530 (merged class keeps only primary `<methods>`), #531 (discovery lacks `\.claude\` exclusion — same defect as the local-vstest worktree pollution memory), #532 (wrong agent-memory generalization) are OPEN and deliberately unfixed in #441.

**How to apply:** when reviewing #457/#494/#512, read `<441-FEATURE>/evidence/other/threshold-handoff-494.2026-08-10T23-15.md` before adjudicating any threshold or coverage-figure claim, and expect coverage baselines recaptured after b52874d6 to be non-comparable with pre-fix history (denominators shrank).
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
name: poshqc-bundled-coverage-artifact-reads-zero
description: "run_poshqc_test writes artifacts/pester/powershell-coverage.xml with ZERO covered lines repo-wide (invalid capture); it is the hook's canonical PowerShell path and forces a FAIL row whenever PowerShell is enumerated"
metadata:
type: project
---

The bundled MCP tool `mcp__drm-copilot__run_poshqc_test` writes producer output to `artifacts/pester/` (`pester-junit.xml`, `powershell-coverage.xml`, `powershell-coverage.koverage.xml`; gitignored via `.gitignore:57`). Observed in the #441 review (2026-08-10): the JaCoCo `powershell-coverage.xml` recorded **0 covered / 16075 missed lines for every file in the repo**, including a file whose committed direct-Invoke-Pester JaCoCo at the same head read 90.59%. The capture is an invalid instrument.

**Why:** `artifacts/pester/powershell-coverage.xml` is exactly the path `validate-feature-review-coverage.ps1` reads for PowerShell repo-wide coverage (`Get-JacocoRepoCoverage`). A 0% reading is below the 85% floor, so once PowerShell is enumerated from `pr_context.summary.txt` the hook REQUIRES a FAIL verdict on a PowerShell coverage row. Pester JaCoCo emits no BRANCH counters, so the branch check returns null and skips (no unconditional block, unlike the C# #328/#398 cases).

**How to apply:** In any TaskMaster PowerShell feature review: (1) check the artifact's actual counters before writing rows; (2) if it reads 0, write the repo-wide row as an honest FAIL against the artifact, disposition non-blocking as a pre-existing tool measurement defect, and adjudicate the real floors from the committed feature-evidence direct-Pester JaCoCo (changed-file, new-code, changed-line regression); (3) recommend filing a tooling follow-up. Executors document the MCP tool's payloads as "non-probative" — the direct `Invoke-Pester` runs are the probative evidence. Also note the #441 executor CORRECTION: an empty `git status --porcelain` does NOT prove the tool wrote nothing, because `artifacts/` is gitignored wholesale. Related: [[coverage-hook-skips-when-no-pr-context-summary]], [[project_stale-untracked-coverage-xml-leftover-false-block]].
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Code Review — 2026-08-10-cobertura-coverage-arithmetic-441

- **Timestamp:** 2026-08-10T23-35
- **Reviewer:** feature-review agent
- **Branch:** `bug/cobertura-coverage-arithmetic-441` at `3b8d43fb` vs base `edf3d34c`
- **Files reviewed:** `scripts/vscode/Invoke-MSTestWithCoverage.Helpers.ps1` (357 -> 455 lines), `tests/scripts/vscode/Invoke-MSTestWithCoverage.Helpers.Tests.ps1` (222 -> 468 lines, pure addition)

## Executive Summary

The fix is well-scoped and correctly targeted. The one defective descendant-axis selection (`$cls.SelectNodes('.//lines/line')`, old line 122) is replaced by delegation to a new pure helper, `Get-CoberturaClassLineSummary`, which enumerates the class-level rollup and the method-level view over the child axis, keys by line number, and resolves duplicates by max hits / branch-if-either / larger-denominator condition-coverage precedence. The #478 blended-denominator defect is fixed by replacing the synthetic-document delegation with a direct helper call on the merged class node. The protected union builder is byte-identical to the base. Test additions are disciplined: fail-before was demonstrated with count-based (not rate-based) assertions, fixtures are pure in-memory here-strings, and the helper's precedence branches each have a dedicated unit test. No blocking defects were found. Two Minor and three Informational findings are recorded; the substantive one is a single untested statement (the max-hits update at line 220).

## Findings Table

| Severity | File | Location | Finding | Recommendation | Rationale | Evidence |
| --- | --- | --- | --- | --- | --- | --- |
| Minor | scripts/vscode/Invoke-MSTestWithCoverage.Helpers.ps1 | line 220 | `$existing.Hits = $hits` (the max-hits update when a later duplicate carries strictly larger hits) is executed by no test; every existing dedup fixture presents the larger or equal hits value first, so the update branch is dead in the suite | Add one fixture where the class-level rollup records `hits="0"` and a method-level entry records `hits="1"` for the same line number, asserting `CoveredLines = 1` | The helper's documented contract says duplicates resolve by max(hits); the contract's distinguishing case (later entry larger) is currently unpinned, so a future regression to first-entry-wins would pass the suite | Committed JaCoCo `evidence/qa-gates/pester-coverage-final.2026-08-10T23-10.xml` shows line 220 `mi=1 ci=0`; reviewer re-summed; executor recorded the same gap in `evidence/qa-gates/coverage-delta.2026-08-10T23-10.md` |
| Minor | (tooling, outside diff) | artifacts/pester/powershell-coverage.xml | The bundled `run_poshqc_test` coverage capture records zero covered lines for every file in the repository, contradicted by direct-Pester evidence at the same head | File a follow-up tooling issue through the promotion lifecycle (or fold into #512); do not rely on this artifact for any coverage gate until fixed | An artifact that reads 0.00% for a file with a passing 19-test suite is an invalid instrument; it is also the canonical path the feature-review coverage hook reads for PowerShell | Reviewer parse of the artifact (LINE counters: 16075 missed, 0 covered); § 2.5 of the policy audit |
| Info | docs/.../evidence/qa-gates/poshqc-analyze.2026-08-10T23-10.md | test-file section | Prose says "The 167 added lines introduced no analyzer debt"; the final test-file delta is 246 added lines (167 fixtures + 79 helper tests) | None required; correct if the artifact is ever revised | The finding-set arithmetic (0 findings before and after) is correct and was independently re-verified at HEAD; only the narrative line-count is stale | Reviewer `Invoke-ScriptAnalyzer` run; `git diff --numstat` |
| Info | scripts/vscode/Invoke-MSTestWithCoverage.Helpers.ps1 | lines 368-371 | Merged-class rate expressions duplicate the rounding/fallback logic of `Get-CoberturaCoverageSummary` instead of sharing a second helper | None; accepted design | In-code comment records the rationale: the spec constrains the change to exactly one new helper, and existing assertions depend on identical `[math]::Round(...,6)` rounding and the `'0'` zero-denominator fallback | Diff hunk at old lines 267-279; test `line-rate | Should -Be '1'` |
| Info | scripts/vscode/Invoke-MSTestWithCoverage.Helpers.ps1 | lines 251-259 | `Get-CoberturaClassLineSummary` returns its internal `$lineMap` (with live `Node` references into the caller's XML document) as part of its output object | None required now; consider documenting that `LineMap` entries are mutable views if a third consumer is ever added | Both current consumers only read the aggregate counters or (in the pre-existing merge path pattern) intentionally use nodes; the exposure is deliberate per the helper's `.OUTPUTS` help, and the helper mutates nothing itself | Source read; both call sites inspected |

## Design and Correctness Assessment

- **Root-cause fidelity.** The change edits exactly the defective site and its delegation path, and nothing else. The issue text's line references were wrong (declaration lines, not selections); the spec's corrected site table is what the diff implements. Reviewer verification: zero descendant-axis `.//lines/line` selections remain; the child-axis union builder is byte-identical (`cmp` old 217-268 vs new 311-362).
- **Arithmetic correctness.** Independently reproduced on three inputs: the raw generator document (79957/56124/23109/13472, exactly matching the generator's own root attributes), the pre-change defective figures (161086/113219/46218/26944 — branches exactly 2x, lines slightly less than 2x due to non-uniform duplication, both consistent with the two-view duplication model), and the package-filtered A/B (62345/53013/0.850317).
- **Precedence rule reuse.** The condition-coverage precedence (larger Total, tie-broken by larger Covered) reuses `Get-CoberturaLineConditionCoverageParts` rather than re-deriving the regex, and matches the precedence already used by the merge path — one rule, one implementation.
- **StrictMode safety.** The switch from property access to `GetAttribute` is deliberate and commented (`Set-StrictMode -Version Latest` makes missing XML attribute property access throw). Behavior note: `GetAttribute` on an absent `hits` attribute returns an empty string and `[int]''` throws — fail-fast is acceptable here because Cobertura `<line>` elements always carry `number` and `hits`, and the pre-existing code had the same effective contract.
- **Purity.** The helper performs no I/O and does not mutate the source document; `TotalLines` derives from the dedup map count, not the raw node count. Enumeration order (rollup first, then methods) combined with the update rules makes the result order-insensitive for hits and branch flags; the condition-coverage tie-break (strictly-greater Covered) is order-sensitive only between entries with identical Total and identical Covered, where the retained node is behaviorally indistinguishable.
- **Error handling.** The `<packages>` guard is retained verbatim and now pinned by a dedicated test. No silent catches introduced.

## Test Quality Assessment

- **Fail-before discipline.** F1-F4 assert counts (`lines-valid`, `lines-covered`, `branches-valid`, `branches-covered`) precisely because the rates do not discriminate — the fail-before artifact records that F1's rate assertion passed against the defective code, validating the fixture-design-trap warning. Reviewer reproduced the four failures against the extracted base file with the exact recorded wrong values.
- **Purity.** All 11 new `It` blocks use inline single-quoted here-strings cast to `[xml]`; zero disk writes, zero mocks in new tests, zero external dependencies. Grep for file-system APIs over the test file: no matches (the one `Mock` is a pre-existing allowlist test, untouched).
- **Coverage of the new helper.** 39/40 lines (97.50%); all three precedence branches and the neither-element boundary have dedicated tests; the single gap is the Minor finding above.
- **Non-regression.** Test-file diff is +246/-0; all 8 pre-existing `It` blocks pass unmodified (19/19 at HEAD, reviewer-run).
- **F5/F6 as designed.** Both pass before and after the fix (guard fixtures, correctly not tagged expect-fail): F5 pins that method-only lines are not dropped by the union approach; F6 pins that `<methods>` subtrees are neither merged nor stripped, which is the recorded non-goal now filed as issue #530.

## Verdict

**PASS — 0 blocking findings; 2 Minor and 3 Informational non-blocking findings.** The change is merge-ready as reviewed; NF-1's recommended fixture can ride any later change to this file (or #530/#529 work) without holding this merge.
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# Assumption 2 Subset Proof (P0-T18)

Timestamp: 2026-08-10T22-30

Proves `spec.md` § Assumptions item 2 — *method-level line numbers are a subset of the class-level
rollup* — exhaustively on both committed sample documents, **before** any implementation change, so
that document drift fails at baseline rather than after Phase 2.

`spec.md` records this assumption as verified on three spot-checked classes only and explicitly
`UNVERIFIED` across all classes, because exhaustive proof requires script execution. This artifact
supplies that execution.

Method: a **read-only streaming `XmlReader` pass** (not an `[xml]` cast — the inputs are 17.5 MB and
10.4 MB) that applies, per `<class>`, exactly the union and `max(hits)` rule this plan specifies:
enumerate both the class-level `./lines/line` axis and the `./methods/method/lines/line` axis, key
by `[int]number`, resolve repeats by `max(hits)` / `branch=True` if either / `condition-coverage`
from the larger `Total` tie-broken by larger `Covered`. A **method-only line key** is a key observed
on the method axis and never on the class-level axis.

Command:

```powershell
$root = (git rev-parse --show-toplevel) -replace '/', '\'
& <scratchpad>\Test-Assumption2.ps1 -Path (Join-Path $root 'docs\features\active\2026-08-06-quickfiler-high-confidence-queue-init-stall-424\evidence\baseline\coverage-baseline.cobertura.xml')
& <scratchpad>\Test-Assumption2.ps1 -Path (Join-Path $root 'docs\features\active\2026-08-06-quickfiler-high-confidence-queue-init-stall-424\evidence\qa-gates\coverage-final.cobertura.xml')
```

The analysis script is a throwaway agent-session script held outside the repository (scratchpad); it
writes nothing, mutates nothing, and is not part of the change. Its full logic is described above.

EXIT_CODE: 0

Output Summary:

```
FILE=coverage-baseline.cobertura.xml
classes=3169
class-level distinct=79957
union distinct=79957
union covered=56124
union branches valid=23109
union branches covered=13472
method-only line keys=0

FILE=coverage-final.cobertura.xml
classes=534
class-level distinct=62345
union distinct=62345
union covered=53013
union branches valid=15828
union branches covered=12445
method-only line keys=0
```

## `coverage-baseline.cobertura.xml` (raw generator output, 3169 classes)

| Quantity | Required by P0-T18 | Measured | Match |
| --- | --- | --- | --- |
| class-level distinct | 79957 | **79957** | yes |
| union distinct | 79957 | **79957** | yes |
| union covered | 56124 | **56124** | yes |
| union branches valid | 23109 | **23109** | yes |
| union branches covered | 13472 | **13472** | yes |
| **method-only line keys** | **0** | **0** | yes |

## `coverage-final.cobertura.xml` (post-processed, 534 classes)

| Quantity | Required by P0-T18 | Measured | Match |
| --- | --- | --- | --- |
| class-level distinct | 62345 | **62345** | yes |
| union distinct | 62345 | **62345** | yes |
| union covered | 53013 | **53013** | yes |
| **method-only line keys** | **0** | **0** | yes |

Additionally measured (not required by P0-T18, recorded as an observation): union branches valid
**15828** and union branches covered **12445**. This falls inside the range `[15730, 16582]` that the
research derived analytically for class-level `branches-valid` on this document, against the emitted
defective value of 27848. It is an independent forward prediction of the post-change branch figures
for this input.

## Verdict

**Assumption 2 holds exhaustively on both documents.** `union distinct == class-level distinct` and
`method-only line keys == 0` on every one of the 3169 + 534 = 3703 classes, so the union design and
the class-level oracle agree everywhere. The union formulation therefore reproduces the generator's
own arithmetic exactly (79957 / 56124 / 23109 / 13472), which is the primary correctness oracle for
AC-1.

No spec-level finding is raised. No plan or spec revision is required. The Phase 5
return-to-Phase-2 loop is not implicated by this task in any case.
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Git Baseline — 2026-08-10-cobertura-coverage-arithmetic-441

Timestamp: 2026-08-10T22-30

Command:

```powershell
$root = (git rev-parse --show-toplevel) -replace '/', '\'
Set-Location $root
git rev-parse --abbrev-ref HEAD
git rev-parse HEAD
git rev-parse edf3d34c
git merge-base --is-ancestor edf3d34c HEAD; 'ancestor-check-exit=' + $LASTEXITCODE
git status --porcelain
```

EXIT_CODE: 0

Output Summary:

```
bug/cobertura-coverage-arithmetic-441
a5e336e5ae3443d4197caf5f87036fae1d538f89
edf3d34cb9cd455bd3c1d9f5ee363b825632073c
ancestor-check-exit=0
--- porcelain ---
M docs/features/active/2026-08-10-cobertura-coverage-arithmetic-441/plan.2026-08-10T14-07.md
?? docs/features/active/2026-08-10-cobertura-coverage-arithmetic-441/evidence/
--- end porcelain ---
```

## Assessment

- **Branch:** `bug/cobertura-coverage-arithmetic-441` — matches the plan's required branch exactly.
No branch is created, switched, or renamed by this plan.
- **HEAD:** `a5e336e5ae3443d4197caf5f87036fae1d538f89`. Recorded as an observation only; it is never
used as a later expectation.
- **Base commit:** `edf3d34c` resolves to `edf3d34cb9cd455bd3c1d9f5ee363b825632073c`.
`git merge-base --is-ancestor edf3d34c HEAD` exited **0**, confirming the base is an ancestor of
HEAD, so `git diff ... edf3d34c` gates are well-defined.
- **Working-tree state at baseline:** the plan file
`plan.2026-08-10T14-07.md` is already ` M` (line-ending normalization performed by
`epic-orchestrator` before delegation) and the feature `evidence/` directory is untracked because
this run created it. Both source files under `scripts/` and `tests/` are clean at baseline.
Loading