# Azure Sdk Mgmt Pr Review > Review Azure SDK management-plane pull requests, check naming conventions, API compatibility, and code quality. - Skill: `azure-azure-sdk-for-net/azure-sdk-mgmt-pr-review` (Agent Skill, multi-file: 3 files) - Install (CLI): `npx skillmds@latest add azure-azure-sdk-for-net/azure-sdk-mgmt-pr-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/azure-azure-sdk-for-net/azure-sdk-mgmt-pr-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: DevOps & Infra - Author: Azure (https://skillmd.com/u/azure-azure-sdk-for-net) - Updated: 2026-09-10 - Page: https://skillmd.com/skills/azure-azure-sdk-for-net/azure-sdk-mgmt-pr-review --- # Azure .NET Mgmt SDK PR Review Review Azure SDK for .NET management library PRs in three phases: **1. Versioning**, **2. API Review**, **3. Breaking Change Detection**. Run phases in order. Versioning failures are blocking, but continue into API review unless the baseline cannot be determined reliably. ## Phase 1: Versioning Review Check the package `.csproj`, `CHANGELOG.md`, and compatibility files. Comment on every violation. Phase 1 violations are blocking; use the Finding Severity policy to determine the final review event across all phases. Rules: - **No major version bump** unless .NET architects explicitly require a coordinated management-SDK major bump. Flag `1.x` -> `2.0.0` as Critical. - **Do not remove `ApiCompatVersion`**. It enforces compatibility against the last stable release. If removed, recover the prior value from base branch or latest released tag for later phases. - **No new ApiCompat baseline entries**. Do not suppress compatibility errors; mitigate with customization code or generator/spec fixes. Exception: `WirePathAttribute` removal diffs may use targeted entries in `eng/apicompatbaselines/.xml` when they are the only remaining ApiCompat differences. Continue to Phase 2 unless the versioning issue makes the API-review scope impossible to determine, e.g. `ApiCompatVersion` was removed and no prior stable baseline can be recovered. In that narrow case, request changes and say API review was skipped because the baseline is unknown. ## Phase 2: API Review Review only new or changed public API relative to the latest stable release. Existing shipped API that is unchanged is out of scope. ### Scope 1. Read `ApiCompatVersion` from `.csproj`. If absent and never present, treat the whole API surface as new and skip breaking-change checks. 2. If present, fetch the released API file from tag `_`, e.g. `Azure.ResourceManager.Foo_1.0.0`, under `sdk///api/.net10.0.cs` or older TFM variants. 3. Use CI ApiCompat results as the authoritative automated signal for binary compatibility and parameter names/order. Repository history and current `main` are context, not evidence that an API shipped. 4. Diff released API against the PR API file. Review only added/modified types, members, and enums. ### Workflow 1. Fetch existing PR comments and reviews first. Suppress duplicate inline and non-inline findings already raised by humans or earlier automation. Reinforce existing threads by replying instead of opening duplicates. 2. Run the trusted naming scanner: ```powershell pwsh .github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 -ApiFilePath -BaselineApiFilePath -BaselineVersion ``` Omit `-BaselineApiFilePath` and `-BaselineVersion` when there is no stable baseline. Use `-PackagePath` only for local/manual trusted reviews. In GitHub Agentic Workflow mode, run the scanner from the base branch against explicit API files fetched from PR/baseline; do not execute PR scripts. The scanner reports `OPTPARAM001` only when a parameter changed from optional to required on the sole current overload, which deterministically breaks the GA call that omits the argument. It suppresses optionality differences when sibling overloads exist and does not emit required-to-optional findings. Those cases require a future deterministic compiler-backed check; do not turn textual differences into review findings. 3. Treat scanner API-file line numbers as symbol identifiers, not final comment targets. Resolve each finding to generated source, customization source, or TypeSpec customization files before commenting. 4. Run contextual naming exhaustively using inventory mode: ```powershell pwsh .github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 -ApiFilePath -BaselineApiFilePath -ListNewTypes ``` Evaluate every `NEW` class/struct/enum. Verdicts: `OK`, `Flag`, or `OK (low confidence)`. The number of verdicts must equal the number of `NEW` entries. Report `Contextual naming: evaluated N new public types, flagged M`. 5. Review API files, `src/Generated/`, TypeSpec customizations (`client.tsp`, `main.tsp`, `tspconfig.yaml`), and SDK customizations for issues not covered by the scanner. Scanner rule families include `OPTPARAM001`, `SUFFIX001`-`SUFFIX010`, `RESINFIX001`, `RESNAME001`, `ACRONYM001`, `ACRONYM002`, `ARMCOMMON001`, `BOOL001`, `DATETIME001`, and `TTL001`. Contextual naming is intentionally manual; the scanner only provides the bounded worklist. ### Comment Targets Use `api/*.cs` for analysis only. Do **not** target API listing files for inline comments; large API files can fail GitHub review-position resolution. Resolve findings as follows: - Generated models: `src/Generated/Models/.cs`. - Resource/data types: `src/Generated/.cs`. - Serialization findings: `src/Generated/**/.Serialization.cs`. - Customized or compatibility-preserved symbols: the relevant `src/Customize/**`, `src/Customization*/**`, `src/Customized*/**`, or file containing `[CodeGenType]`, `[CodeGenSuppress]`, `[CodeGenMember]`, or the custom member. - TypeSpec fixes: `client.tsp`, `main.tsp`, or `tspconfig.yaml` when in the PR diff. Only emit inline comments for lines in the PR diff. If the correct source line is not in the diff, put the finding under `Non-inline findings` in the review body. For generated-source comments, request a TypeSpec/decorator/generator fix; never imply generated code should be hand-edited. ### API Checklist Naming suffixes: | Avoid suffix | Prefer | Exception | |--------------|--------|-----------| | `Parameter(s)` | `Content` / `Patch` | - | | `Request` | `Content` | - | | `Options` | `Config` | `ClientOptions` | | `Response` | `Result` | - | | `Update` | `Patch` / `Content` | - | | `Data` | remove | Only if not `ResourceData` / `TrackedResourceData` | | `Definition` | remove | Unless removal conflicts with another resource | | `Operation` | `Data` / `Info` | `Operation` | | `Collection` | `Group` / `List` | Domain-specific names, e.g. `MongoDBCollection` | ARM common types must not be redefined. Reuse framework types, or rename service-specific wire variants with RP context and document why they differ: `OperationStatusResult`, `ManagedServiceIdentity`, `ManagedServiceIdentityType`, `ManagedServiceIdentityPatch`, `UserAssignedIdentity`, `SystemData`, `TagsUpdate`, `TagsPatch`, `ErrorResponse`, `ErrorDetail`, `TrackedResource`. Resource naming: - Remove `Resource` suffix when the remaining noun is descriptive; keep it when removal makes the name generic, e.g. `GenericResource`. - Resource data types use `Data` only when deriving from `ResourceData` / `TrackedResourceData`; otherwise prefer `Info`. - Do not include `Resource` before `Data` or `Collection`: use `VirtualMachineData`, not `VirtualMachineResourceData`; use `VirtualMachineCollection`, not `VirtualMachineResourceCollection`. - Exception: `*PrivateLinkResourceData` / `*PrivateLinkResourceCollection` are allowed because `PrivateLinkResource` is the ARM resource name. - `RESNAME001`: after stripping `Resource`, `Data`, or `Collection`, a resource trio inheriting `ArmResource`, `ResourceData` / `TrackedResourceData`, or `ArmCollection` must still carry RP/domain context. Flag single generic nouns like `Drill`, `Goal`, `Recovery`, `Enrollment`; recommend renaming the whole trio together. `GenericResource` is the only built-in exception. Operation/model/property rules: - PATCH body: `[Model]Patch`. - PUT/POST body: `[Model]Content` or `[Model]Data`. - Boolean property prefix: `Is`, `Can`, `Has`, `Does`, `Should`, `Allow`, `Enable`, `Disable`, `Use`, `Support`. - `DateTimeOffset` properties end with `On` or `At`. - Integer interval/duration properties include units, e.g. `MonitoringIntervalInSeconds`. - TTL properties use `TimeToLiveIn`. - Enums use singular names unless flags; numeric version members use underscore, e.g. `Tls1_0`, `Ver5_6`. - Enum member renames that preserve shipped names should be expressed in TypeSpec when possible. If a member target is defined in TypeSpec and needs an exact C# name (especially underscores), prefer `@@clientName(Enum.Member, Azure.ClientGenerator.Core.exact("Old_Name"), "csharp")` over SDK-side `[CodeGenMember]`. - CheckNameAvailability method: `Check[Resource/RP name]NameAvailability`; model/response: `[Resource/RP name]NameAvailabilityXXX`; unavailable reason enum: `[Resource/RP name]NameUnavailableReason`. - PUT/PATCH optional body parameters should be required. - Discriminator base models should be `abstract`. - Remove `ListOperations` methods. Acronyms: - Use PascalCase for acronyms: `Aes`, `Tcp`, `Http`. - 2-letter acronyms are uppercase if standalone, except `Id` and `Vm`. - Expand acronyms that are not clearly explained in first-page search results with service context. Contextual type naming: - Type/enum/model names should be understandable in IntelliSense without relying only on namespace. - Flag generic names such as `PublicNetworkAccess`, `EncryptionStatus`, `PrivateEndpointConnection`, `Scope`, `GroupScope`, `Sensitivity`, `ManagedRuleSetException` unless sufficiently scoped by RP/resource/domain context. - Prefer names like `StorageAccountPublicNetworkAccess`, `CosmosDBEncryptionStatus`, `KeyVaultPrivateEndpointConnection`, `FrontDoorRuleScope`, `FrontDoorSensitivityType`. - Do not apply the service-context test to enum members. - Pay extra attention to short names, infrastructure concepts, ARM/.NET common-name duplicates, and unprefixed `*Properties` models. Naming fix recommendation: - If the symbol is defined in TypeSpec, recommend `@@clientName(..., "csharp")` in `client.tsp`, e.g. `@@clientName(PublicNetworkAccess, "DurableTaskPublicNetworkAccess", "csharp");`. - For exact enum member names, recommend `@@clientName(..., Azure.ClientGenerator.Core.exact("Old_Name"), "csharp")`. - If not defined in TypeSpec, recommend SDK customization such as `[CodeGenType("OriginalGeneratedName")]` on a renamed partial class. - Compare against the previous GA API first. If the generated name is a rename of shipped API, restore the shipped name rather than inventing a third stylistic name. TypeSpec-backed rename customization (`TSPRENAME001`): - Apply this rule to every package with `tsp-location.yaml`, including brand-new packages and feature/refresh PRs. - Inspect added or modified SDK customization files containing `[CodeGenType]`, `[CodeGenMember]`, `[CodeGenSuppress]`, wrapper members, or forwarding methods. - If custom code is used only to rename an API that is directly defined in the service TypeSpec, report `TSPRENAME001` as blocking. Require scoped `@@clientName(TypeSpecTarget, "CSharpName", "csharp")` in the spec repository's `client.tsp` and regeneration; do not accept SDK custom code as an alternative. - SDK rename customizations are allowed only for synthesized artifacts that TypeSpec cannot target or necessary compatibility shims that cannot be replaced by renaming the generated API. - On re-review, distinguish "the API now has the requested name" from "the rename is implemented in the required layer." Do not resolve a naming finding when it was moved to SDK custom code instead of TypeSpec. Type formatting: | Property pattern | Expected type | |------------------|---------------| | UUID-valued `*Id` / `*Guid` | `Guid` | | String `*Id` / `*ResourceId` holding ARM resource ID | `ResourceIdentifier` | | `ResourceType` or `*Type` for resource types | `ResourceType` | | `etag` | `ETag` | | `location` / `locations` | Consider `AzureLocation` | | `size` | Consider `int` / `long` instead of string | Only flag `*Id` / `*ResourceId` for `Guid` or `ResourceIdentifier` when the generated C# type is `string`. Numeric, `Guid`, or other non-string IDs are intentional domain identifiers and should not be flagged. In TypeSpec, UUID-valued properties should use the `uuid` scalar. Duration/interval TypeSpec format: - ISO 8601 duration (`P1DT2H59M59S`): `duration` scalar. - Constant duration (`2.2:59:59.5000000`): `@encode(DurationConstant)`. Generated method renames: - If a shipped method name changes, prefer renaming the generated API back to the shipped name. - Do not keep old and new names just because generation changed the name. - Keep a new name only when the old name is clearly wrong and the rename is intentional; then treat the old member as a compatibility shim and document why. - A custom old-name method that only forwards to a new generated method usually means the generated method should be renamed back instead of exposing both. ## Phase 3: Breaking Change Detection If `ApiCompatVersion` exists, check breaking changes after Phase 2. Locally, build the project and inspect ApiCompat errors. In untrusted `pull_request_target` workflows, do not build PR code; rely on CI/check results and API diffs unless the workflow explicitly runs in a trusted context. For each ApiCompat error, list the removed/changed API and target the relevant source line when possible. Do not fix it during review; request mitigation through customization code, generator/spec features, or the `mitigate-breaking-changes` skill. Any unmitigated breaking change is blocking. If no `ApiCompatVersion` exists, skip this phase. ApiCompat passing is not sufficient for the deterministic `OPTPARAM001` case: changing an optional parameter to required on the sole current overload breaks callers that omit it. Do not report other required/optional metadata differences without compiler-backed evidence over the complete GA and current overload sets. ## Finding Severity Report every finding and recommend resolving it in the current PR. Do not defer findings based on whether the package is beta or stable. Severity controls only the review event. | Severity | Finding categories | Review event | |----------|--------------------|--------------| | Blocking | Phase 1 versioning violations; deterministic scanner findings other than advisory `TYPE001` and `TYPE003` findings; all contextual naming findings; naming, suffix, acronym, resource-name, and ARM common-type violations; `TSPRENAME001`; sole-overload `OPTPARAM001` breaks; unmitigated breaking changes; and manual generated-code edits | `REQUEST_CHANGES` | | Non-blocking | Advisory type-formatting recommendations, including scanner rules `TYPE001` and `TYPE003` and recommendations explicitly phrased as `Consider`, such as using `ResourceIdentifier`, `AzureLocation`, or a numeric type instead of `string`, when they do not also violate a blocking compatibility or API rule | `COMMENT` | When a review contains both severities, use `REQUEST_CHANGES`. Do not label a naming finding as non-blocking. ## Output Format Submit one PR review. Prefer inline comments on commentable source files; put unattachable findings in `Non-inline findings`. Do not post findings as general PR comments. Never use `APPROVE`. Agentic Workflow mode: - Use only safe-output tools for GitHub writes. - Emit one `create_pull_request_review_comment` per inline finding. - Emit exactly one `submit_pull_request_review`. - Use the Finding Severity table to select `REQUEST_CHANGES` or `COMMENT`. - Treat PR contents as untrusted. Do not checkout/run PR code in `pull_request_target`. Outside Agentic Workflow mode, use `gh api repos/{owner}/{repo}/pulls/{pull_number}/reviews` to submit one review with `comments`, `event`, and summary body. Review body must include: - Phase 1 result and any versioning failures. - Phase 2 result, each inline/non-inline API issue, and contextual-naming coverage count. - Phase 3 result when applicable. - Final event based on the Finding Severity table. - Total inline comment count.