Correct the MTP Dependency and Runner Claims in OPERATIONS.md - #97
Conversation
Answers three findings raised on #96, the promotion of #95. `PhotoCleanerTests.csproj` takes `PrivateAssets="all"` on `Microsoft.Testing.Extensions.CodeCoverage`, and deliberately not the `IncludeAssets` list its sibling references carry. That list omits the `compile` assets, and MTP's generated `SelfRegisteredExtensions.cs` then fails to build with `CS0234: The type or namespace name 'CodeCoverage' does not exist`. A comment records this, since the next reader has the same list five lines below to copy from. `OPERATIONS.md` had two wrong claims, both introduced by #95. The Dependabot runbook said `Microsoft.Testing.Extensions.CodeCoverage`, `xunit.v3`, and `Microsoft.NET.Test.Sdk` all resolve Microsoft.Testing.Platform 2.3.3. Only the first two declare it: the coverage extension directly, and `xunit.v3` through `xunit.v3.mtp-v2`. `Microsoft.NET.Test.Sdk` 18.9.0 declares `Microsoft.TestPlatform.TestHost` and `Microsoft.CodeCoverage`, the VSTest stack, and no MTP dependency at all. The skew hazard is therefore between two packages rather than three, and the paragraph now says so while keeping the point that `nuget-deps` bumps all three together. That paragraph also now prescribes `dotnet nuget why` rather than `dotnet list package --include-transitive`. The flat list cannot detect this skew: NuGet unifies the platform to one version per target framework, so it prints a single healthy row whichever major each package was built against, which is a false all-clear on exactly the failure being guarded. The `global.json` entry said `dotnet test` reaches the suite "rather than the VSTest host the .NET 10 SDK no longer runs it under". The SDK did not drop VSTest. What needs .NET 10 is the `global.json` `test.runner` key itself, since that is the SDK which reads it, and MTP predates the key. The entry now says that, and names all three things a return to VSTest would take rather than only the adapter. Verified: `dotnet nuget why PhotoCleanerTests/PhotoCleanerTests.csproj Microsoft.Testing.Platform` prints exactly the two roots the prose describes. Build (0 warnings), csharpier, `dotnet format style`, `dotnet husky run`, markdownlint, and the hub's `prose_lint.py` over the diff are clean, and the suite runs 373 tests at an unchanged 74.5% line rate.
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request marks the code coverage package as private and updates operational guidance for Microsoft.Testing.Platform, VSTest, Dependabot troubleshooting, and ChangesTest stack dependency guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk:⚪ Minimal · up to This PR corrects test-project dependency metadata and operational documentation without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Title checkExplanation The title accurately identifies the primary documentation changes in OPERATIONS.md, including corrected MTP dependency and runner information. It does not mention the related project-file change, but the title remains concise and specific. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoCorrect MTP dependency guidance and isolate coverage assets
AI Description
Diagram
High-Level Assessment
Files changed (2) |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to documentation corrections and a safe, build-oriented test dependency metadata tweak with no issues found in the reviewed diff.
Pull request overview
This PR corrects operational documentation about the Microsoft.Testing.Platform (MTP) dependency chain and test-runner requirements, and updates the test project’s coverage package reference metadata to prevent a build break from a copied IncludeAssets pattern.
Changes:
- Set
PrivateAssets="all"onMicrosoft.Testing.Extensions.CodeCoverageand document whyIncludeAssetsmust not be copied fromxunit.analyzers. - Fix
OPERATIONS.mdto correctly identify which packages declare MTP, and prescribedotnet nuget why(notdotnet list package --include-transitive) for diagnosing platform-major skew. - Clarify the
global.jsonguidance: .NET 10+ is required to read thetest.runnerkey, and reverting to VSTest would require multiple coordinated changes.
File summaries
| File | Description |
|---|---|
| PhotoCleanerTests/PhotoCleanerTests.csproj | Adds PrivateAssets="all" to the MTP coverage extension and documents why IncludeAssets is intentionally omitted. |
| OPERATIONS.md | Corrects MTP dependency claims, updates the Dependabot troubleshooting command, and clarifies the global.json/runner requirements and VSTest rollback steps. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Review by Qodo
1. |
Uh oh!
There was an error while loading. Please reload this page.
Qodo raised, against the previous commit, that the `global.json` entry named three things as selecting Microsoft.Testing.Platform and omitted the coverage package. Correct: with `Microsoft.Testing.Extensions.CodeCoverage` in place, a project carrying only the other three would run under VSTest with no coverage collector at all, so the list understated what the choice rests on. The entry now names four rather than three, and no longer frames them as a rollback recipe, since a recipe stated in a runbook rots into a wrong one. It describes the current state instead, which carries the same information without promising a procedure. It also records that a move back is not this repo's alone to make. The `dotnet test` invocation that collects coverage lives in the hub-hosted validator, so the runner choice is only partly local. Verified: build (0 warnings), 373 tests at an unchanged 74.5% line rate, markdownlint clean, and the hub's `prose_lint.py` clean over the diff, with the one remaining `sentence-length` hit on `OPERATIONS.md:47` being the pre-existing opening sentence this branch does not touch.
There was a problem hiding this comment.
🟢 Approval recommended
Changes are narrowly scoped, internally consistent with the referenced workflow contract, and appear unlikely to introduce functional or documentation regressions.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
The doc-drift half of the hub resync against ptr727/ProjectTemplate at 2.0.526, separated from the MTP migration (#95, #97) because the two are different drift classes. Audit run stamp 2026-08-30T20:48:19Z, hub f3b4cc9. - `GOVERNANCE.md`, two `verbatim` sections re-vendored byte-exact. "Durable Knowledge and Self-Improvement" had lost the bullet on when a durable rule earns a mechanical hook, and "Repository Boundaries and Write Safety" trailed the hub on the worktree-isolation bullet, which now names the `gh-write-guard` hook backstop and the standalone-clone fallback's grant. - `WORKFLOW.md`, two dropped hub clauses restored. D8.3 regains the sentence on a tracker setting `auto-merge: false`, and D1.2 regains the parenthetical naming both `needs:` edges on the validation job. Every repo-specific annotation on D1.1, D1.2, D1.4, D1.5, D4.1 and the S1 to S11 rows is kept. - `AGENTS.md`, the intro said the file holds "two things". It has held three since the Fleet Bootstrap section was carried, so it now says so. - `.editorconfig-checker.json`, the hub's `Exclude` block carried. This repo has no Python tree, but `uv` creates those cache directories when running a Python script from the hub against this checkout, so the excludes are reachable here. - `.github/skills/repo-worktree/SKILL.md` and `resync-a-repo/SKILL.md`, verbatim re-vendors. Deliberately not changed. `WORKFLOW.md` Section 3 keeps its local "and the hub's `repo-config/README.md`" pointer, which the hub does host and which matches how `AUDIT.md` already refers to hub-hosted tooling. Verified: every repo-specific block in the three mixed-fidelity files survives the re-vendor, checked by full-file diff against both the hub and the pre-change copy. All 18 verbatim `GOVERNANCE.md` sections and all 3 `AGENTS.md` sections now byte-match hub f3b4cc9, and `diff -rq` over `.github/skills/` is empty. markdownlint and the hub's `prose_lint.py` over the diff are clean.
* Re-Vendor Drifted Governance Sections and Skill Files from the Hub The doc-drift half of the hub resync against ptr727/ProjectTemplate at 2.0.526, separated from the MTP migration (#95, #97) because the two are different drift classes. Audit run stamp 2026-08-30T20:48:19Z, hub f3b4cc9. - `GOVERNANCE.md`, two `verbatim` sections re-vendored byte-exact. "Durable Knowledge and Self-Improvement" had lost the bullet on when a durable rule earns a mechanical hook, and "Repository Boundaries and Write Safety" trailed the hub on the worktree-isolation bullet, which now names the `gh-write-guard` hook backstop and the standalone-clone fallback's grant. - `WORKFLOW.md`, two dropped hub clauses restored. D8.3 regains the sentence on a tracker setting `auto-merge: false`, and D1.2 regains the parenthetical naming both `needs:` edges on the validation job. Every repo-specific annotation on D1.1, D1.2, D1.4, D1.5, D4.1 and the S1 to S11 rows is kept. - `AGENTS.md`, the intro said the file holds "two things". It has held three since the Fleet Bootstrap section was carried, so it now says so. - `.editorconfig-checker.json`, the hub's `Exclude` block carried. This repo has no Python tree, but `uv` creates those cache directories when running a Python script from the hub against this checkout, so the excludes are reachable here. - `.github/skills/repo-worktree/SKILL.md` and `resync-a-repo/SKILL.md`, verbatim re-vendors. Deliberately not changed. `WORKFLOW.md` Section 3 keeps its local "and the hub's `repo-config/README.md`" pointer, which the hub does host and which matches how `AUDIT.md` already refers to hub-hosted tooling. Verified: every repo-specific block in the three mixed-fidelity files survives the re-vendor, checked by full-file diff against both the hub and the pre-change copy. All 18 verbatim `GOVERNANCE.md` sections and all 3 `AGENTS.md` sections now byte-match hub f3b4cc9, and `diff -rq` over `.github/skills/` is empty. markdownlint and the hub's `prose_lint.py` over the diff are clean. * Drop the D1.2 Parenthetical Again, Which Is False for This Repo Both reviewers independently flagged the D1.2 clause restored in the previous commit. They are right, and the restore was a mistake. The hub's parenthetical reads "(both the aggregator and `smoke-build`, which `needs:` the validation job by name)". In this repo `smoke-build` declares `needs: [changes]` and nothing else, per `.github/workflows/test-pull-request.yml`, so the clause asserts an edge that does not exist here. The bullet's own repo-specific annotation two sentences later already says as much: the aggregator `needs:` validate directly, independent of `changes`/`smoke-build`. So its absence from this copy was a deliberate local adaptation rather than drift from the hub, and re-vendoring it put a false claim about this repo's own workflow into this repo's own contract doc. D1.2 goes back to the form it had on develop. This does not generalize to the D8.3 clause in the same commit, which stays. That one describes a tracker this repo does not have, rather than contradicting a workflow it does have, and `WORKFLOW.md` carries the whole D8 section as inapplicable-but-present already. Verified against the workflow rather than the doc: `grep` for the `needs:` lines in `test-pull-request.yml` gives `smoke-build` -> `[changes]` and `check-workflow-status` -> `[changes, validate, smoke-build]`. markdownlint and the hub's `prose_lint.py` over the diff stay clean.
Answers three findings raised on #96, the
develop -> mainpromotion of #95. A finding against a promotion PR cannot be fixed on that PR directly, since its head isdevelop, so the fix lands here first.PhotoCleanerTests.csproj: PrivateAssets, and Deliberately Not IncludeAssetsCopilot observed that
Microsoft.Testing.Extensions.CodeCoveragecarries noPrivateAssets/IncludeAssetsmetadata, unlike thecoverlet.collectorit replaced and thexunit.analyzersreference five lines below.Half of that is right, and the other half breaks the build. Measured both ways:
PrivateAssetsplus thatIncludeAssetslist fails to compile. The list (runtime; build; native; contentfiles; analyzers; buildtransitive) omitscompile, and MTP's generated registration then cannot see the type:obj/Debug/net10.0/SelfRegisteredExtensions.cs(17,9): error CS0234: The type or namespace name 'CodeCoverage' does not exist in the namespace 'Microsoft.Testing.Extensions'.PrivateAssets="all"alone is fine. 373 tests, exit 0,line-rate="0.7446903312045151", identical to before.So this takes
PrivateAssets="all"and adds a comment saying why theIncludeAssetshalf must not be copied, since the next reader has the exact list sitting five lines below to copy from.In fairness to the finding's own stated rationale, the effect here is consistency rather than a leak that was actually happening:
PrivateAssetssuppresses asset flow to consuming projects, and this test project is never packed and isProjectReferenced by nothing.OPERATIONS.md: Two Wrong Claims, Both Introduced by #95The Dependabot runbook named three packages as carrying Microsoft.Testing.Platform. Only two declare it.
dotnet nuget why PhotoCleanerTests/PhotoCleanerTests.csproj Microsoft.Testing.Platform:Microsoft.NET.Test.Sdk18.9.0 declaresMicrosoft.TestPlatform.TestHostandMicrosoft.CodeCoverage, the VSTest stack, and no MTP dependency at all (confirmed against its nuspec andproject.assets.json). The skew hazard is between two packages, not three. The paragraph now says so while keeping the point thatnuget-depsbumps all three together.That paragraph also now prescribes the right command. The review suggested
dotnet list package --include-transitive. That cannot detect this skew: NuGet unifies the platform to one version per target framework, so it prints a single healthy2.3.3row whichever major each package was actually built against. On the 18.0.x/MTP-1.x caseWORKFLOW.mdD1.6 describes, it would print that same healthy row while the run throwsTypeLoadExceptionand reports full coverage over zero tests, which is a false all-clear on precisely the failure the paragraph exists to guard.dotnet nuget whyreads declared dependencies and does answer it.The
global.jsonentry claimed the .NET 10 SDK "no longer runs" the suite under VSTest. It does not drop VSTest. What needs .NET 10 is theglobal.jsontest.runnerkey itself, since that is the SDK which reads it; MTP predates the key and an MTP test project is a self-hosting executable. The entry now says that, and names all three things a return to VSTest would take (the adapter, theIsTestingPlatformApplicationopt-out, and dropping the key) rather than only the adapter.Also Measured, Not Changed Here
Microsoft.NET.Test.Sdknow looks like dead weight, since VSTest cannot discover xUnit v3 tests without the adapter this repo dropped. It is not a free removal: without it the build fails withxUnit.net v3 test projects must be executable (set project property '<OutputType>Exe</OutputType>'), because it is what sets that implicitly today. That is its own change with its own surface, so it is not in this PR.Verification
Build (0 warnings),
dotnet csharpier check .,dotnet format style --verify-no-changes,dotnet husky run, markdownlint, and the hub'sprose_lint.pyover this diff are all clean. The suite runs 373 tests (368 passed, 5 Docker-gated skips) at an unchanged 74.5% line rate.Not Answered Here
The fourth finding on #96 is against
WORKFLOW.mdD1.6'ssecrets: inheritclaim. That is hub canonical text and a known hub defect, already filed as ptr727/ProjectTemplate#1114. It is declined in its own thread rather than fixed downstream.Summary by CodeRabbit
Documentation
Build & Configuration