Extract shared versioning module, add -CloseFixedIssues flag, and earliest-release-wins milestone validation#35858
Conversation
Move 7 version/milestone helpers (Get-CurrentMajorVersion, Get-MainBranchForVersion, Get-VersionFromGitRef, ConvertTo-Milestone, ConvertBranchToMilestone, Get-TagSortKey, Find-PreviousTag) out of Fix-MilestoneDrift.ps1 and into a new reusable PowerShell module at .github/scripts/shared/MauiReleaseVersioning.psm1 so other release tooling (e.g. release-readiness scripts) can share one source of truth for MAUI's version/milestone naming scheme. Fix-MilestoneDrift.ps1 now Import-Modules the new psm1 instead of defining these helpers inline. Net: -162 / +8 lines on the script. The module uses Set-StrictMode internally and exports the 7 helpers explicitly. Several string parameters use [AllowEmptyString()][AllowNull()] to preserve back-compat with the dot-sourced positional-parameter calls in the existing tests. All 91 existing Pester tests in Fix-MilestoneDrift.Tests.ps1 still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…h PRs
GitHub only auto-closes 'fixes #NNN' linked issues for PRs merged to the
default branch (main). When a PR merges to a non-default development
branch like net11.0, the linked issue stays open even though the work
shipped. This adds an opt-in mechanism to deliberately close those
issues.
Script changes (.github/scripts/Fix-MilestoneDrift.ps1):
* New -CloseFixedIssues switch (default off, behavior unchanged when
not passed).
* New Close-LinkedIssue helper: checks current issue state via gh,
no-ops if already closed, honors -Apply for dry-run, swallows gh
failures with a warning so one bad issue doesn't fail the run.
Leaves a comment explaining why it was closed (PR # + base branch).
* New Invoke-GhCli thin wrapper around the gh CLI so Pester can mock
gh invocations without spawning real processes.
* Reuses existing Get-LinkedIssues regex parser — no duplicated
'fixes|closes|resolves' parsing.
* Wired into both the single-PR path (Invoke-AnalyzeSinglePr) and the
tag-based audit path (Invoke-AnalyzeRelease) — only invoked when
-CloseFixedIssues is set.
Workflow wiring (.github/workflows/fix-milestone-drift.yml):
* New close_fixed_issues workflow_dispatch input (default false).
* On the pull_request_target auto-trigger, -CloseFixedIssues is passed
automatically when github.event.pull_request.base.ref matches net*.0.
Deliberately NOT auto-enabled for main, release/*, or inflight/* —
those either auto-close already or need human judgment.
Tests (.github/scripts/Fix-MilestoneDrift.Tests.ps1):
7 new Pester tests cover Close-LinkedIssue:
- already-closed issue (no gh close call)
- case-insensitive 'closed' state
- open issue + -Apply: correct gh close args + comment text
- dry-run mode: no gh close call
- gh view failure: warn, don't throw
- gh close failure: warn, don't throw
- empty BaseRef: comment falls back to placeholder
Pester results: 98 tests pass (91 existing + 7 new).
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 -- 35858Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35858" |
When an issue is already milestoned to an earlier release (e.g., .NET 10 SR6)
and a later-released fix PR is in the audit range (e.g., a net11.0 forward port),
the previous behavior would overwrite the earlier milestone with the later one.
That erased the 'first shipped in' history.
This adds an earliest-release-wins rule:
* If the issue's current milestone is null/Backlog/Planning, always
apply the target milestone (no validation needed).
* If the issue's current milestone is a real release earlier than the
target, look for a merged PR that links the issue with a fix verb
and has that milestone. If found, KEEP the earlier milestone.
* If the current milestone is a real release later than the target,
or equal, apply the target.
The check is scoped to issues only — PRs land in exactly one branch, so
the audit target is by definition correct for a PR's milestone.
New shared helpers in MauiReleaseVersioning.psm1:
* Get-MilestoneSortKey — numeric chronological key, null for non-release
* Compare-MauiMilestone — -1/0/1/null comparison
New script-local helper in Fix-MilestoneDrift.ps1:
* Test-MilestoneValidForIssue — gh search of linking PRs, caches lookups
Report changes:
* New Kept bucket alongside Corrections
* Console output adds [KEEP] log lines
* JSON output adds 'kept' array and 'kept_earlier' count
* Markdown summary adds a 'Kept (earlier milestone validated)' table
Tests:
* 20 new Pester tests (118 total, was 98)
* Covers sort key edges, comparison ordering, validation with mocks,
cache behavior, gh failure modes, and the 'fix verb prefix' shape
Verified against the 11.0.0-preview.3.26203.7 audit run:
* Issue #34490 (.NET 10 SR6) — KEEP (validated via #34501)
* Issue #31280 (.NET 10 SR7) — KEEP (validated via #34885)
* Other 51 corrections proceed as before.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 'if … else @() in assignment' pattern collapses an empty array to $null under StrictMode, which makes $keptItems.Count throw 'property Count cannot be found on this object'. The error fires in Save-ReportJson, which runs *before* Invoke-ApplyCorrections — so the script never gets a chance to write any milestone corrections when zero items were kept. This regressed in the previous commit (earlier-milestone-wins validation) and was masked while developing because preview3 always produced KEEPs. Reproduces deterministically on any release where no SR-vs-preview collisions exist (e.g. the preview2 audit). Fix: declare $keptItems with an explicit [object[]] cast and a default of @(), so it stays a real (possibly empty) array even when no Kept field is on the report. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ersedes #35754) Folds #35754's net11-preview-readiness work into the existing release-readiness skill so dotnet/maui has ONE deterministic readiness pipeline that handles both Servicing Releases (SR) and Previews — in both in-flight and candidate (pre-cut) modes. Changes ======= Find-ReleaseReadinessTrackers.ps1 - Four-lane detector now covers SR + Preview branches grounded in the tag-existence rule (a release is in-flight if and only if its expected tag has not been published). - -AllActiveMajors enumerates every active major (current + lower in-flight) in one envelope, ready for matrix expansion. - Trackers always advertise a canonical proposed branchName (even for candidates that have not been cut yet); a new branchExists flag is the explicit signal for whether the branch is on origin. Get-PreviewReadiness.ps1 (new, ~720 lines) - Port of #35754's net11 readiness adapted to the skill structure. - Takes -Branch + -Mode {in-flight,candidate} + -SurveyRef; candidate mode surveys net<major>.0 with the inflight-bump check skipped. - Embeds canonical idempotency markers (release-readiness-tracker / -flavor / -mode) so daily issue updates can join on the canonical key. - Preserves the human-notes block between marker fences across re-runs. release-readiness.yml workflow (new) - Weekday 08:30 UTC cron + workflow_dispatch + PR validation. - detect-trackers job emits a matrix from Find-Trackers -AllActiveMajors; one matrix job per tracker dispatches to Get-ReleaseReadiness (SR) or Get-PreviewReadiness (preview) based on branchType. - Idempotent issue handling: look up an open tracker issue by the canonical marker, edit it in place; otherwise create one with report / s/triaged / area-release-readiness labels. - Activity gate: when recentCommitCount == 0 AND no open tracker issue exists, skip new-issue creation (existing open issues are still refreshed). - PR-trigger validate job runs the test suite and smoke-runs every detected tracker against the report scripts; never touches issues. SKILL.md - Documents the SR + Preview entry points, the tag-existence rule, the daily workflow, the branchExists / branchName contract, and the shared MauiReleaseVersioning.psm1 dependency. Tests - Release-readiness suite: 273/273 passing (added preview-lane unit tests + -AllActiveMajors E2E; updated for SR7-shipped live state and the new branchExists contract). - Fix-MilestoneDrift Pester suite: 118/118 passing (validates that the earliest-release-wins milestone logic merged in from #35858 still works after this branch's edits). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Multimodal review found a few correctness issues in the new milestone validation paths:
|
… PRs The previous earliest-release-wins guard was too loose in two ways: 1. For issues, it trusted the linking PR's milestone field as proof the fix shipped in an earlier release. That field can be stale — e.g. a hotfix PR milestoned '.NET 10 SR7' but whose commit only lives in tag 10.0.71 (which maps to '.NET 10 SR7.1', not SR7). The previous logic would incorrectly KEEP an issue at SR7 in that case. 2. It didn't apply to PRs at all. The assumption 'a PR lands in one branch, so the target is by definition correct' breaks when a PR gets cherry-picked across release branches. Example: PR #34527 originally shipped in 10.0.60 (SR6) but its commit also appears in 10.0.80 (SR8) via cherry-pick — an SR8 audit would stomp the correct SR6 milestone. Both holes share the same fix: require git-level proof that the commit in question is actually reachable from a tag that maps to the claimed earlier milestone. Implemented via: - Get-TagsForMilestone(name) → tags whose ConvertTo-Milestone matches - Get-PrsInTag(tag) → cached HashSet of PR numbers reachable from tag - Test-PrShippedInMilestone(pr, milestone) → cross-check the two Test-MilestoneValidForIssue now requires the linking PR's commit be present in a tag mapping to the issue's current milestone (not just that the PR's own milestone field matches). Test-MilestoneValidForPr is new and applies the same check directly to the PR being audited. Both are wired into Test-AndRecordCorrection so issues and PRs share the KEEP branch. The check is fail-safe: any null/empty/unparseable milestone or git failure falls through to existing behavior — the new code can only ever turn a would-be mutation into a no-op. Validated against 10.0.80 dry-run: KEEPs #34527 (correctly, shipped in SR6), still moves the other 78 PRs that genuinely only ship in SR8. Tests: 118 → 122, all green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
There was a problem hiding this comment.
3-Reviewer Adversarial Review
Methodology: 3 independent reviewers (different model families), adversarial consensus with a dispute round for 1/3 findings. Read full source, not just diff.
Verdict: solid PR with one production-path regression and one regex hole that together neutralize commit 3 in the live workflow path. Plus 4 should-fix items. Recommend addressing A, B, C before merge.
@kubaflo's multimodal review findings are independently confirmed by this consensus (mapped below as B, D, E).
❌ Critical (must-fix before merge)
A. Initialize-MilestoneValidationContext is never called in single-PR mode (3/3 after dispute)
Fix-MilestoneDrift.ps1—Invoke-AnalyzeSinglePr(~lines 742–908) never seeds the validation context. OnlyInvoke-AnalyzeRelease(line 937) does.- Consequence: in the GitHub Action's auto-trigger path (
pull_request_target→-PrNumber N),$script:validationAllTagsstays$null,Get-TagsForMilestonereturns@(),Test-PrShippedInMilestonereturns$false, and the entire earliest-release-wins KEEP guard never fires. - The exact bug commit 3 was written to prevent will silently happen on every PR merge in production. A
net11.0cleanup that saysFixes #34490will stomp.NET 10 SR6→.NET 11.0-preview3and also auto-close the issue when-CloseFixedIssuesis active. - Fix: after
$Majorand$Repoare known inInvoke-AnalyzeSinglePr, add:$allTagsForCtx = Get-AllTags $Repo Initialize-MilestoneValidationContext -RepoPath $Repo -AllTags ([string[]]$allTagsForCtx) -Major $Major
- Add an integration Pester test that drives
Invoke-AnalyzeSinglePrwith an earlier-valid milestone and asserts the item lands in$report.Kept. (No such coverage exists today — that's why this slipped through.)
B. Get-MilestoneSortKey SR regex doesn't match the .NET <N>.0 SR… form (3/3, also flagged by @kubaflo)
shared/MauiReleaseVersioning.psm1lines 311–318 — patterns require.NET <num> SR…exactly, no.0.- I verified via
gh api repos/dotnet/maui/milestones?state=all: both forms coexist in production —.NET 10 SR4,.NET 10 SR8(no.0) AND.NET 10.0 SR1,.NET 10.0 SR2.1,.NET 10.0 GA(with.0). - Net effect:
Compare-MauiMilestone '.NET 10.0 SR1' '.NET 11.0-preview3'→$null→ KEEP guard at line 695 (if ($null -ne $cmp -and $cmp -lt 0)) skips → correct earlier milestone overwritten. - Notably,
Test-MilestoneMatchalready normalizes both forms — the sort key just didn't get the memo. - Fix: accept optional
.0in both SR patterns (and GA defensively):if ($Milestone -match '^\.NET (\d+)(?:\.0)? SR(\d+)\.(\d+)$') { ... } if ($Milestone -match '^\.NET (\d+)(?:\.0)? SR(\d+)$') { ... } if ($Milestone -match '^\.NET (\d+)(?:\.0)? GA$') { ... }
- Add parameterized tests for the
.0forms incl.Compare-MauiMilestone '.NET 10.0 SR5' '.NET 11.0-preview3' | Should -Be -1.
C. Test-MilestoneValidForIssue fails open on gh search errors (2/3 after dispute)
Fix-MilestoneDrift.ps1lines 586–593 — on$LASTEXITCODE -ne 0the function warns, caches$false, returns$false. Caller inTest-AndRecordCorrectionline 696 treats$falseidentically to "earlier milestone is invalid" and proceeds with the destructive correction.- During a bulk tag-mode audit, a transient rate-limit or network blip would silently destroy legitimate earlier milestones with no recovery path. The current Pester test at lines 619–623 explicitly locks in this fail-open behavior.
- Fix: return
$null(or throw a typed exception) on API failure and update the caller to treat$nullas "uncertain — skip this item, don't queue a correction." Update the test to verify the fail-safe outcome.
⚠️ Should-fix
D. Test-MilestoneValidForIssue search misses URL-form and title-only references (3/3, also flagged by @kubaflo)
- Query
"#$IssueNumber in:body"(line 586) won't return a PR that uses onlyFixes https://github.com/dotnet/maui/issues/N(no#NNNtoken) or that only putsFixes #Nin the title. Same failure mode as C — falsely returns false → milestone overwritten. - The downstream verb regex at line 609 was widened to handle both forms, but the upstream query gates what reaches it.
- Fix: broaden to e.g.
"(#$IssueNumber OR issues/$IssueNumber) (in:body OR in:title)", or two narrow searches merged. Consider raising--limit 50for long-lived issues.
E. KEEP bucket is not deduped (3/3, also flagged by @kubaflo)
Test-AndRecordCorrection(~lines 670–684) dedupesAlreadyCorrectvia_checkedItemsandCorrectionsvia aWhere-Objectscan, but the KEEP branch (lines 704–714) appends unconditionally. In tag-mode walks, an issue or cherry-picked PR referenced by multiple linking PRs lands in$Report.Keptonce per visit.- Effect: inflated
kept_earliercounts in JSON, duplicate rows in the GitHub issue's "Kept" table, duplicate[KEEP]console lines. - Fix: add
_keptItems = [HashSet[string]]::new()and guard the append:if (-not $Report.ContainsKey('_keptItems')) { $Report._keptItems = [System.Collections.Generic.HashSet[string]]::new() } if (-not $Report._keptItems.Add("$ItemType`:$ItemNumber")) { return }
F. Get-LinkedIssues silently drops the cross-repo short form (2/3 after dispute)
Fix-MilestoneDrift.ps1~line 349 — first regex is(?:fix.../close.../resolve...)\s+#(\d+). The cross-repo formFixes dotnet/maui#12345doesn't match (the second regex only handleshttps://github.com/...URLs).- This is internally inconsistent:
Test-MilestoneValidForIssue's verb regex at line 609 WAS widened to(?:[a-z0-9_\-./]+)?#$IssueNumberand a Pester test asserts cross-repo forms work for validation. Extraction silently misses them; validation accepts them. - Effect: with
-CloseFixedIssuesactive, an issue referenced asFixes dotnet/maui#Nwon't be closed (or milestoned). - Fix: add the optional prefix to the first regex:
'(?:fix(?:es|ed)?|close[sd]?|resolve[sd]?)\s+(?:[A-Za-z0-9_\-./]+)?#(\d+)'
Cleared (orchestrator-verified, no concerns)
- Workflow
pull_request_targetcheckout safety — noref:parameter, defaults to merge commit on base ref → script content is trusted ✓ - Shell injection via
INPUT_*env vars — properly quoted,[int]cast on$PrNumberrejects garbage ✓ Find-ReleaseBranchForCommit's--grep="(#$PrNum)"—$PrNumis[int]✓Set-ItemMilestoneworks on closed milestones (raw API with milestone number) — confirmed live in the SR7.1 case during validation ✓Save-ReportJsonempty-Kept handling — the[object[]]cast on line 1080 correctly defends StrictMode.Count✓Close-LinkedIssuecase-insensitiveCLOSEDcheck + warning-not-throwing failure mode ✓gh issue close--reason completed --comment "..."invocation — correct arg shape, escaping handled via splat ✓
Discarded
- Mass-mutation-via-untrusted-text concern around
-CloseFixedIssues(1/3, opt-in flag and the project has the same exposure onmainvia GitHub's built-in auto-close, so this isn't a net new attack surface).
Test coverage observation
The current Pester suite exercises Test-MilestoneValidForIssue, Test-MilestoneValidForPr, Test-AndRecordCorrection, and Close-LinkedIssue in isolation but has no integration test that drives Invoke-AnalyzeSinglePr end-to-end with a KEEP-eligible item. Adding one would have caught Finding A immediately. Recommend adding before merge.
The PR description's "91 → 118" test count is also stale by 1 commit (actual is ~122 — minor description nit, not a correctness issue).
…en milestone parsing & issue-linking, fail-safe on uncertain validation, dedupe KEEP bucket PR review findings from 3-reviewer adversarial review (Opus, GPT-5.5, Gemini-3.1-Pro): A. `Invoke-AnalyzeSinglePr` never called `Initialize-MilestoneValidationContext`, making the earliest-release-wins KEEP guard dead code in the live workflow path. Now seeds the context (reusing `$allTags` when already loaded from -ReleaseTag mode). B. `Get-MilestoneSortKey` regexes treated `.NET 10.0 SR4` / `.NET 10.0 GA` as non-comparable (production uses BOTH `.NET 10 SR4` and `.NET 10.0 SR4` forms). Both SR and GA patterns now accept an optional `.0` between major and label. C. `Test-MilestoneValidForIssue` returned `$false` on gh search/parse failure, which caused `Test-AndRecordCorrection` to clobber valid earlier milestones on transient API errors. Now returns `$null` (tristate), and the caller skips the item with a warning instead of queueing a destructive correction. D. The original `gh search prs "#N in:body"` query missed two real cases: (1) `Fixes` in the PR title (not body) and (2) URL-form linking (`Fixes https://github.com/dotnet/maui/issues/N` with no `#N` token anywhere). Now issues two searches (`#N in:title,body` + `issues/N in:body`) and dedupes by PR number. E. `Test-AndRecordCorrection` could add the same item to the Kept bucket multiple times when it was reached via different linking PRs in a tag-range walk. Added a `_keptItems` HashSet for idempotent KEEP recording (mirrors the existing `_checkedItems` dedup for AlreadyCorrect). F. `Get-LinkedIssues` first regex didn't accept the cross-repo `Fixes dotnet/maui#NNN` form (only `Fixes #NNN`). Added optional `[A-Za-z0-9_\-./]+` prefix so cross-repo linking is parsed too. Test coverage: - Updated existing failure-mode test for Test-MilestoneValidForIssue to assert $null (Fix C). - Updated caching test to assert 2 calls per uncached invocation, not 1 (Fix D). - New Get-MilestoneSortKey tests for `.0`-form milestones (Fix B). - New Get-LinkedIssues tests for cross-repo form (Fix F). - New Test-MilestoneValidForIssue tests for URL-form linking + title-only linking (Fix D). - New Describe block covering Test-AndRecordCorrection KEEP dedup and tristate handling (Fixes A/C/E). - New integration test for Invoke-AnalyzeSinglePr verifying Initialize-MilestoneValidationContext is called (Fix A — closes the coverage gap that let this regression slip). Pester: 134 tests pass (up from 122). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ed earliest-release-wins Brings in: - 076532f Tighten earliest-release-wins: require commit-in-tag proof, extend to PRs Earlier commits from PR #35858 (b4984c3, af5f8a3, 47669e7, 5e341ad) were already in this branch via the previous merge 0b0a152. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…R-side tristate Addresses adversarial review findings from Opus / GPT-5.5 / Gemini-3.1-Pro: 1. CRITICAL — Get-TagsForMilestone derives major from $Milestone arg, not from $script:validationMajor. Pre-fix, validating a net11.0 PR with an issue currently milestoned '.NET 10 SR6' filtered out every 10.x tag (because validationMajor=11), making Test-PrShippedInMilestone return false, which silently clobbered SR6 — the exact data-loss case the KEEP guard exists to prevent. (Opus) 2. CRITICAL — Restricted Get-LinkedIssues and Test-MilestoneValidForIssue cross-repo prefix from [A-Za-z0-9_\-./]+ to '(?:dotnet/maui)?'. The old regex silently matched 'Fixes dotnet/runtime#42' and would have closed unrelated MAUI #42. Mirrors GitHub's own auto-close scope. (GPT-5.5 + Gemini-3.1-Pro) 3. Replaced two-query foreach in Test-MilestoneValidForIssue with a single OR query ('#N in:title,body OR issues/N in:body'). Halves Search API consumption (30 req/min limit was hit after 16 issues) and eliminates the 'discard positive evidence on partial failure' trap. Also removed $null caching so a transient failure no longer permanently disables KEEP for that pair. (Gemini-3.1-Pro + Opus) 4. PR-side tristate: Get-PrsInTag returns $null on git failure (was @()); Test-PrShippedInMilestone propagates that through to Test-MilestoneValidForPr. Test-AndRecordCorrection already had defensive $null handling for issues; now it applies equally to PRs (cherry-pick KEEP path). One transient git failure no longer silently clobbers a valid earlier milestone. (Opus) Also added unary-comma fix to Get-PrsInTag's HashSet return so PowerShell doesn't unwrap single-element collections (caller relies on .Contains()). Tests: 134 → 149 (+15). New coverage: - Get-TagsForMilestone cross-major behavior (the missing test that hid the bug) - Three NEGATIVE cases for cross-repo regex (dotnet/runtime, xamarin, mixed) - Single OR query string assertion + caching test updated to 1 call - Test-PrShippedInMilestone tristate ($true / $false / $null cases) - Test-AndRecordCorrection PR-side $null → skip correction Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…h limit
Addresses two round-3 adversarial review findings:
❌ CRITICAL (GPT-5.5) — Get-PrsInTag's cache-hit early return was missing the
unary-comma that the fresh-read path uses. On the second lookup for the same
tag, PowerShell unwrapped the cached HashSet:
- 1-element cache → Int32 → caller's $prs.Contains($n) throws
- 0-element cache → $null → mistaken for git failure → spurious 'uncertain'
This is the production-path hot loop (issue/PR validation reuses the same
milestone tags across many checks). Fix: 'return ,$script:tagPrCache[$Tag]'.
💡 (Gemini-3.1-Pro) — Test-MilestoneValidForIssue's gh search was capped at
--limit 50. For a heavily-referenced issue whose fixing PR sorts past position
50, the validator would silently return $false and clobber a valid earlier
milestone. Raised to 100 (gh search per-page max).
Tests: 149 → 152 (+3). New coverage:
- Test-PrShippedInMilestone definitive answer on cache hit, both 1-element
and 0-element cases.
- Mock-called-once assertion to prove the cache is actually being hit.
Opus reviewer also weighed in (result unfortunately lost before retrieval);
GPT and Gemini both LGTM'd round 2 apart from these two findings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Multimodal re-review @
|
| # | Finding | Status | Evidence |
|---|---|---|---|
| A | Validation context not seeded in single-PR mode | ✅ Resolved | Fix-MilestoneDrift.ps1:979-980 seeds before the Test-AndRecordCorrection calls at :1022/:1030; regression test Tests.ps1:1041-1053 asserts Major=11 + tags reach the initializer |
| B | Get-MilestoneSortKey missed the .NET <N>.0 SR… form |
✅ Resolved | shared/MauiReleaseVersioning.psm1:308,316,320 accept optional .0; pwsh: Get-MilestoneSortKey '.NET 10.0 SR1' → 10410, Compare-MauiMilestone '.NET 10.0 SR1' '.NET 11.0-preview3' → -1 |
| C | Validation failed open on gh errors |
✅ Resolved | Now tri-state $null (:658,665,711), not cached on failure (:658), and the caller skips via if ($null -eq $isValidEarlier) (:798) before the truthy check (:806) — the only correct PS idiom |
| D | Search missed URL-form / title-only links | ✅ Resolved | Query broadened to #$N in:title,body OR issues/$N in:body (:645); verb regex re-checks both forms on title+body (:685-686) |
| E | Kept bucket not deduped |
✅ Resolved | _keptItems HashSet keyed ItemType:Number (:812-817) |
| F | Get-LinkedIssues dropped dotnet/maui#N |
✅ Resolved | Regex now (?:dotnet/maui)?#(\d+) (:356) and deliberately rejects other owner/repos so a Fixes dotnet/runtime#N can't clobber a MAUI issue — nice touch |
Other things all three reviewers independently cleared: the ,$set unary-comma cache idiom round-trips the HashSet on both hit and miss (incl. 0-element sets) under StrictMode; the pull_request_target surface is injection-safe (gh args are splatted, the issue number is a typed [int], BaseRef is the trusted base branch, [[ "$PR_BASE_REF" == net*.0 ]] rejects main/release/*/inflight/*/empty); and Save-ReportJson's [object[]] cast survives empty Kept/Corrections.
Optional polish (non-blocking)
1. Drop the review-process labels from the test names (Fix-MilestoneDrift.Tests.ps1). Several Describe blocks bake the review round/finding into the name:
:901… (PR #35858 findings A, C, E):1003… (PR #35858 finding A):1056… (PR #35858 round-2 critical finding):1099… (PR #35858 round-2 finding):1156… (PR #35858 round-2):1191… (PR #35858 round-3 finding)
The behavioral half of each name is great; the round-2/round-3/finding A suffix is process noise that git/PR history already records. Suggest naming by behavior only, e.g. Test-PrShippedInMilestone — returns $null (uncertain) on git failure.
2. Manual tag-mode + -CloseFixedIssues + -Apply has a wide blast radius. The close_fixed_issues workflow_dispatch input is appended regardless of mode (fix-milestone-drift.yml:93-95), so a manual dispatch with a tag would call Close-LinkedIssue for every linked open issue across the whole PrevTag..ReleaseTag range (Fix-MilestoneDrift.ps1:1139-1140). I can see this is deliberate (the validation table shows per-tag bulk closes for net11.0 previews) and the already-closed no-op caps the damage — but the input description ("use for PRs merged to non-default branches like net11.0") reads as single-PR-only. Consider either restricting the input to net*.0 tags / single-PR, or expanding the description to call out the tag-mode bulk-close behavior.
3. Nits:
INPUT_PR_NUMBER(string) binds to[int]$PrNumber; a non-numeric manual input throws an unfriendly parameter-binding error. A[[ "$INPUT_PR_NUMBER" =~ ^[0-9]+$ ]]guard in the bash block fails fast. (The auto-trigger path always supplies a valid int.)- The OR-combined
gh searchquery (:645) is asserted only as a string in the unit test; one reviewer confirmed live that the shape returns matches, and the file'sLIVE VALIDATION GUIDEis the established pattern here — worth adding a URL-form case to that guide. Get-MilestoneSortKeyonly collides at values that don't occur in MAUI's cadence (SR ≥ 71, or sub-patch ≥ 10) — noting for completeness, not actionable.- Description says "91 → 118 tests"; the suite is now 152 green — minor description refresh.
Methodology
Three independent reviewers (GPT-5.5, Gemini 3.1 Pro, Claude Opus 4.7), each given the full current source, pwsh 7.5.4, and the prior A–F findings, tasked to verify resolution and adversarially hunt for regressions introduced by the fix rounds. Consensus: A–F resolved, no new criticals (2/3 found nothing new; the Opus pass surfaced items 2–3). Finding 1 is from the maintainer pass. Every claim was spot-checked against the code and a local Invoke-Pester run (152/152 passing).
kubaflo's re-review (PR comment 2026-06-15) confirmed all six prior
A-F findings resolved and the PR mergeable, with three optional polish
items. Addresses them:
1. Drop the review-process labels from test names — keeps the behavioral
half (which is informative) and drops the round/finding suffix
(process noise that git history already records). Touches 4 'It'
blocks and 6 'Describe' blocks in Fix-MilestoneDrift.Tests.ps1.
2. Expand the workflow_dispatch close_fixed_issues input description
to call out the tag-mode bulk-close blast radius. Previously read
as PR-only; now explicitly labels [PR-mode] vs [Tag-mode] semantics
so a manual operator can see the fan-out cost before dispatching.
3a. Add a numeric guard for the manual pr_number input so a non-numeric
value fails with a clear ::error:: message instead of an opaque
PowerShell parameter-binding error from the [int]$PrNumber binding.
3b. Add an 8th dry-run example to the LIVE VALIDATION GUIDE covering
the URL-form linked-issue case (Fixes https://github.com/.../issues/N).
The unit test asserts the OR query as a string; this guide entry
covers the live behavior so a regression in the URL branch surfaces.
Also refreshed the PR description's stale '91 → 118 tests' to the
current 152.
Tests: Invoke-Pester → 152/152 passing. No behavioural change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3-reviewer adversarial review (Sonnet/GPT/Gemini) of round-4 converged
on two genuine defects:
1. Item 1 cleanup was incomplete. 6 test names still carried process
noise:
- line 345: '(PR #35858 follow-up finding)'
- lines 939, 956: 'finding E:' prefix
- lines 968, 983: 'finding C:' prefix
- line 996: 'findings A + E together:' prefix
All 6 now reworded to keep the behavioral half and drop the
review-process label. Verified clean via grep.
2. Item 3b URL-form guide entry was misleading on two counts (all three
reviewers caught the underlying issue, Sonnet pinpointed both):
a) Claimed it covers 'Test-MilestoneValidForIssue branch', but the
validator is only called when Compare-MauiMilestone returns
non-zero — and the example inputs (PR 35662 / issue 35615) share
a milestone, so Test-AndRecordCorrection short-circuits before
the validator runs. What -PrNumber 35662 actually exercises is
Get-LinkedIssues' URL-form parser (the kubaflo finding D path).
b) Cited a regression sentinel ('Linked issues: (none)') that does
not exist anywhere in Fix-MilestoneDrift.ps1. An operator would
look for a phantom string and miss a real URL-branch regression.
Rewrote the entry to:
- Accurately describe what it tests (Get-LinkedIssues URL parser,
not Test-MilestoneValidForIssue).
- Use the actual sentinel 'Issues checked: 1' (verified by running
the dry-run against /Users/shneuvil/Projects/maui — output was
exactly 'Issues checked: 1').
- Point the reader at the existing Pester test for the validator's
URL-form OR query.
Reviewers also flagged INFO items not addressed here:
- Numeric guard allows '0' through. 0 is a non-existent PR; downstream
Get-PrInfo will warn and skip. Practical risk = nil; not worth
adding another rule.
- Workflow description is 316 chars vs 61-85 for other inputs.
Functional, just wraps in the dispatch UI tooltip. The blast-radius
warning is the point — losing it to fit a line length defeats the
purpose.
- PR description's 'Tests' section enumerates per-helper counts that
are now stale (e.g. '4 tests for Test-MilestoneValidForIssue').
The aggregate '152 tests' is correct; per-helper enumeration is
historical and will keep drifting. Not fixing now.
Tests: 152/152 passing. Empirically verified the new guide entry's
'Issues checked: 1' claim by executing the dry-run against the live
repo.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round 4 + 5 — kubaflo polish + adversarial review of the polish Thanks @kubaflo for the multimodal re-review. All three optional polish items addressed, then I ran an adversarial 3-model review of my own fixes (Sonnet 4.6 / GPT 5.4 / Gemini 3.1 Pro). All three reviewers converged on two genuine defects in my round-4 commit ( Item 1 (process-noise test names) — round-4 cleaned the 10 sites you enumerated, but adversarial review caught 6 more I missed in the same file (line 345 Item 2 (workflow input description) — expanded with explicit Item 3a (numeric guard) — added Item 3b (URL-form live-validation example) — round-4 added an 8th guide entry pointing at PR #35662 / issue #35615 with a misleading claim. All three reviewers caught it:
Round-5 rewrites the entry to (a) accurately describe what Item 3c (PR description) — refreshed Test status: 152/152 Pester passing on The adversarial-of-the-adversarial pattern (running 3 independent models against my own fix commit) caught real defects the original author + first-pass reviewer both missed. Documenting this loop in case it's useful for |
…ersedes #35754) Folds #35754's net11-preview-readiness work into the existing release-readiness skill so dotnet/maui has ONE deterministic readiness pipeline that handles both Servicing Releases (SR) and Previews — in both in-flight and candidate (pre-cut) modes. Changes ======= Find-ReleaseReadinessTrackers.ps1 - Four-lane detector now covers SR + Preview branches grounded in the tag-existence rule (a release is in-flight if and only if its expected tag has not been published). - -AllActiveMajors enumerates every active major (current + lower in-flight) in one envelope, ready for matrix expansion. - Trackers always advertise a canonical proposed branchName (even for candidates that have not been cut yet); a new branchExists flag is the explicit signal for whether the branch is on origin. Get-PreviewReadiness.ps1 (new, ~720 lines) - Port of #35754's net11 readiness adapted to the skill structure. - Takes -Branch + -Mode {in-flight,candidate} + -SurveyRef; candidate mode surveys net<major>.0 with the inflight-bump check skipped. - Embeds canonical idempotency markers (release-readiness-tracker / -flavor / -mode) so daily issue updates can join on the canonical key. - Preserves the human-notes block between marker fences across re-runs. release-readiness.yml workflow (new) - Weekday 08:30 UTC cron + workflow_dispatch + PR validation. - detect-trackers job emits a matrix from Find-Trackers -AllActiveMajors; one matrix job per tracker dispatches to Get-ReleaseReadiness (SR) or Get-PreviewReadiness (preview) based on branchType. - Idempotent issue handling: look up an open tracker issue by the canonical marker, edit it in place; otherwise create one with report / s/triaged / area-release-readiness labels. - Activity gate: when recentCommitCount == 0 AND no open tracker issue exists, skip new-issue creation (existing open issues are still refreshed). - PR-trigger validate job runs the test suite and smoke-runs every detected tracker against the report scripts; never touches issues. SKILL.md - Documents the SR + Preview entry points, the tag-existence rule, the daily workflow, the branchExists / branchName contract, and the shared MauiReleaseVersioning.psm1 dependency. Tests - Release-readiness suite: 273/273 passing (added preview-lane unit tests + -AllActiveMajors E2E; updated for SR7-shipped live state and the new branchExists contract). - Fix-MilestoneDrift Pester suite: 118/118 passing (validates that the earliest-release-wins milestone logic merged in from #35858 still works after this branch's edits). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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!
Summary
Three related, surgical changes to
.github/scripts/around MAUI release/milestone tooling:MauiReleaseVersioningPowerShell module fromFix-MilestoneDrift.ps1so other release tooling can reuse the same version/milestone helpers.-CloseFixedIssuesflag toFix-MilestoneDrift.ps1(and wire it into the workflow) so issues fixed by PRs merged to non-default branches likenet11.0actually get closed..NET 10 SR6) just because a later follow-up PR onnet11.0also fixes it.No behavior change unless you opt into the new flag, and the new validation only ever turns a would-be mutation into a no-op — it can never create writes that didn't exist before.
Motivation (net11.0 close-issue gap)
GitHub only auto-closes "fixes #NNN" linked issues for PRs merged to the default branch (
mainfordotnet/maui). When work targetsnet11.0(the .NET 11 development branch), the PR merges fine and the milestone gets set, but the linked issue stays open — so the team has to manually close issues for every fix shipped onnet11.0.This PR adds a deliberate, opt-in closing action. The auto-trigger turns it on automatically only when the PR's base ref matches
net*.0, and leavesmain,release/*, andinflight/*alone (those either already auto-close or need human judgment).Motivation (earliest-release-wins)
When a bug was first fixed in (say)
.NET 10 SR6and a follow-up clean-up landed onnet11.0, the linked issue is correctly milestoned to.NET 10 SR6to record where the fix first shipped. Running the auditor against an11.0tag was then re-milestoning that issue to.NET 11.0-previewN, erasing the "first shipped in" signal.The fix: before overwriting an issue's existing real-release milestone, verify a fix-linking PR in the current milestone actually exists. If yes, leave it alone (KEEP). If the current milestone is
null/Backlog/Planningor a later release than the target, behavior is unchanged.Commit 1 — Extract shared
MauiReleaseVersioningmodule.github/scripts/shared/MauiReleaseVersioning.psm1exporting helpers used across release tooling (Get-CurrentMajorVersion,Get-MainBranchForVersion,Get-VersionFromGitRef,ConvertTo-Milestone,ConvertBranchToMilestone,Get-TagSortKey,Find-PreviousTag)..github/scripts/Fix-MilestoneDrift.ps1— removes the in-script copies of those helpers andImport-Modules the new psm1 instead.Set-StrictMode -Version Latestinternally (import doesn't leak strict mode out, unlike dot-sourcing) and has an explicitExport-ModuleMemberblock.[AllowEmptyString()][AllowNull()]on string params for back-compat with existing positional-parameter tests.Commit 2 —
-CloseFixedIssuesflagScript (
Fix-MilestoneDrift.ps1)[switch]$CloseFixedIssuesparameter (default off; behavior unchanged when not passed).Close-LinkedIssuefunction:gh issue view --json state,number,title.-Apply→gh issue close N --reason completed --comment "Closed by #PR (merged to <baseRef>). GitHub only auto-closes for PRs merged to the default branch; this issue was fixed by a PR merged to a non-default branch.".-Apply→ dry-run path (log only, noghcall).ghfailures → warn and continue, do not throw (one bad issue mustn't fail the whole script).Invoke-GhClithin wrapper aroundghso Pester can mock CLI calls without spawning real processes.Get-LinkedIssuesregex (no duplicatedfix(es|ed)?|close[sd]?|resolve[sd]?parsing).Invoke-AnalyzeSinglePr) and the tag-based audit path (Invoke-AnalyzeRelease), gated on-CloseFixedIssues.Workflow (
fix-milestone-drift.yml)close_fixed_issuesworkflow_dispatchinput (boolean, defaultfalse).pull_request_targetauto-trigger,-CloseFixedIssuesis appended automatically whengithub.event.pull_request.base.refmatchesnet*.0:main,release/*, orinflight/*.-CloseFixedIssueswheninputs.close_fixed_issues == 'true'.Commit 3 — Earliest-release-wins milestone validation
Shared module additions
Get-MilestoneSortKey— converts a milestone name to a sortable integer (major * 1000 + phase), where phase:preview = 100+N,rc = 200+N, GA =300, SR =400 + (sr * 10) + sub. Returns$nullfor non-release buckets like Backlog/Planning/Future.Compare-MauiMilestone— returns-1 / 0 / 1(earlier / equal / later) using the sort key.Script (
Fix-MilestoneDrift.ps1)Test-MilestoneValidForIssue— for an issue already on a real earlier release, searchesgh search prs "#$n in:body"for a PR with a fix verb (fixes/closes/resolves #N) that itself sits on the current milestone. Hit → milestone is "valid earlier", KEEP. Miss → fall through to the existing apply path.$script:milestoneValidationCachekeyed by"$IssueNumber|$Milestone"so the same issue checked via multiple linking PRs only hitsghonce.Keptbucket to the report — surfaced in console output, JSON report (kept[]+summary.kept_earlier), and the rolled-up GitHub issue summary.[object[]]$keptItems = @()then conditional assign inSave-ReportJsonso a report with zero KEEPs no longer collapses to$nullunder StrictMode.Tests
Added Pester coverage for everything new — totals went from 91 → 152 tests across the three review rounds, all green.
Close-LinkedIssue(already-closed both cases, dry-run, apply with comment text check,gh view/closefailures, empty BaseRef).Get-MilestoneSortKey(each phase, sub-revisions, Backlog/Planning/Future return$null, malformed input).Compare-MauiMilestone(-1/0/1 across phases,$nullhandling).Test-MilestoneValidForIssue(valid earlier KEEP, no linking PR → fall through,ghfailure → fall through, cache hit).How to verify
Production validation
Ran the new tooling live across every shipped
net11.0preview tag — full PR/issue milestone audit + opt-in closes + earliest-release-wins:11.0.0-preview.511.0.0-preview.411.0.0-preview.311.0.0-preview.211.0.0-preview.1The 2 KEEPs on preview3 (
#34490on.NET 10 SR6,#31280on.NET 10 SR7) were both manually verified as correct: each issue was first fixed by a main → SR PR and the net11.0 PR was a follow-up — exactly the case the validation is designed to catch.