diff --git a/.github/workflows/api-review-baselines.yml b/.github/workflows/api-review-baselines.yml index aba373d580a..5dde7732eda 100644 --- a/.github/workflows/api-review-baselines.yml +++ b/.github/workflows/api-review-baselines.yml @@ -49,6 +49,29 @@ jobs: pull_number: prNumber }); + let baseSha = pullRequest.base.sha; + const targetSha = pullRequest.merge_commit_sha ?? pullRequest.head.sha; + + // pull_request_target's `base.sha` is the original base from when the PR was opened, so it + // includes anything pushed to the target branch while the PR was open. The merge commit's + // first parent is the target tip immediately before the merge — that gives a diff that + // reflects only what the PR introduced. + if (pullRequest.merged && pullRequest.merge_commit_sha) { + try { + const { data: mergeCommit } = await github.rest.git.getCommit({ + owner, + repo, + commit_sha: pullRequest.merge_commit_sha + }); + if (mergeCommit.parents.length > 0) { + baseSha = mergeCommit.parents[0].sha; + console.log(`Using merge commit's first parent ${baseSha} as base.`); + } + } catch (err) { + console.log(`Failed to fetch merge commit; falling back to PR base.sha: ${err.message}`); + } + } + const files = await github.paginate(github.rest.pulls.listFiles, { owner, repo, @@ -65,8 +88,8 @@ jobs: })); core.setOutput('pr_number', String(prNumber)); - core.setOutput('base_sha', pullRequest.base.sha); - core.setOutput('target_sha', pullRequest.merge_commit_sha ?? pullRequest.head.sha); + core.setOutput('base_sha', baseSha); + core.setOutput('target_sha', targetSha); core.setOutput('has_baselines', String(baselineFiles.length > 0)); core.setOutput('files_json', JSON.stringify(baselineFiles)); diff --git a/eng/Tools/ApiChief/Commands/EmitDelta.cs b/eng/Tools/ApiChief/Commands/EmitDelta.cs index fb21a0ded36..76a0cc1a464 100644 --- a/eng/Tools/ApiChief/Commands/EmitDelta.cs +++ b/eng/Tools/ApiChief/Commands/EmitDelta.cs @@ -163,27 +163,94 @@ private static string FormatDiffMarkdown(ApiType type) var typeAdded = type.IsNew; var typeRemoved = type.IsRemoved; + var stageTag = FormatStageTag(type.Stage); if (typeRemoved) { - lines.Add($"- {type.Type}"); + lines.Add($"- {stageTag}{type.Type}"); } else if (typeAdded) { - lines.Add($"+ {type.Type}"); + lines.Add($"+ {stageTag}{type.Type}"); + } + else if (type.PreviousType != null && type.PreviousType != type.Type) + { + lines.Add($"- {stageTag}{type.PreviousType}"); + lines.Add($"+ {stageTag}{type.Type}"); } else { - lines.Add($" {type.Type}"); + lines.Add($" {stageTag}{type.Type}"); } AppendStageDiffLine(lines, type.Removals, '-'); AppendStageDiffLine(lines, type.Additions, '+'); AppendGroupedDiffMembers(lines, type); - return $"```diff{Environment.NewLine}{string.Join(Environment.NewLine, lines)}{Environment.NewLine}```{Environment.NewLine}"; + var wrapped = lines.SelectMany(WrapDiffLine); + return $"```diff{Environment.NewLine}{string.Join(Environment.NewLine, wrapped)}{Environment.NewLine}```{Environment.NewLine}"; + } + + private const int MaxDiffLineLength = 160; + + private static IEnumerable WrapDiffLine(string line) + { + if (line.Length <= MaxDiffLineLength) + { + yield return line; + yield break; + } + + var prefix = line.Length >= 2 ? line[..2] : " "; + var content = line.Length >= 2 ? line[2..] : line; + const string continuationIndent = " "; + var firstMax = MaxDiffLineLength - prefix.Length; + var contMax = MaxDiffLineLength - prefix.Length - continuationIndent.Length; + + var isFirst = true; + while (content.Length > (isFirst ? firstMax : contMax)) + { + var max = isFirst ? firstMax : contMax; + var breakAt = FindWrapBreak(content, max); + var chunk = content[..breakAt].TrimEnd(); + yield return isFirst ? prefix + chunk : prefix + continuationIndent + chunk; + content = content[breakAt..].TrimStart(); + isFirst = false; + } + + if (content.Length > 0) + { + yield return isFirst ? prefix + content : prefix + continuationIndent + content; + } + } + + private static int FindWrapBreak(string value, int maxLen) + { + if (value.Length <= maxLen) + { + return value.Length; + } + + // Prefer breaking right after a comma to keep type lists readable. + var commaIdx = value.LastIndexOf(',', maxLen - 1); + if (commaIdx > 0 && commaIdx + 1 < value.Length && value[commaIdx + 1] == ' ') + { + return commaIdx + 2; + } + + var spaceIdx = value.LastIndexOf(' ', maxLen - 1); + if (spaceIdx > 0) + { + return spaceIdx + 1; + } + + // No good break point — break at the limit so we still wrap. + return maxLen; } + private static string FormatStageTag(ApiStage stage) + => stage != ApiStage.Stable ? $"[{stage}] " : string.Empty; + private static void AppendStageDiffLine(List lines, ApiType? changeSet, char prefix) { if (changeSet?.Stage != null && changeSet.Stage != ApiStage.Stable) @@ -257,7 +324,8 @@ private static void AddDiffMembers(List<(string Name, string Line)> entries, ISe foreach (var member in members.OrderBy(m => GetMemberName(m.Member), StringComparer.Ordinal).ThenBy(m => m.Member, StringComparer.Ordinal)) { - entries.Add((GetMemberName(member.Member), $"{prefix} {member.Member}")); + var stageTag = FormatStageTag(member.Stage); + entries.Add((GetMemberName(member.Member), $"{prefix} {stageTag}{member.Member}")); } } diff --git a/eng/Tools/ApiChief/Model/ApiModel.cs b/eng/Tools/ApiChief/Model/ApiModel.cs index 78365dfeec0..120167cd226 100644 --- a/eng/Tools/ApiChief/Model/ApiModel.cs +++ b/eng/Tools/ApiChief/Model/ApiModel.cs @@ -112,10 +112,13 @@ private static ISet FindChanges(ApiModel baseline, ApiModel current, bo var additions = CreateChangeSet(addedStage, addedMethods, addedFields, addedProperties); var removals = CreateChangeSet(removedStage, removedMethods, removedFields, removedProperties); + var headerChanged = currentType != null && baselineType != null && currentType.Type != baselineType.Type; + if (addedStage == ApiStage.Stable && removedStage == ApiStage.Stable && additions == null - && removals == null) + && removals == null + && !headerChanged) { return null; } @@ -124,6 +127,7 @@ private static ISet FindChanges(ApiModel baseline, ApiModel current, bo { Type = outputType.Type, Stage = currentType?.Stage ?? baselineType?.Stage ?? ApiStage.Stable, + PreviousType = headerChanged ? baselineType!.Type : null, Methods = ToDisplayMembers(sharedMethods), Fields = ToDisplayMembers(sharedFields), Properties = ToDisplayMembers(sharedProperties), diff --git a/eng/Tools/ApiChief/Model/ApiType.cs b/eng/Tools/ApiChief/Model/ApiType.cs index d469fe44dd1..420a617b0f4 100644 --- a/eng/Tools/ApiChief/Model/ApiType.cs +++ b/eng/Tools/ApiChief/Model/ApiType.cs @@ -40,7 +40,45 @@ public sealed class ApiType : IEquatable [JsonIgnore] public bool IsRemoved { get; set; } - public bool Equals(ApiType? other) => other != null && Type == other.Type; + /// + /// The baseline declaration when only the type header changed (e.g. an interface was added or removed). + /// Not serialized; only used by the in-memory delta to surface header-only modifications. + /// + [JsonIgnore] + public string? PreviousType { get; set; } + + /// + /// Stable identity for matching baseline and current types. Strips the base/interface list and generic + /// constraints so that changes to those are reported as modifications instead of a removed-and-re-added type. + /// + [JsonIgnore] + public string Identity => GetIdentity(Type); + + private static string GetIdentity(string type) + { + if (string.IsNullOrEmpty(type)) + { + return string.Empty; + } + + var endIdx = type.Length; + + var whereIdx = type.IndexOf(" where ", StringComparison.Ordinal); + if (whereIdx >= 0) + { + endIdx = whereIdx; + } + + var colonIdx = type.IndexOf(" : ", StringComparison.Ordinal); + if (colonIdx >= 0 && colonIdx < endIdx) + { + endIdx = colonIdx; + } + + return type[..endIdx].TrimEnd(); + } + + public bool Equals(ApiType? other) => other != null && Identity == other.Identity; public override bool Equals(object? obj) => Equals(obj as ApiType); - public override int GetHashCode() => Type.GetHashCode(); + public override int GetHashCode() => Identity.GetHashCode(); }