[tylerkron]

CI went red on a run where nothing was wrong. Every test passed — 4,060 on each framework, plus the MCP suite — and then the Windows leg failed anyway, in coverlet's post-test MSBuild step, on Unable to read beyond the end of the stream while it read back its own hit file. The net9.0 coverage report was lost and the non-zero exit failed the job and the build gate with it. Coverage is reporting-only here and deliberately has no threshold that can fail a build, so this failure mode is all cost: a re-run fixes it, and on a merge-queue run it dequeues whatever PR was merging.

Coverage now comes from coverlet.collector instead, which runs inside the test host and has no post-test step that can fail. dotnet test is still a single solution-wide invocation with both frameworks in parallel, and the job summary and the uploaded artifact look exactly as they did.

What a reviewer might push back on. The collector puts no framework in the report name — it always writes coverage.cobertura.xml under a per-run GUID directory — so the framework has to survive in the path instead. The test project sets VSTestResultsDirectory to ../coverage/$(TargetFramework), which expands once per inner build; CI then flattens <tfm>/<guid>/coverage.cobertura.xml back to the coverage.<tfm>.cobertura.xml names that coverage-summary.py and the artifact already expect, so neither of those changed. Doing it this way (a project property, not --results-directory on the command line) is what avoids splitting the run into -f net9.0 and -f net10.0 steps, which would have serialized the two frameworks and cost about 35s a leg. coverlet.runsettings is load-bearing rather than decorative: without it the collector's own default exclusions move the reported line rate from 88.86% to 86.91%.

One real behaviour change for local development: dotnet test no longer prints coverlet's console coverage table. It writes cobertura under src/coverage/<tfm>/ instead.

Verification

• Full suite green on net9.0 and net10.0, both test projects.
• Wall clock measured before and after: 40.9s with coverlet.msbuild, 36.6s with the collector — both frameworks still run in parallel.
• Reported numbers identical to the old configuration on the same tree (net9.0 line-rate 0.8886, 18,165 valid lines).
• Ran CI's coverage step (flatten + coverage-summary.py) locally; the rendered table has the same shape and the same net9.0 / net10.0 rows.
• CoverageReportingTests guards the one relationship that is now split across two files — where the project writes reports vs. where the workflow looks for them — since a mismatch would degrade silently to "No coverage reports were produced by this run." Confirmed it fails when the workflow glob's depth is changed.

closes #689

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Collect coverage in-process to prevent false CI failures

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Replaces post-test MSBuild coverage with in-process collection to protect green test runs.
• Preserves parallel framework execution while restoring framework-labelled reports for summaries
 and artifacts.
• Adds contract tests for collector configuration, output paths, and CI discovery.

Diagram

graph TD
  CI["CI workflow"] --> Test["dotnet test"] --> Project["Test project"] --> Collector["Coverage collector"] --> Paths["TFM result paths"] --> Flatten["Report flattening"] --> Output["Summary artifacts"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Separate framework test runs

• ➕ Makes framework identity explicit on each command line.
• ➕ Can write directly to framework-specific report locations.
• ➖ Serializes or complicates solution-wide testing.
• ➖ Adds roughly 35 seconds per CI leg according to verification.

2. Dedicated coverage job

• ➕ Fully isolates coverage failures from the required test gate.
• ➕ Keeps coverage-specific configuration out of normal local test runs.
• ➖ Duplicates the full test suite and runner usage.
• ➖ Produces coverage from a different execution than the gated tests.

Recommendation: Keep the project-scoped in-process collector. It removes the known post-test failure mode, preserves one parallel multi-target invocation, and maintains existing report consumers; contract tests appropriately guard the otherwise fragile project-to-workflow path agreement.

Files changed (4) +207 / -12

Bug fix (1) +15 / -4

ci.ymlFlatten collector reports before publishing coverage +15/-4

Flatten collector reports before publishing coverage

• Updates CI documentation for in-process collection and discovers collector outputs beneath framework and GUID directories. It renames them to the existing framework-labelled Cobertura filenames before summary generation and artifact upload.

.github/workflows/ci.yml

Tests (1) +144 / -0

CoverageReportingTests.csGuard the coverage configuration contract +144/-0

Guard the coverage configuration contract

• Adds regression tests confirming the collector is enabled, the post-test MSBuild path remains disabled, and project output paths match the workflow glob. It also verifies the referenced runsettings file exists and requests Cobertura output.

src/Daqifi.Core.Tests/Build/CoverageReportingTests.cs

Other (2) +48 / -8

Daqifi.Core.Tests.csprojSwitch test coverage to the in-process collector +20/-8

Switch test coverage to the in-process collector

• Removes coverlet.msbuild collection properties and package usage. Configures VSTest to use coverlet.collector with framework-specific results directories and repository run settings, preserving parallel multi-target execution.

src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj

coverlet.runsettingsPreserve existing Cobertura coverage semantics +28/-0

Preserve existing Cobertura coverage semantics

• Adds collector settings that explicitly retain Cobertura output and exclude generated-code attributes. This keeps coverage rates and downstream report expectations comparable with the previous MSBuild configuration.

src/Daqifi.Core.Tests/coverlet.runsettings

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Report move can fail CI ✓ Resolved 🐞 Bug ☼ Reliability

Description

The reporting-only summary step now executes mv -f without handling failures, so a permissions,
lock, or filesystem error while flattening a coverage report returns a non-zero step result and can
fail an otherwise green matrix leg. This recreates the exact policy violation the PR is intended to
remove: coverage post-processing can still make the required build gate red.

Code

.github/workflows/ci.yml[97]

+          mv -f "$report" "src/coverage/coverage.$framework.cobertura.xml"

Relevance

●●● Strong

PR #648 explicitly accepted making coverage reporting-only; unsuppressed mv can violate that
requirement.

PR-#648

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new move is executed inside the unsuppressed Coverage summary step, while the required build job
reports success only when the entire test matrix succeeds. The summary script deliberately catches
all coverage parsing and output exceptions and returns zero, confirming that reporting failures are
not intended to affect CI; accepted PR #648 established the same requirement for this step.

.github/workflows/ci.yml[85-106]
.github/workflows/ci.yml[123-136]
.github/scripts/coverage-summary.py[169-204]
PR-#648

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Coverage flattening is reporting-only, but an unhandled `mv -f` failure can fail the CI job after all tests pass.

## Issue Context
The Python summary already converts all report parsing and output failures into warnings, and the workflow explicitly states that coverage has no build-failing threshold. Apply the same policy to the newly added filesystem post-processing while retaining a visible warning.

## Fix Focus Areas
- .github/workflows/ci.yml[85-106]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Workflow parser remains unguarded ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

ExpectedFrameworks_StayReadableTheWayTheWorkflowReadsThem duplicates the intended single-line
parser in C# but only verifies that the workflow mentions the project path, so changing or breaking
the workflow's sed extraction still leaves this test green. This defeats the test's stated purpose
of detecting drift between how the project declares frameworks and how CI reads them.

Code

src/Daqifi.Core.Tests/Build/CoverageReportingTests.cs[R144-145]

+        // And that the check is still pointed at this project.
+        Assert.Contains("src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj", WorkflowText, StringComparison.Ordinal);

Relevance

●●● Strong

PR #679 accepted the same duplicated-parser regression-test weakness; recent precedent directly
matches this finding.

PR-#679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test parses the project independently at lines 135-139, then its only assertion about CI at
lines 144-145 checks for the project path; the actual production parser is a separate sed pipeline
at .github/workflows/ci.yml:108, so its behavior is never exercised or pinned. Past PR #679
documents the same regression-test weakness: duplicating parsing logic instead of exercising the
implementation allows the real parser to regress unnoticed.

src/Daqifi.Core.Tests/Build/CoverageReportingTests.cs[135-145]
.github/workflows/ci.yml[108-111]
PR-#679

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The regression test reimplements the expected framework parser but does not verify that CI still uses that parser; it only checks that the workflow contains the project path.

## Issue Context
The production extraction is the `sed`/`tr` command in `.github/workflows/ci.yml`. A future edit can make that command incompatible while this test continues to pass.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Build/CoverageReportingTests.cs[120-145]
- .github/workflows/ci.yml[104-113]

Assert the actual workflow extraction command/pattern, or move framework extraction into a shared script that the workflow invokes and the test exercises directly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Partial coverage loss is silent ✓ Resolved 🐞 Bug ◔ Observability

Description

Coverage is now produced independently for two target frameworks, but the workflow warns only when
the flattened report array is completely empty; if collection fails for either net9.0 or net10.0,
the summary and artifact quietly contain just the surviving framework. This contradicts the stated
fallback that a missing collector report is surfaced by the summary and makes per-framework coverage
regressions invisible.

Code

.github/workflows/ci.yml[R95-98]

+        for report in src/coverage/*/*/coverage.cobertura.xml; do
+          framework=$(basename "$(dirname "$(dirname "$report")")")
+          mv -f "$report" "src/coverage/coverage.$framework.cobertura.xml"
+        done

Relevance

●●● Strong

Accepted coverage hardening favors surfacing missing reports; checking both expected frameworks is a
deterministic observability fix.

PR-#648

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test project declares two target frameworks and uses $(TargetFramework) to produce separate
report roots. The new loop flattens whatever reports happen to exist, but the subsequent condition
emits a diagnostic only when the resulting array has zero entries, with no check that both expected
framework labels are present.

src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj[3-25]
.github/workflows/ci.yml[89-106]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow detects total coverage loss but silently accepts a missing report for one of the two target frameworks.

## Issue Context
`Daqifi.Core.Tests` targets net9.0 and net10.0 and writes each framework to a separate collector results directory. Coverage remains reporting-only, so missing reports should produce warnings rather than fail the job.

## Fix Focus Areas
- .github/workflows/ci.yml[89-106]
- src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj[3-25]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: 🚀 Fast: This is a localized, single-file test enhancement validating one CI/workflow parsing relationship; it has limited blast radius and no security, API, schema, or concurrency risk.

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit e0b70a3

Results up to commit b164360 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Report move can fail CI ✓ Resolved 🐞 Bug ☼ Reliability

Description

The reporting-only summary step now executes mv -f without handling failures, so a permissions,
lock, or filesystem error while flattening a coverage report returns a non-zero step result and can
fail an otherwise green matrix leg. This recreates the exact policy violation the PR is intended to
remove: coverage post-processing can still make the required build gate red.

Code

.github/workflows/ci.yml[97]

+          mv -f "$report" "src/coverage/coverage.$framework.cobertura.xml"

Relevance

●●● Strong

PR #648 explicitly accepted making coverage reporting-only; unsuppressed mv can violate that
requirement.

PR-#648

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new move is executed inside the unsuppressed Coverage summary step, while the required build job
reports success only when the entire test matrix succeeds. The summary script deliberately catches
all coverage parsing and output exceptions and returns zero, confirming that reporting failures are
not intended to affect CI; accepted PR #648 established the same requirement for this step.

.github/workflows/ci.yml[85-106]
.github/workflows/ci.yml[123-136]
.github/scripts/coverage-summary.py[169-204]
PR-#648

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Coverage flattening is reporting-only, but an unhandled `mv -f` failure can fail the CI job after all tests pass.

## Issue Context
The Python summary already converts all report parsing and output failures into warnings, and the workflow explicitly states that coverage has no build-failing threshold. Apply the same policy to the newly added filesystem post-processing while retaining a visible warning.

## Fix Focus Areas
- .github/workflows/ci.yml[85-106]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Partial coverage loss is silent ✓ Resolved 🐞 Bug ◔ Observability

Description

Coverage is now produced independently for two target frameworks, but the workflow warns only when
the flattened report array is completely empty; if collection fails for either net9.0 or net10.0,
the summary and artifact quietly contain just the surviving framework. This contradicts the stated
fallback that a missing collector report is surfaced by the summary and makes per-framework coverage
regressions invisible.

Code

.github/workflows/ci.yml[R95-98]

+        for report in src/coverage/*/*/coverage.cobertura.xml; do
+          framework=$(basename "$(dirname "$(dirname "$report")")")
+          mv -f "$report" "src/coverage/coverage.$framework.cobertura.xml"
+        done

Relevance

●●● Strong

Accepted coverage hardening favors surfacing missing reports; checking both expected frameworks is a
deterministic observability fix.

PR-#648

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test project declares two target frameworks and uses $(TargetFramework) to produce separate
report roots. The new loop flattens whatever reports happen to exist, but the subsequent condition
emits a diagnostic only when the resulting array has zero entries, with no check that both expected
framework labels are present.

src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj[3-25]
.github/workflows/ci.yml[89-106]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow detects total coverage loss but silently accepts a missing report for one of the two target frameworks.

## Issue Context
`Daqifi.Core.Tests` targets net9.0 and net10.0 and writes each framework to a separate collector results directory. Coverage remains reporting-only, so missing reports should produce warnings rather than fail the job.

## Fix Focus Areas
- .github/workflows/ci.yml[89-106]
- src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj[3-25]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5f7f649

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit e0b70a3

[tylerkron]

Qodo-clean, CI green — ready for review. Verified on head e0b70a3: Qodo summary reports Bugs (0) / Rule violations (0) with all three findings resolved, zero unresolved qodo-code-review review threads, and re-confirmed after a 3-minute settle. CI green on all legs (build, macos, ubuntu, windows) on that exact SHA. Current origin/main (d75d82a, #703's IDE0161 + CA1707 build-time enforcement) is already an ancestor of this head, and a local dotnet build -c Release under those rules is clean — 0 warnings, 0 errors across net9.0 and net10.0.