Harden MT migration skill with defect classes found in a 150-task migration

[ViktorHofer]

Encodes into the migration skill and reviewer agent the defect classes found while migrating ~150 tasks to [MSBuildMultiThreadableTask] across dotnet/arcade (arcade#17381), dotnet/source-build-assets (#1776) and the dotnet/dotnet VMR.

Why

Microsoft.Build.TaskAuthoring.Analyzer was enabled in all three repos and every build was 0 warnings / 0 errors. A manual audit afterwards still found 9 real defects:

Why it was missed	Count
Code outside the analysis scope (unannotated base class)	2
API not on the monitored list (AssemblyName.GetAssemblyName)	3
Failure class not modeled at all (task-object race, memoized failure, nested task construction, path crossing a DI boundary)	4

Every one of these was findable by reading the code with the right checklist — so they belong in the skill. Analyzer gaps are filed separately against #14772 (#14791, #14792, #14793) plus #14794 for a runtime forcing function.

Changes

SKILL.md

• Sin 8 — swallowed exceptions hiding an unresolved path. Where a catch means a semantic answer ("not an assembly") rather than "fail", an unresolved path produces a silently wrong result instead of an error. In the VMR's CheckForPoison this skipped both poison checks, turning a leaked binary into a false negative in source-build's leak-detection gate — with a green build.
• Analyzer-invisible path consumers. MSBuildTask0003 monitors 8 types; anything else taking a path string is equally unsafe and silent. AssemblyName.GetAssemblyName on a raw input was the most-repeated defect of the whole exercise. Notes the string-vs-stream overload distinction.
• Unsafe code in an unannotated base class. The attribute is Inherited = false, so bases are unannotated — and under scope = multithreadable_only they are never analyzed, despite executing multithreaded. Real case: AkaMSLinksBase held File.ReadAllText on a raw task input while both derived tasks were annotated and clean.
• Engine-owned shared state: RegisterTaskObject. Non-atomic read/write with no static field in sight, so it reads as thread-safe. Includes the benign/broken decision table — LocateDotNet is fine, SingleError was not.
• Paths crossing interface / DI boundaries, where no System.IO type appears in the task at all.
• New "Verification" section — a clean analyzer run is not a migration, with the miss breakdown above and why "it worked when I tested it" is weak evidence for a failure mode that is load- and schedule-dependent.
• New "Migrating Is Not Always the Right Answer" section — leaving a task unannotated preserves today's TaskHost behavior exactly and is a supported outcome. A slower task beats a wrong one.
• Sign-off checklist extended with the above; fixed a pre-existing broken anchor to Sin 2.

mt-migration-reviewer.agent.md

• Operating rule 7: never accept a clean analyzer run as a basis for approval.
• Step 1 now walks the base chain, catch blocks, caches and DI boundaries.
• Step 2 hazard table gains analyzer-invisible APIs, DI boundaries, semantic-answer catches and the RegisterTaskObject pair. Also corrects an existing row: AssemblyName.GetAssemblyName was listed only under Sin 2 message leakage, missing that the call is itself CWD-dependent.
• Step 5: explicitly approve a justified decision not to migrate, so the agent does not create an incentive to maximize annotation count.

Docs only — no product code, no tests affected.

[Copilot]

Pull request overview

This PR updates the mt-migration plugin documentation to incorporate additional defect classes observed during a large multithreaded-task migration effort, extending the playbook and reviewer agent to explicitly cover analyzer blind spots (base-class scope gaps, analyzer-invisible path consumers, semantic-answer catch blocks, engine-owned shared state via RegisterTaskObject, and DI/interface boundaries).

Changes:

• Expands the migration playbook from “7” to “8” compatibility sins, adding a new Sin 8 section and extending the sign-off checklist accordingly.
• Adds explicit guidance and hazard-table entries for analyzer-invisible path consumers (e.g., AssemblyName.GetAssemblyName(string) and other string-overload path sinks) and DI/interface boundary path flows.
• Updates the MT migration reviewer agent rules/hazard table to enforce “clean analyzer run is not evidence” and to proactively audit base chains, semantic-answer catches, caches, and DI boundaries.

Show a summary per file

File	Description
plugins/mt-migration/skills/multithreaded-task-migration/SKILL.md	Extends the migration playbook with new defect classes (Sin 8), new hazard categories, and an expanded verification/sign-off checklist.
plugins/mt-migration/README.md	Updates the plugin overview text to reflect “8 deadly sins”.
plugins/mt-migration/agents/mt-migration-reviewer.agent.md	Strengthens reviewer operating rules and hazard table to cover analyzer gaps and approve justified non-migrations.

Review details

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite