mpg-migration-pr-review

name	mpg-migration-pr-review
description	Review Azure SDK management-plane migration PRs (Swagger/AutoRest -> TypeSpec). Checks customization quality, TypeSpec decorator usage, and migration-specific anti-patterns on top of the standard mgmt PR review.

Azure .NET Mgmt SDK Migration PR Review

Review Azure SDK for .NET management-library migration PRs from Swagger/AutoRest to TypeSpec. This skill extends azure-sdk-mgmt-pr-review; run its Phases 1-3 first, then run these migration phases.

Use this skill for migration PRs indicated by titles like Migrate to TypeSpec / MPG migration, adding tsp-location.yaml, deleting src/autorest.md, adding TypeSpec metadata.json, or broadly regenerating src/Generated/. Do not use it for normal TypeSpec-based management feature PRs unless they change migration shape or mitigation code.

Phases:

1. Versioning Review - base skill.
2. API Review - base skill.
3. Breaking Change Detection - base skill.
4. Migration Customization Review.
5. TypeSpec Decorator Preference Review.
6. Migration New API Triage.

Migration notes for base phases:

• Phase 2: migration often introduces generic C# names such as Scope, GroupScope, Sensitivity, ManagedRuleSetException; flag contextual naming issues and require @@clientName(..., "csharp") when defined in TypeSpec.
• Phase 3: migration breaking changes are expected but still must be mitigated. ApiCompat baseline entries remain forbidden except targeted WirePathAttribute removal entries in the centralized baseline file.

Phase 4: Migration Customization Review

Review added/modified files under src/Custom*/, src/Customization*/, or src/Customized*/. Comment on source/customization/TypeSpec lines in the PR diff; never target api/*.cs inline. Treat issues as blocking when they risk incorrect API shape, broken compatibility, generated-code drift, or unmaintainable migration debt; minor cleanup can be a suggestion.

Rules:

• 4.1 Justification comments: each customization file or significant block must explain why it exists: breaking-change mitigation, generator bug with issue link, naming fix, backward compatibility, etc.
• 4.2 [CodeGenMember] vs [CodeGenSuppress]: after confirming the member cannot be renamed in TypeSpec, use [CodeGenMember("GeneratedName")] for same-data renames so generator metadata, serialization, and samples remain linked. Use [CodeGenSuppress("MemberName", ...)] only when removing/suppressing a generated member or rewriting different behavior.
• 4.3 [CodeGenType] is not a spec-defined rename mechanism - blocking: do not accept [CodeGenType("SpecName")] custom partials that only rename a model, enum, union, property container, or other API that is directly defined in the service TypeSpec. Require @@clientName(TypeSpecTarget, "CSharpName", "csharp") in client.tsp and regeneration instead. [CodeGenType] is allowed only when the generated C# artifact is synthesized/not directly targetable in TypeSpec, or when it is part of a necessary GA compatibility shim that cannot replace the generated name.
• 4.4 EditorBrowsable(Never) misuse: only hide backward-compat members that have replacements. Never hide the only public constructor, method, or property for its purpose.
• 4.5 Rename in TypeSpec, don't hide-and-replace - blocking: when a TypeSpec-defined API has the wrong C# name, require a scoped @@clientName(..., "csharp") customization in the spec and regeneration. Do not suggest or accept [CodeGenType], [CodeGenMember], [CodeGenSuppress], wrapper members, or forwarding methods merely to rename that API. If the artifact is synthesized by the C# generator and is not directly targetable in TypeSpec, use the smallest SDK customization. A custom old-name method that only forwards to a new generated method usually means the generated method should be renamed back.
• 4.6 ModelFactory anti-patterns: do not suppress generated ModelFactory methods without strong justification. Backward-compat overloads should delegate to generated public ModelFactory overloads and translate renamed parameters/types as needed; do not call internal constructors or private Core helpers. Add only overloads that existed in the previous stable API.
• 4.7 Constructor suppression: if a generated constructor has the wrong signature, first fix root cause in : missing flattened properties -> ; wrong property type -> ; missing envelope members -> flatten the envelope. Review every constructor for a possible decorator fix.

Compact examples:

• Bad TypeSpec-defined type rename: [CodeGenType("CommunityEndpointProtocol")] public partial struct VirtualEnclaveCommunityEndpointProtocol.
• Good TypeSpec-defined type rename: @@clientName(CommunityEndpointProtocol, "VirtualEnclaveCommunityEndpointProtocol", "csharp"); in client.tsp, followed by regeneration.
• Bad rename: [CodeGenSuppress("Tls1_0")] plus custom Tls10 member.
• Good synthesized-member rename when TypeSpec cannot target it: [CodeGenMember("Tls1_0")] public static RedisTlsVersion Tls10 { get; } = ...;.
• Bad ModelFactory compatibility: private helper calling internal generated constructors.
• Good ModelFactory compatibility: compatibility overload delegates to ArmRedisModelFactory.RedisData(...).
• Bad generated edit: changing src/Generated/Models/SomeModel.cs by hand.
• Good generated fix: suppress in src/Custom/SomeModel.cs, provide custom member, then regenerate.

Phase 5: TypeSpec Decorator Preference Review

Prefer TypeSpec decorators over SDK custom code when they can express the fix. Decorators are easier to maintain, keep migration behavior centralized in TypeSpec/customization inputs, and reduce custom code.

Always include the "csharp" scope parameter for decorators in azure-rest-api-specs; unscoped decorators affect other language emitters.

Common replacements:

• [CodeGenType], [CodeGenMember], suppression, or wrapper code used only to rename a TypeSpec-defined API -> @@clientName(TypeSpecTarget, "CSharpName", "csharp").
• Manual wrapper properties for an unflattened Properties envelope -> @@flattenProperty(Model.properties, "csharp").
• Manual property type conversion (string -> ResourceIdentifier, AzureLocation, etc.) plus custom serialization -> @@alternateType(Model.prop, TargetType, "csharp").
• Custom pageable wrappers for old Pageable<T> / AsyncPageable<T> shape -> @@markAsPageable(Interface.operation, "csharp"), with #suppress "@azure-tools/typespec-azure-core/no-legacy-usage" "migration" when needed.
• Public accessibility for an internal generated type -> try @@access(Type, Access.public, "csharp") before [CodeGenType]; fall back only when @@access is ineffective, such as nested/wrapper generated types.

When suggesting a decorator, include a scoped example and target the relevant client.tsp/TypeSpec line when in the diff.

On re-review, inspect every newly added rename customization and compare it with prior review guidance. If an earlier comment requested a TypeSpec rename but the author added SDK custom code instead, keep the finding blocking; do not mark the naming issue resolved. The review summary must distinguish "API has the requested name" from "rename implemented in the required layer."

Phase 6: Migration New API Triage

Migration PRs can show many additive public APIs after regeneration. Classify each meaningful new type/member from the API diff before accepting it.

1. Compare PR API against previous GA API from ApiCompatVersion when available.
2. For each meaningful new public API, classify it:

Category	Identify by	Review action
Real service/API-version addition	Exists only in newer TypeSpec input/API version; no equivalent in previous GA swagger/API surface	Keep; mention significant additions in summary.
Rename of existing API	Same operation id, route, resource type, model semantics, or member semantics as previous GA API	Request changes: restore shipped name/shape with scoped @@clientName or another TypeSpec decorator when directly targetable. Use a minimal SDK shim only for artifacts that TypeSpec cannot target or compatibility behavior that a rename cannot preserve.
Generator convenience/drift	New overload/grouping/resource method/pageable/collection method appears because MPG grouped or named the same REST operation differently	Investigate; request a rename/decorator fix only when the generated API can be renamed in TypeSpec or otherwise expressed by a supported spec-level customization. If the API is the correct generated shape from TypeSpec and cannot be renamed in spec, keep it and preserve the previous GA shape with the smallest compatibility shim needed for ApiCompat.

Use operation IDs, generated XML docs, request paths, resource type/parent hierarchy, and previous GA API listings to distinguish real additions from renamed existing APIs. When a spec-level rename or decorator can produce the shipped API shape, do not keep both old and new names unless there is an approved deliberate reason. When the new API is the correct generated shape from TypeSpec and cannot be renamed in spec, keep it and preserve the old GA API shape for compatibility.

Duplicate API detection:

• Scan for duplicate model/type shapes (Foo, FooResource, FooData, FooGenerated, FooContent), duplicate extensible enums with the same wire values or case-only members, and duplicate methods/overloads for the same operation ID, route, resource type, and wire parameters.
• Compare each suspected duplicate with the previous GA API and the TypeSpec source. Post a duplicate-API alarm only when the extra public API can be removed by renaming the TypeSpec-defined element with @@clientName(..., "csharp") or another supported spec-level decorator.
• Do not alarm on duplicates that are the correct generated shape from TypeSpec but cannot be renamed in spec. In those cases, honor the generated code from the spec and keep the previous GA shape through minimal compatibility customizations so ApiCompat remains satisfied.
• Prefer spec-level renames for TypeSpec-defined duplicates. Use SDK customizations such as [CodeGenType], [CodeGenMember], or [CodeGenSuppress] only for generated C# artifacts that are not directly expressible in TypeSpec or for the GA compatibility shim.

Output Format

Follow base skill output rules: one PR review, safe-output tools in GitHub Agentic Workflow mode, no direct GitHub writes from agentic workflows, no execution of untrusted PR code, and no inline comments on api/*.cs.

Each migration inline comment should start with a rule ID such as **[4.1]**, **[4.11]**, or **[5.2]**, explain the violation, and suggest the fix. Put unattachable findings in Non-inline findings.

Review body must include:

• Phase 1-3 results from the base skill.
• Phase 4 customization result, grouped by 4.1-4.11.
• Phase 5 decorator-preference result.
• Phase 6 new API triage result, including accepted real additions, flagged rename/drift counts, and duplicate model/enum/operation counts.
• Final counts for critical issues, should-fix issues, and suggestions.