Resolve SR sub-patch milestones from the earliest containing tag - #35972
Conversation
The milestone-drift script mapped a commit found on an SR release branch to
a milestone using only the branch name (release/10.0.1xx-sr7 -> .NET 10 SR7).
An SR branch ships many servicing drops (10.0.70=SR7, 10.0.71=SR7.1, ...), so
the base SR is too coarse: a revert/hotfix that lands after the base SR shipped
actually goes out in a later sub-patch, and milestoning it as the base SR can
downgrade an already-correct SR7.1 issue back to SR7.
Add Get-RefinedReleaseMilestone, which refines an SR branch milestone to the
earliest SR-family tag (X.0.{sr}{sub}) that contains the commit. If no family
tag contains it yet, it uses the next sub-patch after the latest shipped tag,
clamped to the SR family so it never crosses into the next SR. Non-SR
milestones (preview/rc/GA) are returned unchanged. Wire it into both match
paths of Find-ReleaseBranchForCommit; the PR-number grep fallback captures the
oldest on-branch SHA (--reverse) and is hardened against stderr noise.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35972Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35972" |
github-actions
Bot
added
the
area-infrastructure
Apply consensus fixes from a multi-model adversarial review of the SR sub-patch milestone resolution: - Test-CommitInTag: distinguish a real git error (exit >1) from a clean "not an ancestor" (exit 1). A transient failure on the earliest containing tag is now thrown instead of being silently read as "not contained", which could over-refine the milestone. Callers catch this and fall back to the coarser base SR milestone. - Grep fallback: emit '%H<US>%s' and accept only commits whose SUBJECT ends with the squash token '(#PrNum)' (new Get-OnBranchShaFromLog helper). This rejects body-only PR mentions and quoted reverts (e.g. Revert "Fix X (#100)" (#200)) that a raw --grep would match and refine to the wrong sub-patch. - Get-RefinedReleaseMilestone: wrap the tag scan so a git error falls back to the base SR milestone (never wrong, just less precise) instead of crashing or predicting a wrong drop. - Add Pester coverage for the git-error fallback and grep subject matching (genuine squash, body-only mention, quoted revert, oldest-first selection, trailing whitespace, #42-vs-#420 prefix). 171 Pester tests pass; live dry-runs unchanged (#35694 -> SR7.1, #34620 -> SR6, #30132 -> preview3). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Multi-model code review — PR #35972
3-model consensus: LGTM with 3 non-blocking suggestions
What this changes
Fixes an SR sub-patch milestoning bug in the milestone-drift automation. The script previously mapped commits to milestones using only branch name (release/10.0.1xx-sr7 → .NET 10 SR7), which is too coarse since a single SR branch ships many servicing drops (SR7, SR7.1, SR7.2, ...). A revert/hotfix landing after the base SR shipped goes out in a later sub-patch, but was mislabeled to the base SR — potentially downgrading already-correct milestones.
Concrete case: PR #35694 backport to release/10.0.1xx-sr7, contained only in tag 10.0.71 (ships in SR7.1), but script resolved to SR7, risking downgrade of linked issue #35584 from SR7.1 → SR7.
Fix: Add Get-RefinedReleaseMilestone — for commits on SR branches, resolve milestone from the earliest SR-family tag that actually contains the commit (git ancestry via git merge-base --is-ancestor). If no family tag contains it yet, predict the next sub-patch after latest shipped (clamped to SR family — no silent SR7→SR8 crossing). Non-SR milestones unchanged.
Note: Author pushed hardening commit d5aa71f0a after initial independent reviews, addressing error-handling gaps found by reviewers.
Review approach
Independent phase: 3 models (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro) each read full source, diff, git history, ran tests, and verified end-to-end against real tagged repos.
Cross-pollination phase: All 3 models reconciled findings. Opus discovered the hardening commit and recalibrated against current HEAD.
Findings
💡 Suggestion 1 — Test coverage gap
File: .github/scripts/Fix-MilestoneDrift.Tests.ps1:1259
Category: maintainability
All 3 models flagged this
All new tests mock both Test-CommitInTag and Get-AllTags, so actual git integration (git merge-base --is-ancestor ancestry, git tag -l parsing) is never exercised end-to-end in CI. The commit-message grep fallback is now well-tested via dedicated Get-OnBranchShaFromLog test block (8 unmocked cases).
Why non-blocking: Reviewers manually verified against real tagged repos (SR7→SR7.1 for #35694 case, untagged→SR7.2, family clamp). Mocking style matches rest of suite.
Optional follow-up: One integration-style test that builds a tiny tagged repo and calls unmocked helpers would guard against future git-flag/SHA-format regression.
💡 Suggestion 2 — Consider caching git tag lookups
File: .github/scripts/Fix-MilestoneDrift.ps1:293
Category: performance
All 3 models flagged this
Get-RefinedReleaseMilestone calls Get-AllTags $Repo (→ git tag -l) once per matched PR, so N SR-branch PRs re-shell git tag -l N times.
Why low priority:
- Follows existing convention (already uncached at 3 other sites in same script)
git tag -lis local/fast even with thousands of tags- Drift script processes bounded set of drifted items
Optional micro-optimization: If future batch sizes grow large, hoist tag list into per-run cache (like Get-PrsInTag memoization pattern).
💡 Suggestion 3 — Behavioral change for untagged commits
File: .github/scripts/Fix-MilestoneDrift.ps1:313
Category: edge-case
Opus + Gemini
Untagged post-base-SR commits now produce FUTURE milestones (e.g. .NET 10 SR7.2) that likely don't exist in GitHub yet → downstream skips with warning (line 681) rather than applying.
Why this is correct:
- Old base-SR answer was exactly the bug being fixed
- Self-corrects on later run once real tag ships
- Safe/correct degradation (deferred, not mislabeled)
Worth confirming: Automation is expected to re-run after sub-patch milestones are created, since post-base-SR commits are now deferred where previously they were mislabeled.
Verification performed by reviewers
- ✅ Full suite: 162 passed / 0 failed (incl. 9 new tests) on pwsh 7.5.4 / Pester 5.7.1
- ✅ End-to-end unmocked against real tagged repos:
- C1 in
.70→SR7 - C2 in
.71only →SR7.1(exact #35694 bug case) - C3 untagged →
SR7.2 - Unrelated
10.0.80(SR8) correctly ignored for SR7 family
- C1 in
- ✅ Anti-hijack: Two commits mentioning
(#100)→ oldest SHA wins - ✅ Downstream safety: Refined-but-nonexistent milestone → warn-and-skip (no crash)
- ✅ PowerShell safety: Only
[int]values interpolated (no injection) - ✅ CI checks: passing
Verdict
LGTM (unanimous)
Confidence: High
Reasoning: The fix demonstrably solves the stated problem with correct git ancestry logic, family-boundary clamping, and safe fallbacks. All 3 reviewers verified end-to-end including edge cases. The only findings are non-blocking maintenance/performance suggestions. Author's hardening commit addressed error-handling gaps.
Review conducted by: @kubaflo's autonomous multi-model review loop
Models: Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro
Timestamp: 2026-06-17T19:10Z
| It 'refines to the earliest sub-patch tag that contains the commit (SR7 → SR7.1)' { | ||
| # Family tags 10.0.70/71/72 all exist; commit shipped in .71 (and is | ||
| # therefore also in .72), but NOT in the base .70. Earliest containing = .71. | ||
| Mock Get-AllTags { return @('10.0.60', '10.0.70', '10.0.71', '10.0.72', '11.0.0') } |
There was a problem hiding this comment.
💡 [3-model consensus] Test coverage gap: All new tests mock both Test-CommitInTag and Get-AllTags, so the actual git integration (git merge-base --is-ancestor ancestry and git tag -l parsing) is never exercised end-to-end in CI. The commit-message grep fallback is now well-tested via the Get-OnBranchShaFromLog test block (8 cases, unmocked), but the git ancestry remains mock-only. Reviewers verified manually against real tagged repos (SR7→SR7.1 for #35694, untagged→SR7.2, family clamp). Optional: one integration-style test that builds a tiny tagged repo and calls unmocked helpers would guard against future git-flag/SHA-format regression. Non-blocking — mocking style matches the rest of the suite.
| try { | ||
| $low = $sr * 10 | ||
| $high = $low + 9 | ||
| $familyTags = @(Get-AllTags $Repo | Where-Object { |
There was a problem hiding this comment.
💡 [3-model consensus] Consider caching Get-AllTags $Repo (→ git tag -l) per batch run. The refiner calls this once per matched PR, so N SR-branch PRs re-shell git tag -l N times. Low priority: (1) follows existing convention (already uncached at 3 other sites), (2) git tag -l is local/fast even with thousands of tags, (3) drift script processes bounded set. Optional micro-optimization if future batch sizes grow large.
| # silently crossing the family boundary (e.g. SR7 → SR8). | ||
| if ($familyTags.Count -gt 0 -and $familyTags[-1] -match "^$msMajor\.0\.(\d+)$") { | ||
| $nextPatch = [int]$Matches[1] + 1 | ||
| if ($nextPatch -le $high) { |
There was a problem hiding this comment.
💡 [Opus + Gemini] Behavioral change worth confirming: Untagged post-base-SR commits now produce FUTURE milestones (e.g. .NET 10 SR7.2) that likely don't exist in GitHub yet → downstream skips with warning (line 681) rather than applying the old mislabeled base-SR. This is safe/correct (old answer was the bug being fixed) and self-corrects once real tag ships. Confirms automation is expected to re-run after sub-patch milestones are created, since post-base-SR commits are now deferred where previously they were mislabeled. No code change required.
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
🤖 Multi-model re-review — round 2 (head d5aa71f)
3-model consensus: LGTM
Excellent hardening commit! 171/171 Pester tests pass.
What changed since round 1
Shane pushed commit d5aa71f ("Harden milestone resolution from adversarial review") addressing robustness concerns from a different review source:
-
✅
Test-CommitInTag— Distinguishes git error (exit > 1, throws) from clean "not ancestor" (exit 1). Prevents silent over-refinement from transient failures. -
✅ Grep fallback →
Get-OnBranchShaFromLog— Emits%H<US>%s, accepts only commits whose SUBJECT ends with squash token(#PrNum). Rejects body-only mentions and quoted reverts (e.g.,Revert "Fix X (#100)" (#200)). Verified correct against 8 new Pester test cases. -
✅
Get-RefinedReleaseMilestone— Wraps tag scan in try/catch; git error falls back to base SR milestone instead of crashing. Family-boundary clamp prevents SR7→SR8 leakage.
Empirical validation: All models verified correctness at source. Opus ran 171/171 Pester tests locally + edge-case probes. No bugs found. No regressions introduced.
Round-1 suggestions (all still valid, all non-blocking)
Round-1 had 3 non-blocking suggestions. All remain valid optional improvements. (inline)
Verdict
LGTM (unanimous, high confidence)
The hardening commit is correct, thoroughly tested, and regression-free. Ready for merge.
3-model panel: Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro
| try { | ||
| $low = $sr * 10 | ||
| $high = $low + 9 | ||
| $familyTags = @(Get-AllTags $Repo | Where-Object { |
There was a problem hiding this comment.
💡 [3-model consensus: non-blocking] Consider caching Get-AllTags $Repo per batch run. The refiner calls this once per matched PR, so N SR-branch PRs re-shell git tag -l N times. Low priority: (1) follows existing convention (already uncached at 3 other sites), (2) git tag -l is local/fast even with thousands of tags, (3) drift script processes bounded set. Optional micro-optimization if future batch sizes grow large.
| It 'refines to the earliest sub-patch tag that contains the commit (SR7 → SR7.1)' { | ||
| # Family tags 10.0.70/71/72 all exist; commit shipped in .71 (and is | ||
| # therefore also in .72), but NOT in the base .70. Earliest containing = .71. | ||
| Mock Get-AllTags { return @('10.0.60', '10.0.70', '10.0.71', '10.0.72', '11.0.0') } |
There was a problem hiding this comment.
💡 [3-model consensus: non-blocking] Test coverage gap: All new tests mock both Test-CommitInTag and Get-AllTags, so the actual git integration (git merge-base --is-ancestor ancestry and git tag -l parsing) is never exercised end-to-end in CI. The hardening commit added thorough unmocked coverage for the grep fallback via Get-OnBranchShaFromLog tests. Optional: one integration-style test that builds a tiny tagged repo and calls unmocked helpers would guard against future git-flag/SHA-format regression. Non-blocking — mocking style matches the rest of the suite.
Adds an unmocked Pester block that builds a throwaway local git repo (temp dir, empty commits, SR-family tags 10.0.70/71/72) and exercises the real Get-RefinedReleaseMilestone / Test-CommitInTag plumbing end-to-end: earliest-containing-tag resolution (SR7.1), base-drop (SR7), next-sub-patch prediction for an untagged commit (SR7.3), non-SR passthrough, and the ancestor true/false/throw-on-bad-object paths. No GitHub, no network, no mutation of the checkout; skipped cleanly when git is absent. Addresses the integration-test gap raised in the PR review panel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Problem
The milestone-drift automation (
Fix-MilestoneDrift.ps1) maps a commit found on an SR release branch to a milestone using only the branch name —release/10.0.1xx-sr7→.NET 10 SR7. But a single SR release branch ships many servicing drops over its lifetime (10.0.70= SR7,10.0.71= SR7.1,10.0.72= SR7.2, …), so the base SR is too coarse.A revert/hotfix that lands after the base SR shipped actually goes out in a later sub-patch. Milestoning it as the base SR is wrong — and can even downgrade an already-correct SR7.1 issue back to SR7.
Concrete case that motivated this
release/10.0.1xx-sr7; its commit is contained only in tag10.0.71→ it ships in SR7.1.Fixes #35584.Root cause
Find-ReleaseBranchForCommitresolved the milestone viaConvertBranchToMilestone(branch-name only), which has no notion of sub-patches.Fix (general — no special-casing)
Add
Get-RefinedReleaseMilestone: for a commit found on an SR branch, resolve the milestone from the earliest SR-family tag (X.0.{sr}{sub}) that actually contains the commit (git ancestry). Rules:.79falls back to base SR7, never predicts SR8).It's wired into both match paths of
Find-ReleaseBranchForCommit. The PR-number grep fallback now captures the oldest on-branch SHA (--reverse, so a later revert that re-mentions(#NNN)can't hijack the result) and is hardened against stderr noise (2>$null+ strict 40-hex SHA filter).Result: #35694 → SR7.1, so its existing
Fixes #35584link marks the issue SR7.1. The existing earliest-release-wins guard reconciles the issue regardless of merge order.Note: PR #35625 (the same revert on
inflight/current) intentionally stays SR9 — a PR's milestone tracks the physical branch it merged into; the issue converges to the earliest customer-facing release (SR7.1).Verification
.NET 10 SR7.1(was SR7); issue [Android] MissingMethodException AccessibilityNodeInfoCompat.set_Checked(bool) on 10.0.70 due to AndroidX.Core 1.17 breaking change #35584 already-correct (no downgrade).Review
Reviewed with three independent models (GPT-5.5, Gemini 3.1 Pro, Claude Opus 4.8). Consensus on correctness and contained blast radius (refinement only ever moves within one SR family). Their concrete findings — boundary clamp, stderr hardening, oldest-match grep, and a dead parameter — were all addressed in this PR.