From b96d4d057747464a3ed1d6f3fb469bc0c15ea1d0 Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Thu, 13 Aug 2026 04:21:04 +0000 Subject: [PATCH 1/3] Improve management parameter compatibility review Use released GA assembly metadata as the authoritative baseline, keep raw optional-parameter differences non-blocking until compiler probes demonstrate a break, and add regression coverage for exact signatures and candidate reporting. Fixes #61975 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Check-MgmtNamingRules.ps1 | 80 +++++-- .../Export-GaApiBaseline.ps1 | 202 ++++++++++++++++++ .../skills/azure-sdk-mgmt-pr-review/SKILL.md | 26 ++- .../test/Check-MgmtNamingRules.tests.ps1 | 103 +++++++++ .github/workflows/mgmt-review.lock.yml | 40 ++-- .github/workflows/mgmt-review.md | 21 +- 6 files changed, 426 insertions(+), 46 deletions(-) create mode 100644 .github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1 create mode 100644 .github/skills/azure-sdk-mgmt-pr-review/test/Check-MgmtNamingRules.tests.ps1 diff --git a/.github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 b/.github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 index 3f064f56d4d5..dd52b96f7eae 100644 --- a/.github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 +++ b/.github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 @@ -24,6 +24,10 @@ on types/members that are new or changed compared to the baseline will be reported. This enables deterministic filtering without relying on LLM judgment. +.PARAMETER BaselineVersion + Released package version represented by BaselineApiFilePath. Included in compatibility + findings so reviewers can identify the authoritative GA contract that was compared. + .PARAMETER ExcludeRules Array of rule IDs to skip (e.g., 'SUFFIX001', 'BOOL001'). @@ -50,6 +54,9 @@ param( [Parameter(Mandatory = $false)] [string]$BaselineApiFilePath, + [Parameter(Mandatory = $false)] + [string]$BaselineVersion, + [Parameter(Mandatory = $false)] [string[]]$ExcludeRules = @(), @@ -123,12 +130,12 @@ if (-not (Test-Path $ApiFilePath)) { } Write-Host "Scanning: $ApiFilePath" -ForegroundColor Cyan -$lines = Get-Content $ApiFilePath +$lines = @(Get-Content $ApiFilePath) $totalLines = $lines.Count # Load baseline API file for filtering (if provided) -$baselineLines = @{} -$baselineTypeKeys = @{} +$baselineLines = [System.Collections.Generic.Dictionary[string, bool]]::new([System.StringComparer]::Ordinal) +$baselineTypeKeys = [System.Collections.Generic.Dictionary[string, bool]]::new([System.StringComparer]::Ordinal) if ($BaselineApiFilePath) { if (-not (Test-Path $BaselineApiFilePath)) { throw "Baseline API file not found: $BaselineApiFilePath" @@ -342,14 +349,17 @@ function Get-ApiMethodInfos([string[]]$apiLines) { Name = $Matches['name'] Type = $parameterType IsOptional = $defaultSeparator -ge 0 + Default = if ($defaultSeparator -ge 0) { $parameter.Substring($defaultSeparator + 1).Trim() } else { $null } }) } $key = "$namespace|$typeName|$memberName|$($parameterTypes -join ',')" $methods[$key] = [pscustomobject]@{ + Namespace = $namespace TypeName = $typeName MemberName = $memberName Parameters = $parameters.ToArray() + Signature = "$memberName($parameterText)" Line = $lineIndex + 1 } } @@ -357,6 +367,7 @@ function Get-ApiMethodInfos([string[]]$apiLines) { return $methods } + #endregion #region --- Inventory mode (-ListNewTypes) --- @@ -407,19 +418,23 @@ if ($ListNewTypes) { #region --- Rule Checks --- # ===================================================== -# RULE: OPTPARAM - Preserve required/optional metadata +# RULE: PARAM/OPTPARAM - Preserve GA parameter compatibility # ===================================================== -# ApiCompat primarily protects binary compatibility. Changing whether a shipped -# parameter is optional can still break source compilation or introduce overload -# ambiguity, so compare every matching public method/constructor signature against -# the stable API baseline. +# ApiCompat primarily protects binary compatibility. Parameter names, positional +# ordering among same-typed parameters, and optional metadata can still break source +# callers. Compare against the released GA signature, then evaluate the complete +# current overload set before reporting optionality differences. if ($BaselineApiFilePath -and - ($ExcludeRules -notcontains 'OPTPARAM001' -or $ExcludeRules -notcontains 'OPTPARAM002')) { + ($ExcludeRules -notcontains 'PARAMNAME001' -or + $ExcludeRules -notcontains 'PARAMORDER001' -or + $ExcludeRules -notcontains 'OPTPARAM001' -or + $ExcludeRules -notcontains 'OPTPARAM002')) { $currentMethods = Get-ApiMethodInfos $lines $baselineMethods = Get-ApiMethodInfos (Get-Content $BaselineApiFilePath) + $baselineLabel = if ($BaselineVersion) { "GA baseline $BaselineVersion" } else { 'GA baseline' } - foreach ($key in $currentMethods.Keys) { - if (-not $baselineMethods.ContainsKey($key)) { + foreach ($key in $baselineMethods.Keys) { + if (-not $currentMethods.ContainsKey($key)) { continue } @@ -428,11 +443,18 @@ if ($BaselineApiFilePath -and $parameterCount = [Math]::Min($currentMethod.Parameters.Count, $baselineMethod.Parameters.Count) $optionalToRequired = [System.Collections.Generic.List[string]]::new() $requiredToOptional = [System.Collections.Generic.List[string]]::new() + $renamedParameters = [System.Collections.Generic.List[string]]::new() + $baselineNames = @($baselineMethod.Parameters | ForEach-Object { $_.Name }) + $currentNames = @($currentMethod.Parameters | ForEach-Object { $_.Name }) for ($parameterIndex = 0; $parameterIndex -lt $parameterCount; $parameterIndex++) { $currentParameter = $currentMethod.Parameters[$parameterIndex] $baselineParameter = $baselineMethod.Parameters[$parameterIndex] + if ($baselineParameter.Name -cne $currentParameter.Name) { + $renamedParameters.Add("'$($baselineParameter.Name)' to '$($currentParameter.Name)'") + } + if ($baselineParameter.IsOptional -and -not $currentParameter.IsOptional -and $ExcludeRules -notcontains 'OPTPARAM001') { @@ -445,13 +467,33 @@ if ($BaselineApiFilePath -and } } + if ($renamedParameters.Count -gt 0) { + $sameNamesDifferentOrder = $baselineNames.Count -eq $currentNames.Count -and + @(Compare-Object ($baselineNames | Sort-Object -CaseSensitive) ($currentNames | Sort-Object -CaseSensitive) -CaseSensitive).Count -eq 0 + $ruleId = if ($sameNamesDifferentOrder) { 'PARAMORDER001' } else { 'PARAMNAME001' } + if ($ExcludeRules -notcontains $ruleId) { + $changeDescription = if ($sameNamesDifferentOrder) { + "Parameter order changed from '$($baselineNames -join ', ')' to '$($currentNames -join ', ')'." + } else { + "Parameter name changes: $($renamedParameters -join ', ')." + } + $violations.Add([NamingViolation]::new( + $ruleId, 'Error', 'Source Compatibility', + $currentMethod.TypeName, $currentMethod.MemberName, + "$changeDescription Named or positional callers compiled against $baselineLabel can bind differently or fail. Baseline signature: $($baselineMethod.Signature). Current signature: $($currentMethod.Signature).", + "Preserve the exact parameter names and ordering from $baselineLabel. Investigate forwarding code separately for runtime-semantic correctness.", + $currentMethod.Line + )) + } + } + if ($optionalToRequired.Count -gt 0) { $parameterNames = ($optionalToRequired | ForEach-Object { "'$_'" }) -join ', ' $violations.Add([NamingViolation]::new( - 'OPTPARAM001', 'Error', 'Source Compatibility', + 'OPTPARAM001', 'Warning', 'Source Compatibility Candidate', $currentMethod.TypeName, $currentMethod.MemberName, - "Parameter(s) $parameterNames changed from optional to required on '$($currentMethod.MemberName)'. ApiCompat does not report this binary-compatible source break.", - "Restore the optional defaults from the stable API baseline.", + "Parameter(s) $parameterNames changed from optional to required relative to $baselineLabel. This textual difference is not a blocking finding until the complete overload sets are compiled with representative GA calls. Baseline signature: $($baselineMethod.Signature). Current signature: $($currentMethod.Signature).", + "Compile positional, named, omitted, and default-literal calls against both GA and current overload sets; block only for a demonstrated source break.", $currentMethod.Line )) } @@ -459,10 +501,10 @@ if ($BaselineApiFilePath -and if ($requiredToOptional.Count -gt 0) { $parameterNames = ($requiredToOptional | ForEach-Object { "'$_'" }) -join ', ' $violations.Add([NamingViolation]::new( - 'OPTPARAM002', 'Error', 'Source Compatibility', + 'OPTPARAM002', 'Warning', 'Source Compatibility Candidate', $currentMethod.TypeName, $currentMethod.MemberName, - "Parameter(s) $parameterNames changed from required to optional on '$($currentMethod.MemberName)'. This can introduce overload ambiguity that ApiCompat does not detect.", - "Remove the default values and preserve the stable API signature.", + "Parameter(s) $parameterNames changed from required to optional relative to $baselineLabel. This textual difference is not a blocking finding until the complete overload sets are compiled with representative GA calls. Baseline signature: $($baselineMethod.Signature). Current signature: $($currentMethod.Signature).", + "Compile positional, named, omitted, and default-literal calls against both GA and current overload sets; block only for demonstrated ambiguity or changed binding.", $currentMethod.Line )) } @@ -1042,6 +1084,10 @@ for ($i = 0; $i -lt $totalLines; $i++) { if ($BaselineApiFilePath -and $baselineLines.Count -gt 0) { $filteredViolations = [System.Collections.Generic.List[NamingViolation]]::new() foreach ($v in $violations) { + if ($v.Category -like 'Source Compatibility*') { + $filteredViolations.Add($v) + continue + } # For type-level violations, check if the type declaration line exists in baseline # For member-level violations, check if the member line exists in baseline $violationLine = $lines[$v.Line - 1].Trim() diff --git a/.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1 b/.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1 new file mode 100644 index 000000000000..1e04518f3782 --- /dev/null +++ b/.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1 @@ -0,0 +1,202 @@ +#!/usr/bin/env pwsh +<# +.SYNOPSIS + Exports the public API surface from a released NuGet assembly. + +.DESCRIPTION + Downloads the exact package version configured by ApiCompatVersion and runs GenAPI + over the released DLL. Package code is never loaded or executed; only assembly + metadata is read. Use the output to verify disputed parameter signatures before + reporting a management SDK compatibility finding. +#> +[CmdletBinding()] +param( + [Parameter(Mandatory = $true)] + [string]$PackageName, + + [Parameter(Mandatory = $true)] + [string]$Version, + + [Parameter(Mandatory = $true)] + [string]$OutputPath, + + [string]$TargetFramework, + + [string]$GenApiVersion = '5.0.0-beta.19552.1' +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +if ($PackageName -notmatch '^[A-Za-z0-9_.-]+$') { + throw "Invalid package name: $PackageName" +} +if ($Version -notmatch '^[0-9A-Za-z.+-]+$') { + throw "Invalid package version: $Version" +} +if ($TargetFramework -and $TargetFramework -notmatch '^[A-Za-z0-9.-]+$') { + throw "Invalid target framework: $TargetFramework" +} + +$tempDirectory = Join-Path ([System.IO.Path]::GetTempPath()) ("ga-api-" + [guid]::NewGuid().ToString('N')) +New-Item -ItemType Directory -Path $tempDirectory | Out-Null + +try { + $packageId = $PackageName.ToLowerInvariant() + $packageVersion = $Version.ToLowerInvariant() + $packageArchive = Join-Path $tempDirectory "$packageId.$packageVersion.nupkg" + $packageDirectory = Join-Path $tempDirectory 'package' + $packageUrl = "https://api.nuget.org/v3-flatcontainer/$packageId/$packageVersion/$packageId.$packageVersion.nupkg" + Invoke-WebRequest -Uri $packageUrl -OutFile $packageArchive + [System.IO.Compression.ZipFile]::ExtractToDirectory($packageArchive, $packageDirectory) + + $libDirectory = Join-Path $packageDirectory 'lib' + if (-not (Test-Path $libDirectory)) { + throw "Released package $PackageName $Version does not contain a lib directory." + } + + if ($TargetFramework) { + $frameworkDirectory = Join-Path $libDirectory $TargetFramework + if (-not (Test-Path $frameworkDirectory)) { + throw "Released package $PackageName $Version does not contain lib/$TargetFramework." + } + } else { + $availableFrameworks = @(Get-ChildItem -Path $libDirectory -Directory) + $preferredFrameworks = @('net10.0', 'net9.0', 'net8.0', 'netstandard2.1', 'netstandard2.0') + $frameworkDirectory = $null + foreach ($preferredFramework in $preferredFrameworks) { + $match = $availableFrameworks | Where-Object { $_.Name -eq $preferredFramework } | Select-Object -First 1 + if ($match) { + $frameworkDirectory = $match.FullName + break + } + } + if (-not $frameworkDirectory) { + $frameworkDirectory = $availableFrameworks | Sort-Object Name | Select-Object -First 1 -ExpandProperty FullName + } + $TargetFramework = Split-Path $frameworkDirectory -Leaf + } + + $assemblyPath = Join-Path $frameworkDirectory "$PackageName.dll" + if (-not (Test-Path $assemblyPath)) { + throw "Released assembly not found: $assemblyPath" + } + + $genApiPackageVersion = $GenApiVersion.ToLowerInvariant() + $nugetPackages = if ($env:NUGET_PACKAGES) { + $env:NUGET_PACKAGES + } else { + Join-Path $HOME '.nuget/packages' + } + $genApiPackageDirectory = Join-Path $nugetPackages "microsoft.dotnet.genapi/$genApiPackageVersion" + if (-not (Test-Path $genApiPackageDirectory)) { + $restoreProject = Join-Path $tempDirectory 'restore.csproj' + @" + + net8.0 + + +"@ | Set-Content -Path $restoreProject + & dotnet restore $restoreProject --source 'https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-net/nuget/v3/index.json' + if ($LASTEXITCODE -ne 0) { + throw "Failed to restore Microsoft.DotNet.GenAPI $GenApiVersion." + } + } + + $genApiDll = Get-ChildItem -Path (Join-Path $genApiPackageDirectory 'tools') -Recurse -Filter 'Microsoft.DotNet.GenAPI.dll' | + Where-Object { $_.FullName -match 'netcoreapp' } | + Sort-Object FullName -Descending | + Select-Object -First 1 -ExpandProperty FullName + if (-not $genApiDll) { + throw "Microsoft.DotNet.GenAPI.dll was not found in package version $GenApiVersion." + } + + $dependencyProject = Join-Path $tempDirectory 'dependencies.csproj' + @" + + $TargetFramework + + +"@ | Set-Content -Path $dependencyProject + & dotnet restore $dependencyProject --source 'https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-net/nuget/v3/index.json' + if ($LASTEXITCODE -ne 0) { + throw "Failed to restore the dependency closure for $PackageName $Version." + } + + $assetsPath = Join-Path $tempDirectory 'obj/project.assets.json' + $assets = Get-Content -Raw $assetsPath | ConvertFrom-Json -Depth 100 + $packageFolder = $assets.packageFolders.PSObject.Properties | + Select-Object -First 1 -ExpandProperty Name + $framework = $assets.project.frameworks.PSObject.Properties | + Where-Object { $_.Value.targetAlias -eq $TargetFramework } | + Select-Object -First 1 + $targetNames = [System.Collections.Generic.List[string]]::new() + $targetNames.Add($TargetFramework) + if ($framework) { + $targetNames.Add($framework.Name) + } + if ($TargetFramework -match '^netstandard(?\d+\.\d+)$') { + $targetNames.Add(".NETStandard,Version=v$($Matches['version'])") + } elseif ($TargetFramework -match '^netcoreapp(?\d+\.\d+)$') { + $targetNames.Add(".NETCoreApp,Version=v$($Matches['version'])") + } + $target = $assets.targets.PSObject.Properties | + Where-Object { + $targetProperty = $_ + @($targetNames | Where-Object { $targetProperty.Name -like "$_*" }).Count -gt 0 + } | + Select-Object -First 1 -ExpandProperty Value + if (-not $target) { + throw "Could not resolve restored target '$TargetFramework' in $assetsPath." + } + $libraryDirectories = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($library in $target.PSObject.Properties) { + foreach ($assetGroupName in @('compile', 'runtime')) { + $assetGroup = $library.Value.$assetGroupName + if (-not $assetGroup) { + continue + } + foreach ($asset in $assetGroup.PSObject.Properties) { + if ($asset.Name.EndsWith('.dll')) { + $assetPath = Join-Path $packageFolder (Join-Path $library.Name.ToLowerInvariant() $asset.Name) + if (Test-Path $assetPath) { + $libraryDirectories.Add((Split-Path $assetPath -Parent)) | Out-Null + } + } + } + } + } + $dotnetExecutable = (Get-Command dotnet).Source + $dotnetTarget = (Get-Item $dotnetExecutable).Target + $dotnetRoot = if ($env:DOTNET_ROOT) { + $env:DOTNET_ROOT + } elseif ($dotnetTarget) { + Split-Path $dotnetTarget -Parent + } else { + Split-Path $dotnetExecutable -Parent + } + foreach ($referenceDirectory in (Get-ChildItem -Path (Join-Path $dotnetRoot 'packs') -Recurse -Directory -Filter $TargetFramework -ErrorAction SilentlyContinue)) { + $libraryDirectories.Add($referenceDirectory.FullName) | Out-Null + } + $libraryDirectories.Add($PSHOME) | Out-Null + + $resolvedOutputPath = [System.IO.Path]::GetFullPath($OutputPath) + $outputDirectory = Split-Path $resolvedOutputPath -Parent + New-Item -ItemType Directory -Force -Path $outputDirectory | Out-Null + + $genApiOutput = & dotnet $genApiDll $assemblyPath --api-only --lib-path ($libraryDirectories -join ';') --out $resolvedOutputPath 2>&1 + $genApiOutput | Write-Host + if ($LASTEXITCODE -ne 0 -or -not (Test-Path $resolvedOutputPath) -or + (Get-Item $resolvedOutputPath).Length -eq 0) { + throw "GenAPI failed to export $PackageName $Version." + } + if ($genApiOutput -match 'Unable to resolve assembly') { + throw "GenAPI could not resolve the full dependency closure for $PackageName $Version." + } + + Write-Host "Exported $PackageName $Version ($([System.IO.Path]::GetFileName($frameworkDirectory))) to $resolvedOutputPath" +} finally { + if (Test-Path $tempDirectory) { + Remove-Item -Recurse -Force $tempDirectory + } +} diff --git a/.github/skills/azure-sdk-mgmt-pr-review/SKILL.md b/.github/skills/azure-sdk-mgmt-pr-review/SKILL.md index abd3c4444a83..73d7b635b5b3 100644 --- a/.github/skills/azure-sdk-mgmt-pr-review/SKILL.md +++ b/.github/skills/azure-sdk-mgmt-pr-review/SKILL.md @@ -25,20 +25,19 @@ Review only new or changed public API relative to the latest stable release. Exi ### 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. Diff released API against the PR API file. Review only added/modified types, members, and enums. +2. If present, use the public assembly from the released NuGet package at exactly `ApiCompatVersion` as the authoritative GA contract. Inspect metadata only; do not execute package code. Use `Export-GaApiBaseline.ps1 -PackageName -Version -TargetFramework -OutputPath ` to produce a readable metadata listing. Repository history and current `main` are context, never evidence that an API shipped. +3. 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. Use it as the scanner input and readable projection of the assembly, but resolve any discrepancy in favor of released assembly metadata. +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 + pwsh .github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 -ApiFilePath -BaselineApiFilePath -BaselineVersion ``` Omit `-BaselineApiFilePath` 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. - When a baseline is supplied, the scanner also compares required/optional parameter metadata for every matching public method and constructor. Investigate every `OPTPARAM001` and `OPTPARAM002` finding as a potential blocking source-compatibility issue; ApiCompat may not report them. - - Before requesting restoration for `OPTPARAM001`, inspect the complete overload set and verify the change compiles for representative positional and named calls. A compatibility shim may need to keep parameters required when restoring defaults would introduce `CS0121`; the text scanner intentionally reports these cases rather than approximating the C# overload-resolution binder. Neither `[Obsolete]` nor `[EditorBrowsable]` changes overload resolution, and `[OverloadResolutionPriority]` only helps consumers compiling with C# 13 or later. + When a baseline is supplied, the scanner compares GA parameter names, same-typed positional ordering, and required/optional metadata. `PARAMNAME001` and `PARAMORDER001` identify exact signature differences. `OPTPARAM001` and `OPTPARAM002` are textual candidates only; the scanner intentionally does not approximate the C# overload-resolution binder. 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 @@ -47,7 +46,14 @@ Review only new or changed public API relative to the latest stable release. Exi 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`, `OPTPARAM002`, `SUFFIX001`-`SUFFIX010`, `RESINFIX001`, `RESNAME001`, `ACRONYM001`, `ACRONYM002`, `ARMCOMMON001`, `BOOL001`, `DATETIME001`, and `TTL001`. Contextual naming is intentionally manual; the scanner only provides the bounded worklist. +Scanner rule families include `PARAMNAME001`, `PARAMORDER001`, `OPTPARAM001`, `OPTPARAM002`, `SUFFIX001`-`SUFFIX010`, `RESINFIX001`, `RESNAME001`, `ACRONYM001`, `ACRONYM002`, `ARMCOMMON001`, `BOOL001`, `DATETIME001`, and `TTL001`. Contextual naming is intentionally manual; the scanner only provides the bounded worklist. + +Parameter compatibility: +- Treat the `ApiCompatVersion` assembly as authoritative for parameter names, ordering, types, and optionality. Do not substitute the previous repository source shape. +- Inspect every overload with the same containing type and member name. Compile representative GA calls against synthesized declarations for both the GA and current overload sets: required arguments supplied positionally and by name, omitted optional arguments, positional prefixes, combinations of named arguments, `default`, and explicitly typed defaults where overload types differ. +- A textual optionality difference is not blocking when another overload preserves every GA call shape. Conversely, report a concrete call that no longer compiles, becomes ambiguous, or binds to a behaviorally incompatible type. +- Keep signature and runtime-semantic analysis separate. A shim can be source-compatible but forward an argument to the wrong generated parameter; report that as a forwarding bug, not as a fabricated GA signature difference. +- Every compatibility finding must state the `ApiCompatVersion`, the exact GA signature, the current signature, and a representative broken call. Do not say an API was previously shipped unless it is present in released assembly metadata. ### Comment Targets @@ -155,7 +161,7 @@ If `ApiCompatVersion` exists, check breaking changes after Phase 2. Locally, bui 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 source compatibility. Before declaring this phase complete, investigate every `OPTPARAM001` and `OPTPARAM002` finding against the complete overload set. Do not infer that a previously reviewed overload covers its siblings; compare every matching signature against the stable baseline. +ApiCompat passing is not sufficient for source compatibility. Before declaring this phase complete, investigate every `PARAMNAME001`, `PARAMORDER001`, `OPTPARAM001`, and `OPTPARAM002` result against the complete GA and current overload sets and released assembly metadata. `OPTPARAM001` and `OPTPARAM002` remain non-blocking candidates unless a compiler probe demonstrates a source break. Do not infer that a previously reviewed overload covers its siblings. ## Finding Severity @@ -163,8 +169,8 @@ Report every finding and recommend resolving it in the current PR. Do not defer | 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`; required/optional parameter compatibility findings; unmitigated breaking changes; manual generated-code edits; and migration-specific violations | `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` | +| Blocking | Phase 1 versioning violations; deterministic scanner findings other than advisory `TYPE001`, `TYPE003`, and unverified `OPTPARAM001`/`OPTPARAM002` candidates; all contextual naming findings; naming, suffix, acronym, resource-name, and ARM common-type violations; `TSPRENAME001`; demonstrated parameter name/order/optionality compatibility breaks; unmitigated breaking changes; manual generated-code edits; and migration-specific violations | `REQUEST_CHANGES` | +| Non-blocking | Unverified `OPTPARAM001`/`OPTPARAM002` candidates; 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. diff --git a/.github/skills/azure-sdk-mgmt-pr-review/test/Check-MgmtNamingRules.tests.ps1 b/.github/skills/azure-sdk-mgmt-pr-review/test/Check-MgmtNamingRules.tests.ps1 new file mode 100644 index 000000000000..6d1b3cebf722 --- /dev/null +++ b/.github/skills/azure-sdk-mgmt-pr-review/test/Check-MgmtNamingRules.tests.ps1 @@ -0,0 +1,103 @@ +#!/usr/bin/env pwsh +[CmdletBinding()] +param( + [string]$ScannerPath = (Join-Path $PSScriptRoot '..' 'Check-MgmtNamingRules.ps1') +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$ScannerPath = (Resolve-Path $ScannerPath).Path +$failures = [System.Collections.Generic.List[string]]::new() + +function Assert([bool]$condition, [string]$message) { + if ($condition) { + Write-Host " [PASS] $message" + } else { + Write-Host " [FAIL] $message" -ForegroundColor Red + $script:failures.Add($message) + } +} + +function Invoke-Scanner([string[]]$baselineMembers, [string[]]$currentMembers) { + $caseDirectory = Join-Path $script:tempRoot ([guid]::NewGuid().ToString('N')) + New-Item -ItemType Directory -Path $caseDirectory | Out-Null + + $baselinePath = Join-Path $caseDirectory 'baseline.cs' + $currentPath = Join-Path $caseDirectory 'current.cs' + $prefix = @( + 'namespace Azure.ResourceManager.Example', + '{', + ' public partial class ExampleClient', + ' {' + ) + $suffix = @( + ' }', + '}' + ) + + Set-Content -Path $baselinePath -Value ($prefix + $baselineMembers + $suffix) + Set-Content -Path $currentPath -Value ($prefix + $currentMembers + $suffix) + + return (& pwsh -NoLogo -NoProfile -File $ScannerPath ` + -ApiFilePath $currentPath ` + -BaselineApiFilePath $baselinePath ` + -BaselineVersion '1.2.3' 6>&1 | Out-String) +} + +$tempRoot = Join-Path ([System.IO.Path]::GetTempPath()) ("mgmt-review-tests-" + [guid]::NewGuid().ToString('N')) +New-Item -ItemType Directory -Path $tempRoot | Out-Null + +try { + Write-Host 'Case: exact GA signature' + $output = Invoke-Scanner ` + @(' public ExampleClient(long count, string mode, string name, string vmSkuName) { }') ` + @(' public ExampleClient(long count, string mode, string name, string vmSkuName) { }') + Assert ($output -notmatch '\[PARAM(?:NAME|ORDER)001\]') 'does not flag a signature that exactly matches GA' + + Write-Host 'Case: same-typed parameters reordered' + $output = Invoke-Scanner ` + @(' public ExampleClient(long count, string mode, string vmSkuName, string name) { }') ` + @(' public ExampleClient(long count, string mode, string name, string vmSkuName) { }') + Assert ($output -match '\[PARAMORDER001\]') 'flags positional order changes with identical parameter types' + Assert ($output -match 'GA baseline 1\.2\.3') 'reports the compared GA version' + Assert ($output -match 'ExampleClient\(long count, string mode, string vmSkuName, string name\)') 'reports the exact GA signature' + + Write-Host 'Case: named parameter changed' + $output = Invoke-Scanner ` + @(' public void GetAll(string kind = null) { }') ` + @(' public void GetAll(string skip = null) { }') + Assert ($output -match '\[PARAMNAME001\]') 'flags a named-argument compatibility change' + Assert ($output -match "'kind'.*'skip'") 'reports GA and current parameter names' + + Write-Host 'Case: case-only named parameter change' + $output = Invoke-Scanner ` + @(' public void Get(string resourceId) { }') ` + @(' public void Get(string resourceID) { }') + Assert ($output -match '\[PARAMNAME001\]') 'treats C# named arguments as case-sensitive' + + Write-Host 'Case: optional parameter became required' + $output = Invoke-Scanner ` + @(' public void GetAll(string filter = null, System.Threading.CancellationToken cancellationToken = default) { }') ` + @(' public void GetAll(string filter, System.Threading.CancellationToken cancellationToken) { }') + Assert ($output -match '\[OPTPARAM001\]') 'emits an optional-to-required candidate' + Assert ($output -match 'Source Compatibility Candidate') 'labels textual optionality differences as candidates' + Assert ($output -match 'Baseline signature: GetAll\(string filter = null, System\.Threading\.CancellationToken cancellationToken = default\)') 'includes the exact GA signature' + Assert ($output -match 'not a blocking finding until the complete overload sets are compiled') 'requires compiler confirmation before blocking' + + Write-Host 'Case: required parameter became optional' + $output = Invoke-Scanner ` + @(' public void Create(string name) { }') ` + @(' public void Create(string name = null) { }') + Assert ($output -match '\[OPTPARAM002\]') 'emits a required-to-optional candidate' + Assert ($output -match 'block only for demonstrated ambiguity or changed binding') 'requires a demonstrated binding break' +} finally { + Remove-Item -Recurse -Force $tempRoot +} + +if ($failures.Count -gt 0) { + Write-Host "`n$($failures.Count) assertion(s) failed." -ForegroundColor Red + exit 1 +} + +Write-Host "`nAll management review scanner tests passed." -ForegroundColor Green diff --git a/.github/workflows/mgmt-review.lock.yml b/.github/workflows/mgmt-review.lock.yml index 8950df5bb14b..a93469fcccd5 100644 --- a/.github/workflows/mgmt-review.lock.yml +++ b/.github/workflows/mgmt-review.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"9f26d58a1f50b6b3195db9e0cf47aa9c4b3acb4e4451aac4702e6b579c2255a8","body_hash":"322b173b898b6841b943a39a35e84536f99606027821116f38990b0e35a4ad21","compiler_version":"v0.81.6","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.65"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b6939c4770c90b517a957ea38dfe224a10010b1e770ba5e01434e4d746217c88","body_hash":"8a687088f3631e5ec198a43ae20a52785a493a336b10c36ce9cbd53a541d40c0","compiler_version":"v0.81.6","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.65"}} # gh-aw-manifest: {"version":1,"secrets":["GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/cache/save","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"ba6380cc6e5be5d21677bebe04d52fb48e3abec7","version":"v0.81.6"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.11","digest":"sha256:979723c628182da7729333f2208bb249fd25ddee579645cf9a3892d681a929c7","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.11@sha256:979723c628182da7729333f2208bb249fd25ddee579645cf9a3892d681a929c7"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.11","digest":"sha256:807e4831999b44513b0a66e5859d478dc4da7ae74ab1918cec967d513f95bf9d","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.11@sha256:807e4831999b44513b0a66e5859d478dc4da7ae74ab1918cec967d513f95bf9d"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.11","digest":"sha256:ff27ea0525ad953a6adee28a5fbe9d2e22be47dbec755c15767af4ea3f91df7d","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.11@sha256:ff27ea0525ad953a6adee28a5fbe9d2e22be47dbec755c15767af4ea3f91df7d"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.30","digest":"sha256:35625d1a2269b1238606078c879f59a91cffc4ac33eb54bf39c6418822c1a8be","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.3.30@sha256:35625d1a2269b1238606078c879f59a91cffc4ac33eb54bf39c6418822c1a8be"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.4.0","digest":"sha256:2afb26356481d1a350e14544a6e160f7f7ec1561a1ea309b823665abf0309036","pinned_image":"ghcr.io/github/github-mcp-server:v1.4.0@sha256:2afb26356481d1a350e14544a6e160f7f7ec1561a1ea309b823665abf0309036"}]} # This file was automatically generated by gh-aw (v0.81.6). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # @@ -133,7 +133,7 @@ jobs: GH_AW_INFO_EXPERIMENTAL: "false" GH_AW_INFO_SUPPORTS_TOOLS_ALLOWLIST: "true" GH_AW_INFO_STAGED: "false" - GH_AW_INFO_ALLOWED_DOMAINS: '["defaults","dev.azure.com","dotnet","github"]' + GH_AW_INFO_ALLOWED_DOMAINS: '["api.nuget.org","defaults","dev.azure.com","dotnet","github","pkgs.dev.azure.com"]' GH_AW_INFO_FIREWALL_ENABLED: "true" GH_AW_INFO_AWF_VERSION: "v0.27.11" GH_AW_INFO_AWMG_VERSION: "" @@ -253,20 +253,20 @@ jobs: run: | bash "${RUNNER_TEMP}/gh-aw/actions/create_prompt_first.sh" { - cat << 'GH_AW_PROMPT_49c3cac599e6f1ab_EOF' + cat << 'GH_AW_PROMPT_83bf1b15cc134f7a_EOF' - GH_AW_PROMPT_49c3cac599e6f1ab_EOF + GH_AW_PROMPT_83bf1b15cc134f7a_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/xpia.md" cat "${RUNNER_TEMP}/gh-aw/prompts/temp_folder_prompt.md" cat "${RUNNER_TEMP}/gh-aw/prompts/markdown.md" cat "${RUNNER_TEMP}/gh-aw/prompts/safe_outputs_prompt.md" - cat << 'GH_AW_PROMPT_49c3cac599e6f1ab_EOF' + cat << 'GH_AW_PROMPT_83bf1b15cc134f7a_EOF' Tools: add_comment, create_pull_request_review_comment(max:100), submit_pull_request_review, missing_tool, missing_data, noop, dismiss_stale_change_requests, publish_pr_check - GH_AW_PROMPT_49c3cac599e6f1ab_EOF + GH_AW_PROMPT_83bf1b15cc134f7a_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/mcp_cli_tools_prompt.md" - cat << 'GH_AW_PROMPT_49c3cac599e6f1ab_EOF' + cat << 'GH_AW_PROMPT_83bf1b15cc134f7a_EOF' The following GitHub context information is available for this workflow: {{#if github.actor}} @@ -308,9 +308,9 @@ jobs: stop immediately and report the limitation rather than spending turns trying to work around it. - GH_AW_PROMPT_49c3cac599e6f1ab_EOF + GH_AW_PROMPT_83bf1b15cc134f7a_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/github_mcp_tools_with_safeoutputs_prompt.md" - cat << 'GH_AW_PROMPT_49c3cac599e6f1ab_EOF' + cat << 'GH_AW_PROMPT_83bf1b15cc134f7a_EOF' # Azure .NET Management SDK PR Review @@ -386,7 +386,8 @@ jobs: 1. Identify the package root, `.csproj`, `CHANGELOG.md`, API surface files under `api/`, generated files under `src/Generated/`, customization files under `src/Custom*/`, `src/Customization*/`, or `src/Customized*/`, and TypeSpec customization files such as `client.tsp` and `tspconfig.yaml`. 2. Determine whether this is a migration PR. Use the migration skill when the PR title or files indicate Swagger/AutoRest to TypeSpec migration, such as adding `tsp-location.yaml`, deleting `src/autorest.md`, adding TypeSpec `metadata.json`, or broadly regenerating `src/Generated/`. 3. Determine whether the package is TypeSpec-backed by checking for `tsp-location.yaml`. For every TypeSpec-backed package, inspect added or modified SDK customization files for rename-only `[CodeGenType]`, `[CodeGenMember]`, `[CodeGenSuppress]`, wrappers, or forwarding methods, even when the PR is not a migration. - 4. Determine the latest released stable API baseline from `ApiCompatVersion` in the package `.csproj` when present. Fetch the corresponding tagged API file by tag name `_`. + 4. Determine the latest released stable API baseline from `ApiCompatVersion` in the package `.csproj` when present. Run the trusted base-branch `.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1` script for exactly that package version and the baseline target framework. It downloads the released NuGet package and reads public assembly metadata without executing package code. This assembly is authoritative for whether an API shipped and for parameter names, ordering, types, and optionality. + 5. Fetch the corresponding tagged API file by tag name `_`. Use it as the deterministic scanner input and readable projection of the assembly. If repository history, current `main`, or the tagged API file conflicts with released assembly metadata, use the assembly metadata. ## Step 2 - Run deterministic checks @@ -399,12 +400,22 @@ jobs: If a baseline API file is available, pass it too: ```powershell - pwsh .github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 -ApiFilePath -BaselineApiFilePath + pwsh .github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 -ApiFilePath -BaselineApiFilePath -BaselineVersion ``` Use only the scanner script fetched from the base branch and API surface files fetched from the PR head and baseline tag into temporary files. Do not run the scanner over a PR checkout. - When a baseline is available, the scanner deterministically compares required/optional parameter metadata for every matching public method and constructor. Treat every `OPTPARAM001` or `OPTPARAM002` result as blocking. Do not rely on ApiCompat or on checking only overloads named in prior review comments: ApiCompat can pass while optional metadata changes break source compilation or create ambiguity between sibling overloads. + When a baseline is available, the scanner deterministically compares parameter names, same-typed positional ordering, and required/optional metadata against GA. Treat `OPTPARAM001` and `OPTPARAM002` as textual candidates only; the scanner does not approximate the C# overload-resolution binder. + + For every `PARAMNAME001`, `PARAMORDER001`, `OPTPARAM001`, or `OPTPARAM002` result: + + 1. Confirm the exact signature in the released `ApiCompatVersion` assembly. Never replace it with a signature inferred from previous repository source. + 2. Inspect all current overloads with the same containing type and member name. + 3. Build a minimal compiler probe from synthesized declarations for the complete GA and current overload sets. Check required arguments supplied positionally and by name, omitted optional arguments, positional prefixes, combinations of named arguments, `default`, and explicitly typed defaults. A textual optionality difference alone is not blocking. + 4. Report the baseline version, exact GA signature, current signature, and a concrete broken or ambiguous call. Do not claim an API was previously shipped unless it exists in released assembly metadata. + 5. Analyze forwarding/runtime semantics separately. If a compatibility overload forwards `skip` into `kind`, report the incorrect delegation even when its public signature matches GA. + + Do not compile or execute PR code. Compile only the minimal synthesized declarations and call sites. Keep every unproven `OPTPARAM001` or `OPTPARAM002` candidate non-blocking. ## Step 3 - Apply the skill review Apply all relevant phases from the skill files, with these workflow-specific adjustments: @@ -412,7 +423,7 @@ jobs: 1. Phase 1 versioning findings are blocking, but do **not** stop after Phase 1 — continue into Phase 2 and submit one combined review so versioning and API/naming findings reach the author in the same round (per the updated Phase 1 in the skill). 2. Phase 2 API review findings should focus on new or changed public API surface only. 3. **Contextual naming must be exhaustive.** Use the scanner's `-ListNewTypes` inventory mode to enumerate every new public type, then record a verdict for each one in a single pass (see Phase 2 step 4 in the skill). Surfacing only a subset of naming issues per round is the main cause of repeated review rounds and must be avoided. - 4. Phase 3 breaking-change detection must use the CI failure details fetched in Step 0, API diffs, and every source-compatibility result from Step 2. Do not run `dotnet build` in this workflow because that would execute untrusted PR code. If CI reports ApiCompat failures or build errors, surface them with links to the failed check run URL or Azure DevOps target URL. A passing ApiCompat result does not override `OPTPARAM001` or `OPTPARAM002`. + 4. Phase 3 breaking-change detection must use the CI failure details fetched in Step 0, released assembly metadata, API diffs, and every demonstrated source-compatibility result from Step 2. Do not run `dotnet build` in this workflow because that would execute untrusted PR code. If CI reports ApiCompat failures or build errors, surface them with links to the failed check run URL or Azure DevOps target URL. A passing ApiCompat result does not override a demonstrated source break, but an unverified textual parameter difference is not automatically blocking. 5. For every TypeSpec-backed package, apply the base skill's `TSPRENAME001` rule. A rename-only SDK customization for a directly targetable TypeSpec API is blocking and must be replaced with scoped `@@clientName(TypeSpecTarget, "CSharpName", "csharp")` in the spec repository's `client.tsp`, followed by regeneration. 6. For migration PRs, apply Phases 4 and 5 from the migration skill. Treat manual edits to `src/Generated/` as blocking unless there is clear evidence they are generated output rather than hand edits. @@ -480,7 +491,7 @@ jobs: 5. After the spec PR merges, update `tsp-location.yaml` to the latest `main` commit in `azure-rest-api-specs` that contains the merged changes, then regenerate the SDK. ``` - GH_AW_PROMPT_49c3cac599e6f1ab_EOF + GH_AW_PROMPT_83bf1b15cc134f7a_EOF } > "$GH_AW_PROMPT" - name: Interpolate variables and render templates uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 @@ -2008,4 +2019,3 @@ jobs: /tmp/gh-aw/safe-output-items.jsonl /tmp/gh-aw/temporary-id-map.json if-no-files-found: ignore - diff --git a/.github/workflows/mgmt-review.md b/.github/workflows/mgmt-review.md index cc0a31075a46..3a877a2bf534 100644 --- a/.github/workflows/mgmt-review.md +++ b/.github/workflows/mgmt-review.md @@ -38,10 +38,12 @@ engine: queue: max network: allowed: + - api.nuget.org - defaults - dev.azure.com - dotnet - github + - pkgs.dev.azure.com safe-outputs: report-failure-as-issue: false add-comment: @@ -288,7 +290,8 @@ For each changed management SDK package: 1. Identify the package root, `.csproj`, `CHANGELOG.md`, API surface files under `api/`, generated files under `src/Generated/`, customization files under `src/Custom*/`, `src/Customization*/`, or `src/Customized*/`, and TypeSpec customization files such as `client.tsp` and `tspconfig.yaml`. 2. Determine whether this is a migration PR. Use the migration skill when the PR title or files indicate Swagger/AutoRest to TypeSpec migration, such as adding `tsp-location.yaml`, deleting `src/autorest.md`, adding TypeSpec `metadata.json`, or broadly regenerating `src/Generated/`. 3. Determine whether the package is TypeSpec-backed by checking for `tsp-location.yaml`. For every TypeSpec-backed package, inspect added or modified SDK customization files for rename-only `[CodeGenType]`, `[CodeGenMember]`, `[CodeGenSuppress]`, wrappers, or forwarding methods, even when the PR is not a migration. -4. Determine the latest released stable API baseline from `ApiCompatVersion` in the package `.csproj` when present. Fetch the corresponding tagged API file by tag name `_`. +4. Determine the latest released stable API baseline from `ApiCompatVersion` in the package `.csproj` when present. Run the trusted base-branch `.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1` script for exactly that package version and the baseline target framework. It downloads the released NuGet package and reads public assembly metadata without executing package code. This assembly is authoritative for whether an API shipped and for parameter names, ordering, types, and optionality. +5. Fetch the corresponding tagged API file by tag name `_`. Use it as the deterministic scanner input and readable projection of the assembly. If repository history, current `main`, or the tagged API file conflicts with released assembly metadata, use the assembly metadata. ## Step 2 - Run deterministic checks @@ -301,12 +304,22 @@ pwsh .github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 -ApiFileP If a baseline API file is available, pass it too: ```powershell -pwsh .github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 -ApiFilePath -BaselineApiFilePath +pwsh .github/skills/azure-sdk-mgmt-pr-review/Check-MgmtNamingRules.ps1 -ApiFilePath -BaselineApiFilePath -BaselineVersion ``` Use only the scanner script fetched from the base branch and API surface files fetched from the PR head and baseline tag into temporary files. Do not run the scanner over a PR checkout. -When a baseline is available, the scanner deterministically compares required/optional parameter metadata for every matching public method and constructor. Treat every `OPTPARAM001` or `OPTPARAM002` result as blocking. Do not rely on ApiCompat or on checking only overloads named in prior review comments: ApiCompat can pass while optional metadata changes break source compilation or create ambiguity between sibling overloads. +When a baseline is available, the scanner deterministically compares parameter names, same-typed positional ordering, and required/optional metadata against GA. Treat `OPTPARAM001` and `OPTPARAM002` as textual candidates only; the scanner does not approximate the C# overload-resolution binder. + +For every `PARAMNAME001`, `PARAMORDER001`, `OPTPARAM001`, or `OPTPARAM002` result: + +1. Confirm the exact signature in the released `ApiCompatVersion` assembly. Never replace it with a signature inferred from previous repository source. +2. Inspect all current overloads with the same containing type and member name. +3. Build a minimal compiler probe from synthesized declarations for the complete GA and current overload sets. Check required arguments supplied positionally and by name, omitted optional arguments, positional prefixes, combinations of named arguments, `default`, and explicitly typed defaults. A textual optionality difference alone is not blocking. +4. Report the baseline version, exact GA signature, current signature, and a concrete broken or ambiguous call. Do not claim an API was previously shipped unless it exists in released assembly metadata. +5. Analyze forwarding/runtime semantics separately. If a compatibility overload forwards `skip` into `kind`, report the incorrect delegation even when its public signature matches GA. + +Do not compile or execute PR code. Compile only the minimal synthesized declarations and call sites. Keep every unproven `OPTPARAM001` or `OPTPARAM002` candidate non-blocking. ## Step 3 - Apply the skill review Apply all relevant phases from the skill files, with these workflow-specific adjustments: @@ -314,7 +327,7 @@ Apply all relevant phases from the skill files, with these workflow-specific adj 1. Phase 1 versioning findings are blocking, but do **not** stop after Phase 1 — continue into Phase 2 and submit one combined review so versioning and API/naming findings reach the author in the same round (per the updated Phase 1 in the skill). 2. Phase 2 API review findings should focus on new or changed public API surface only. 3. **Contextual naming must be exhaustive.** Use the scanner's `-ListNewTypes` inventory mode to enumerate every new public type, then record a verdict for each one in a single pass (see Phase 2 step 4 in the skill). Surfacing only a subset of naming issues per round is the main cause of repeated review rounds and must be avoided. -4. Phase 3 breaking-change detection must use the CI failure details fetched in Step 0, API diffs, and every source-compatibility result from Step 2. Do not run `dotnet build` in this workflow because that would execute untrusted PR code. If CI reports ApiCompat failures or build errors, surface them with links to the failed check run URL or Azure DevOps target URL. A passing ApiCompat result does not override `OPTPARAM001` or `OPTPARAM002`. +4. Phase 3 breaking-change detection must use the CI failure details fetched in Step 0, released assembly metadata, API diffs, and every demonstrated source-compatibility result from Step 2. Do not run `dotnet build` in this workflow because that would execute untrusted PR code. If CI reports ApiCompat failures or build errors, surface them with links to the failed check run URL or Azure DevOps target URL. A passing ApiCompat result does not override a demonstrated source break, but an unverified textual parameter difference is not automatically blocking. 5. For every TypeSpec-backed package, apply the base skill's `TSPRENAME001` rule. A rename-only SDK customization for a directly targetable TypeSpec API is blocking and must be replaced with scoped `@@clientName(TypeSpecTarget, "CSharpName", "csharp")` in the spec repository's `client.tsp`, followed by regeneration. 6. For migration PRs, apply Phases 4 and 5 from the migration skill. Treat manual edits to `src/Generated/` as blocking unless there is clear evidence they are generated output rather than hand edits. From 292a11306bb970270bc5a96437be05157f4a4806 Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Thu, 13 Aug 2026 05:13:21 +0000 Subject: [PATCH 2/3] Optimize GA baseline dependency resolution Document the repository-approved Azure SDK feed and avoid recursively scanning the full .NET reference-pack tree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Export-GaApiBaseline.ps1 | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1 b/.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1 index 1e04518f3782..5598408550df 100644 --- a/.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1 +++ b/.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1 @@ -40,6 +40,7 @@ if ($TargetFramework -and $TargetFramework -notmatch '^[A-Za-z0-9.-]+$') { $tempDirectory = Join-Path ([System.IO.Path]::GetTempPath()) ("ga-api-" + [guid]::NewGuid().ToString('N')) New-Item -ItemType Directory -Path $tempDirectory | Out-Null +$azureSdkFeed = 'https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-net/nuget/v3/index.json' try { $packageId = $PackageName.ToLowerInvariant() @@ -97,7 +98,8 @@ try { "@ | Set-Content -Path $restoreProject - & dotnet restore $restoreProject --source 'https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-net/nuget/v3/index.json' + # This repository's approved feed has NuGet.org configured as an upstream source. + & dotnet restore $restoreProject --source $azureSdkFeed if ($LASTEXITCODE -ne 0) { throw "Failed to restore Microsoft.DotNet.GenAPI $GenApiVersion." } @@ -118,7 +120,7 @@ try { "@ | Set-Content -Path $dependencyProject - & dotnet restore $dependencyProject --source 'https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-net/nuget/v3/index.json' + & dotnet restore $dependencyProject --source $azureSdkFeed if ($LASTEXITCODE -ne 0) { throw "Failed to restore the dependency closure for $PackageName $Version." } @@ -175,8 +177,16 @@ try { } else { Split-Path $dotnetExecutable -Parent } - foreach ($referenceDirectory in (Get-ChildItem -Path (Join-Path $dotnetRoot 'packs') -Recurse -Directory -Filter $TargetFramework -ErrorAction SilentlyContinue)) { - $libraryDirectories.Add($referenceDirectory.FullName) | Out-Null + $packsDirectory = Join-Path $dotnetRoot 'packs' + foreach ($packDirectory in (Get-ChildItem -Path $packsDirectory -Directory -ErrorAction SilentlyContinue)) { + $referenceDirectory = Get-ChildItem -Path $packDirectory.FullName -Directory | + Sort-Object { [version]$_.Name } -Descending | + ForEach-Object { Join-Path $_.FullName "ref/$TargetFramework" } | + Where-Object { Test-Path $_ } | + Select-Object -First 1 + if ($referenceDirectory) { + $libraryDirectories.Add($referenceDirectory) | Out-Null + } } $libraryDirectories.Add($PSHOME) | Out-Null From c431618d3eece6142d8371538536ecb6ee309349 Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Thu, 13 Aug 2026 05:24:11 +0000 Subject: [PATCH 3/3] Use GA metadata export only for parameter candidates Rely on CI ApiCompat and tagged API files by default, and download released assembly metadata only when a parameter compatibility result needs authoritative confirmation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../skills/azure-sdk-mgmt-pr-review/SKILL.md | 5 ++-- .github/workflows/mgmt-review.lock.yml | 26 +++++++++---------- .github/workflows/mgmt-review.md | 8 +++--- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/.github/skills/azure-sdk-mgmt-pr-review/SKILL.md b/.github/skills/azure-sdk-mgmt-pr-review/SKILL.md index 73d7b635b5b3..64859832d4ce 100644 --- a/.github/skills/azure-sdk-mgmt-pr-review/SKILL.md +++ b/.github/skills/azure-sdk-mgmt-pr-review/SKILL.md @@ -25,8 +25,8 @@ Review only new or changed public API relative to the latest stable release. Exi ### 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, use the public assembly from the released NuGet package at exactly `ApiCompatVersion` as the authoritative GA contract. Inspect metadata only; do not execute package code. Use `Export-GaApiBaseline.ps1 -PackageName -Version -TargetFramework -OutputPath ` to produce a readable metadata listing. Repository history and current `main` are context, never evidence that an API shipped. -3. 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. Use it as the scanner input and readable projection of the assembly, but resolve any discrepancy in favor of released assembly metadata. +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. Use it as the scanner baseline. +3. Use CI ApiCompat results as the primary binary-compatibility and parameter-name signal. Export released assembly metadata only when a `PARAMNAME001`, `PARAMORDER001`, `OPTPARAM001`, or `OPTPARAM002` result needs authoritative confirmation before reporting. Use `Export-GaApiBaseline.ps1 -PackageName -Version -TargetFramework -OutputPath `. Repository history and current `main` are context, never evidence that an API shipped. 4. Diff released API against the PR API file. Review only added/modified types, members, and enums. ### Workflow @@ -50,6 +50,7 @@ Scanner rule families include `PARAMNAME001`, `PARAMORDER001`, `OPTPARAM001`, `O Parameter compatibility: - Treat the `ApiCompatVersion` assembly as authoritative for parameter names, ordering, types, and optionality. Do not substitute the previous repository source shape. +- Do not export the assembly when there are no parameter-compatibility candidates. The tagged API file and CI ApiCompat results are sufficient for normal review scope and binary compatibility. - Inspect every overload with the same containing type and member name. Compile representative GA calls against synthesized declarations for both the GA and current overload sets: required arguments supplied positionally and by name, omitted optional arguments, positional prefixes, combinations of named arguments, `default`, and explicitly typed defaults where overload types differ. - A textual optionality difference is not blocking when another overload preserves every GA call shape. Conversely, report a concrete call that no longer compiles, becomes ambiguous, or binds to a behaviorally incompatible type. - Keep signature and runtime-semantic analysis separate. A shim can be source-compatible but forward an argument to the wrong generated parameter; report that as a forwarding bug, not as a fabricated GA signature difference. diff --git a/.github/workflows/mgmt-review.lock.yml b/.github/workflows/mgmt-review.lock.yml index a93469fcccd5..7dddb4a05a7b 100644 --- a/.github/workflows/mgmt-review.lock.yml +++ b/.github/workflows/mgmt-review.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b6939c4770c90b517a957ea38dfe224a10010b1e770ba5e01434e4d746217c88","body_hash":"8a687088f3631e5ec198a43ae20a52785a493a336b10c36ce9cbd53a541d40c0","compiler_version":"v0.81.6","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.65"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"ac1f2736202c14813ce11a96b5378637bbf75e6d9306f70725af0a5a9d985115","body_hash":"50fb4c5bf5bc4e04fc5b2b50bd72b0465968adc17d4db86f396b4ad44eef0e23","compiler_version":"v0.81.6","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.65"}} # gh-aw-manifest: {"version":1,"secrets":["GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/cache/save","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"ba6380cc6e5be5d21677bebe04d52fb48e3abec7","version":"v0.81.6"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.11","digest":"sha256:979723c628182da7729333f2208bb249fd25ddee579645cf9a3892d681a929c7","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.11@sha256:979723c628182da7729333f2208bb249fd25ddee579645cf9a3892d681a929c7"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.11","digest":"sha256:807e4831999b44513b0a66e5859d478dc4da7ae74ab1918cec967d513f95bf9d","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.11@sha256:807e4831999b44513b0a66e5859d478dc4da7ae74ab1918cec967d513f95bf9d"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.11","digest":"sha256:ff27ea0525ad953a6adee28a5fbe9d2e22be47dbec755c15767af4ea3f91df7d","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.11@sha256:ff27ea0525ad953a6adee28a5fbe9d2e22be47dbec755c15767af4ea3f91df7d"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.30","digest":"sha256:35625d1a2269b1238606078c879f59a91cffc4ac33eb54bf39c6418822c1a8be","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.3.30@sha256:35625d1a2269b1238606078c879f59a91cffc4ac33eb54bf39c6418822c1a8be"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.4.0","digest":"sha256:2afb26356481d1a350e14544a6e160f7f7ec1561a1ea309b823665abf0309036","pinned_image":"ghcr.io/github/github-mcp-server:v1.4.0@sha256:2afb26356481d1a350e14544a6e160f7f7ec1561a1ea309b823665abf0309036"}]} # This file was automatically generated by gh-aw (v0.81.6). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # @@ -253,20 +253,20 @@ jobs: run: | bash "${RUNNER_TEMP}/gh-aw/actions/create_prompt_first.sh" { - cat << 'GH_AW_PROMPT_83bf1b15cc134f7a_EOF' + cat << 'GH_AW_PROMPT_d8355a4387b98e8e_EOF' - GH_AW_PROMPT_83bf1b15cc134f7a_EOF + GH_AW_PROMPT_d8355a4387b98e8e_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/xpia.md" cat "${RUNNER_TEMP}/gh-aw/prompts/temp_folder_prompt.md" cat "${RUNNER_TEMP}/gh-aw/prompts/markdown.md" cat "${RUNNER_TEMP}/gh-aw/prompts/safe_outputs_prompt.md" - cat << 'GH_AW_PROMPT_83bf1b15cc134f7a_EOF' + cat << 'GH_AW_PROMPT_d8355a4387b98e8e_EOF' Tools: add_comment, create_pull_request_review_comment(max:100), submit_pull_request_review, missing_tool, missing_data, noop, dismiss_stale_change_requests, publish_pr_check - GH_AW_PROMPT_83bf1b15cc134f7a_EOF + GH_AW_PROMPT_d8355a4387b98e8e_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/mcp_cli_tools_prompt.md" - cat << 'GH_AW_PROMPT_83bf1b15cc134f7a_EOF' + cat << 'GH_AW_PROMPT_d8355a4387b98e8e_EOF' The following GitHub context information is available for this workflow: {{#if github.actor}} @@ -308,9 +308,9 @@ jobs: stop immediately and report the limitation rather than spending turns trying to work around it. - GH_AW_PROMPT_83bf1b15cc134f7a_EOF + GH_AW_PROMPT_d8355a4387b98e8e_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/github_mcp_tools_with_safeoutputs_prompt.md" - cat << 'GH_AW_PROMPT_83bf1b15cc134f7a_EOF' + cat << 'GH_AW_PROMPT_d8355a4387b98e8e_EOF' # Azure .NET Management SDK PR Review @@ -386,8 +386,8 @@ jobs: 1. Identify the package root, `.csproj`, `CHANGELOG.md`, API surface files under `api/`, generated files under `src/Generated/`, customization files under `src/Custom*/`, `src/Customization*/`, or `src/Customized*/`, and TypeSpec customization files such as `client.tsp` and `tspconfig.yaml`. 2. Determine whether this is a migration PR. Use the migration skill when the PR title or files indicate Swagger/AutoRest to TypeSpec migration, such as adding `tsp-location.yaml`, deleting `src/autorest.md`, adding TypeSpec `metadata.json`, or broadly regenerating `src/Generated/`. 3. Determine whether the package is TypeSpec-backed by checking for `tsp-location.yaml`. For every TypeSpec-backed package, inspect added or modified SDK customization files for rename-only `[CodeGenType]`, `[CodeGenMember]`, `[CodeGenSuppress]`, wrappers, or forwarding methods, even when the PR is not a migration. - 4. Determine the latest released stable API baseline from `ApiCompatVersion` in the package `.csproj` when present. Run the trusted base-branch `.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1` script for exactly that package version and the baseline target framework. It downloads the released NuGet package and reads public assembly metadata without executing package code. This assembly is authoritative for whether an API shipped and for parameter names, ordering, types, and optionality. - 5. Fetch the corresponding tagged API file by tag name `_`. Use it as the deterministic scanner input and readable projection of the assembly. If repository history, current `main`, or the tagged API file conflicts with released assembly metadata, use the assembly metadata. + 4. Determine the latest released stable API baseline from `ApiCompatVersion` in the package `.csproj` when present. Fetch the corresponding tagged API file by tag name `_` and use it as the deterministic scanner baseline. + 5. Use the existing CI ApiCompat result as the primary binary-compatibility and parameter-name signal. Do not export released assembly metadata during scope discovery. ## Step 2 - Run deterministic checks @@ -409,7 +409,7 @@ jobs: For every `PARAMNAME001`, `PARAMORDER001`, `OPTPARAM001`, or `OPTPARAM002` result: - 1. Confirm the exact signature in the released `ApiCompatVersion` assembly. Never replace it with a signature inferred from previous repository source. + 1. Only now run the trusted base-branch `.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1` script for the package's exact `ApiCompatVersion` and baseline target framework. Confirm the signature in released assembly metadata; never replace it with a signature inferred from previous repository source. 2. Inspect all current overloads with the same containing type and member name. 3. Build a minimal compiler probe from synthesized declarations for the complete GA and current overload sets. Check required arguments supplied positionally and by name, omitted optional arguments, positional prefixes, combinations of named arguments, `default`, and explicitly typed defaults. A textual optionality difference alone is not blocking. 4. Report the baseline version, exact GA signature, current signature, and a concrete broken or ambiguous call. Do not claim an API was previously shipped unless it exists in released assembly metadata. @@ -423,7 +423,7 @@ jobs: 1. Phase 1 versioning findings are blocking, but do **not** stop after Phase 1 — continue into Phase 2 and submit one combined review so versioning and API/naming findings reach the author in the same round (per the updated Phase 1 in the skill). 2. Phase 2 API review findings should focus on new or changed public API surface only. 3. **Contextual naming must be exhaustive.** Use the scanner's `-ListNewTypes` inventory mode to enumerate every new public type, then record a verdict for each one in a single pass (see Phase 2 step 4 in the skill). Surfacing only a subset of naming issues per round is the main cause of repeated review rounds and must be avoided. - 4. Phase 3 breaking-change detection must use the CI failure details fetched in Step 0, released assembly metadata, API diffs, and every demonstrated source-compatibility result from Step 2. Do not run `dotnet build` in this workflow because that would execute untrusted PR code. If CI reports ApiCompat failures or build errors, surface them with links to the failed check run URL or Azure DevOps target URL. A passing ApiCompat result does not override a demonstrated source break, but an unverified textual parameter difference is not automatically blocking. + 4. Phase 3 breaking-change detection must use the CI ApiCompat/build results fetched in Step 0, API diffs, released assembly metadata for parameter candidates, and every demonstrated source-compatibility result from Step 2. Do not run `dotnet build` in this workflow because that would execute untrusted PR code. If CI reports ApiCompat failures or build errors, surface them with links to the failed check run URL or Azure DevOps target URL. A passing ApiCompat result does not override a demonstrated source break, but an unverified textual parameter difference is not automatically blocking. 5. For every TypeSpec-backed package, apply the base skill's `TSPRENAME001` rule. A rename-only SDK customization for a directly targetable TypeSpec API is blocking and must be replaced with scoped `@@clientName(TypeSpecTarget, "CSharpName", "csharp")` in the spec repository's `client.tsp`, followed by regeneration. 6. For migration PRs, apply Phases 4 and 5 from the migration skill. Treat manual edits to `src/Generated/` as blocking unless there is clear evidence they are generated output rather than hand edits. @@ -491,7 +491,7 @@ jobs: 5. After the spec PR merges, update `tsp-location.yaml` to the latest `main` commit in `azure-rest-api-specs` that contains the merged changes, then regenerate the SDK. ``` - GH_AW_PROMPT_83bf1b15cc134f7a_EOF + GH_AW_PROMPT_d8355a4387b98e8e_EOF } > "$GH_AW_PROMPT" - name: Interpolate variables and render templates uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 diff --git a/.github/workflows/mgmt-review.md b/.github/workflows/mgmt-review.md index 3a877a2bf534..91150954d2f8 100644 --- a/.github/workflows/mgmt-review.md +++ b/.github/workflows/mgmt-review.md @@ -290,8 +290,8 @@ For each changed management SDK package: 1. Identify the package root, `.csproj`, `CHANGELOG.md`, API surface files under `api/`, generated files under `src/Generated/`, customization files under `src/Custom*/`, `src/Customization*/`, or `src/Customized*/`, and TypeSpec customization files such as `client.tsp` and `tspconfig.yaml`. 2. Determine whether this is a migration PR. Use the migration skill when the PR title or files indicate Swagger/AutoRest to TypeSpec migration, such as adding `tsp-location.yaml`, deleting `src/autorest.md`, adding TypeSpec `metadata.json`, or broadly regenerating `src/Generated/`. 3. Determine whether the package is TypeSpec-backed by checking for `tsp-location.yaml`. For every TypeSpec-backed package, inspect added or modified SDK customization files for rename-only `[CodeGenType]`, `[CodeGenMember]`, `[CodeGenSuppress]`, wrappers, or forwarding methods, even when the PR is not a migration. -4. Determine the latest released stable API baseline from `ApiCompatVersion` in the package `.csproj` when present. Run the trusted base-branch `.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1` script for exactly that package version and the baseline target framework. It downloads the released NuGet package and reads public assembly metadata without executing package code. This assembly is authoritative for whether an API shipped and for parameter names, ordering, types, and optionality. -5. Fetch the corresponding tagged API file by tag name `_`. Use it as the deterministic scanner input and readable projection of the assembly. If repository history, current `main`, or the tagged API file conflicts with released assembly metadata, use the assembly metadata. +4. Determine the latest released stable API baseline from `ApiCompatVersion` in the package `.csproj` when present. Fetch the corresponding tagged API file by tag name `_` and use it as the deterministic scanner baseline. +5. Use the existing CI ApiCompat result as the primary binary-compatibility and parameter-name signal. Do not export released assembly metadata during scope discovery. ## Step 2 - Run deterministic checks @@ -313,7 +313,7 @@ When a baseline is available, the scanner deterministically compares parameter n For every `PARAMNAME001`, `PARAMORDER001`, `OPTPARAM001`, or `OPTPARAM002` result: -1. Confirm the exact signature in the released `ApiCompatVersion` assembly. Never replace it with a signature inferred from previous repository source. +1. Only now run the trusted base-branch `.github/skills/azure-sdk-mgmt-pr-review/Export-GaApiBaseline.ps1` script for the package's exact `ApiCompatVersion` and baseline target framework. Confirm the signature in released assembly metadata; never replace it with a signature inferred from previous repository source. 2. Inspect all current overloads with the same containing type and member name. 3. Build a minimal compiler probe from synthesized declarations for the complete GA and current overload sets. Check required arguments supplied positionally and by name, omitted optional arguments, positional prefixes, combinations of named arguments, `default`, and explicitly typed defaults. A textual optionality difference alone is not blocking. 4. Report the baseline version, exact GA signature, current signature, and a concrete broken or ambiguous call. Do not claim an API was previously shipped unless it exists in released assembly metadata. @@ -327,7 +327,7 @@ Apply all relevant phases from the skill files, with these workflow-specific adj 1. Phase 1 versioning findings are blocking, but do **not** stop after Phase 1 — continue into Phase 2 and submit one combined review so versioning and API/naming findings reach the author in the same round (per the updated Phase 1 in the skill). 2. Phase 2 API review findings should focus on new or changed public API surface only. 3. **Contextual naming must be exhaustive.** Use the scanner's `-ListNewTypes` inventory mode to enumerate every new public type, then record a verdict for each one in a single pass (see Phase 2 step 4 in the skill). Surfacing only a subset of naming issues per round is the main cause of repeated review rounds and must be avoided. -4. Phase 3 breaking-change detection must use the CI failure details fetched in Step 0, released assembly metadata, API diffs, and every demonstrated source-compatibility result from Step 2. Do not run `dotnet build` in this workflow because that would execute untrusted PR code. If CI reports ApiCompat failures or build errors, surface them with links to the failed check run URL or Azure DevOps target URL. A passing ApiCompat result does not override a demonstrated source break, but an unverified textual parameter difference is not automatically blocking. +4. Phase 3 breaking-change detection must use the CI ApiCompat/build results fetched in Step 0, API diffs, released assembly metadata for parameter candidates, and every demonstrated source-compatibility result from Step 2. Do not run `dotnet build` in this workflow because that would execute untrusted PR code. If CI reports ApiCompat failures or build errors, surface them with links to the failed check run URL or Azure DevOps target URL. A passing ApiCompat result does not override a demonstrated source break, but an unverified textual parameter difference is not automatically blocking. 5. For every TypeSpec-backed package, apply the base skill's `TSPRENAME001` rule. A rename-only SDK customization for a directly targetable TypeSpec API is blocking and must be replaced with scoped `@@clientName(TypeSpecTarget, "CSharpName", "csharp")` in the spec repository's `client.tsp`, followed by regeneration. 6. For migration PRs, apply Phases 4 and 5 from the migration skill. Treat manual edits to `src/Generated/` as blocking unless there is clear evidence they are generated output rather than hand edits.