Fix milestone workflow crash when the target milestone doesn't exist yet - #36398
Conversation
The single-PR skip path in Invoke-AnalyzeSinglePr returns an early report when the expected milestone hasn't been created in GitHub yet, but that hashtable omitted the ResolvedMilestone/ResolvedMsNumber keys. Write-Report and Save-ReportJson read $Report.ResolvedMilestone without a ContainsKey guard, so under StrictMode Latest (enabled during normal execution) the missing key throws PropertyNotFound and the workflow exits 1 on every merge targeting a net11.0 branch (expected milestone .NET 11.0-preview7 does not exist yet). Add ResolvedMilestone/ResolvedMsNumber (set to $null) to the skip report so it matches the success-path report shape and downstream writers no longer crash. Add regression tests that drive the milestone-not-found path and faithfully reproduce the CI crash under Set-StrictMode -Version Latest. 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 -- 36398Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36398" |
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the Milestone Management automation when the expected GitHub milestone hasn’t been created yet (e.g., preview milestones on net11.0). The change ensures the “graceful skip” path returns a report object with the same shape as the success path, preventing Set-StrictMode -Version Latest from turning the skip into a hard failure.
Changes:
- Add
ResolvedMilestoneandResolvedMsNumber(both$null) to the early-return skip report inInvoke-AnalyzeSinglePr. - Add Pester coverage for the “milestone not found” skip path, including a StrictMode reproduction that exercises
Write-ReportandSave-ReportJson.
Show a summary per file
| File | Description |
|---|---|
| .github/scripts/Fix-MilestoneDrift.ps1 | Ensures the skip report includes ResolvedMilestone/ResolvedMsNumber keys so report writers don’t throw under StrictMode. |
| .github/scripts/Fix-MilestoneDrift.Tests.ps1 | Adds regression tests validating the skip report shape and verifying no StrictMode crash in the report writers. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
kubaflo
left a comment
There was a problem hiding this comment.
Note
🔍 AI-generated review — independent 3-model panel (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro) then cross-pollinated, run on @kubaflo's behalf. A human maintainer makes the final call.
✅ R1 — UNANIMOUS LGTM (high)
All three models independently reached LGTM/high, and Opus empirically verified the fix end-to-end (not just by reading).
What it fixes
Invoke-AnalyzeSinglePr's missing-milestone early-return report omitted ResolvedMilestone / ResolvedMsNumber. Under Set-StrictMode -Version Latest, downstream Write-Report (Fix-MilestoneDrift.ps1:1326/1328) and Save-ReportJson (:1383) dot-read $Report.ResolvedMilestone unguarded → The property 'ResolvedMilestone' cannot be found on this object → the Milestone Management workflow crashed on every merge (exit 1) while .NET 11.0-preview7 doesn't exist yet. The 2-line fix makes the skip report key-for-key identical to the success report.
Verification (Opus, isolated harness @ head, pwsh 7.5.4 / Pester 5.7.1)
- Reproduced the exact CI crash under StrictMode.
- Reverting only the 2 added lines → both new tests fail (Test 2 throws at the real crash site,
Write-Report:1326); with the fix → 200/200 pass, 0 regressions. - Enumerated every unguarded
$Report.*read on the skip path — all now satisfied.
Cross-pollinated findings
- Root cause: correct & complete (
ResolvedMilestonestrictly required;ResolvedMsNumbernever read but correct for shape parity). ✅ - Behavioral safety: graceful exit 0; happy path,
-Apply,-CloseFixedIssuesall unaffected; release/tag mode still throws loudly for a truly-missing milestone (right fail-open vs fail-loud asymmetry). ✅ - Blast radius: effectively zero — only the
if (-not $match)skip branch changes. ✅ - Tests: two complementary tests (StrictMode-independent structural + faithful StrictMode repro). ✅
CI
maui-pr is skipping (by-design .github/** path-exclusion); gh-aw checks (agent/detection/safe_outputs/conclusion) pass. No workflow runs this PS1's Pester suite — so this review is the effective gate (Opus ran it: green).
💡 Non-blocking suggestions
- Test mock:
Get-AllMilestonesreturns an array rather than the production title→number hashtable — harmless here sinceFind-MatchingMilestoneis mocked to$null, but matching the real shape would be more faithful. - Consider centralizing skip/success report construction to prevent future shape drift, and optionally add a Pester CI job for these
.github/scriptstests.
Verdict: LGTM (high, unanimous, empirically verified). ● Approving.
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!
Symptom
The Milestone Management workflow (
.github/workflows/fix-milestone-drift.yml→.github/scripts/Fix-MilestoneDrift.ps1) auto-sets a PR's milestone on every merge via thepull_request_targetpath (Fix-MilestoneDrift.ps1 -PrNumber <N> -Apply [-CloseFixedIssues]).Right now every merge to a
net11.0branch crashes the workflow because the expected milestone.NET 11.0-preview7does not exist in GitHub yet (only.NET 11.0-preview6exists). Several recent runs failed with the identical error:This is a pre-existing latent bug introduced by the original milestone automation (#34686) — not by the recent tag-trigger change (#36140). It only surfaces now because the next preview milestone hasn't been created yet.
Root cause
In
Invoke-AnalyzeSinglePr, when the target milestone isn't found, the function logsNo GitHub milestone found … Skipping.and returns an early "skip" report hashtable whose intent (per its own comment) is to "return empty report — prevents red CI on every auto-triggered merge."But that early-return hashtable omitted the
ResolvedMilestone/ResolvedMsNumberkeys that the success-path report carries. The caller then runs:Write-Report $report, which reads$Report.ResolvedMilestoneunguarded (noContainsKeycheck), andSave-ReportJson, which reads$Report.ResolvedMilestoneunguarded too.During normal execution the script enables
Set-StrictMode -Version Latest, under which reading a missing hashtable key throwsPropertyNotFound→ exit 1. So the intended graceful-skip is defeated by the report writer, turning a "milestone not created yet" no-op into a hard CI failure.The tag/release-mode path (
Invoke-AnalyzeRelease) intentionally throws when a milestone is missing, and is left unchanged — for tag mode, failing loudly is correct. Only the single-PR skip path is fixed here.The fix
Add the two missing keys (set to
$null) to the early-return hashtable inInvoke-AnalyzeSinglePrso it matches the success-path report shape:This satisfies every unguarded downstream accessor.
Invoke-ApplyCorrectionsiterates the emptyCorrectionslist (no-op) andNew-GitHubIssueonly runs whenCorrections.Count > 0, so no other changes are needed.Regression test
Added tests to
.github/scripts/Fix-MilestoneDrift.Tests.ps1covering the milestone-not-found path.There's a StrictMode subtlety worth calling out: StrictMode is intentionally not enabled when the script is dot-sourced for Pester (guarded by
$MyInvocation.InvocationName -ne '.'). A test that merely callsWrite-Reporton a report missing the key would not throw under Pester and would pass even with the bug present (false negative). So the tests do both:Invoke-AnalyzeSinglePrdown the milestone-not-found path (mockFind-MatchingMilestone→$null) and assert the returned report.ContainsKey('ResolvedMilestone')and.ContainsKey('ResolvedMsNumber')are$true(and reading.ResolvedMilestoneyields$null).Set-StrictMode -Version Latest, then callWrite-ReportandSave-ReportJsonon that report and assert it does not throw — reproducing the exact crash condition.Before/after proof (reverting only the 2-line fix):
The property 'ResolvedMilestone' cannot be found on this objectfromFix-MilestoneDrift.ps1:1326(Write-Report), exactly matching the CI crash.MilestoneTrigger.Tests.ps1+Fix-MilestoneDrift.Tests.ps1).Note
Creating the
.NET 11.0-preview7milestone in GitHub is a separate operational unblock — orthogonal to this code fix. This change ensures the workflow degrades gracefully (as originally intended) whenever the expected milestone hasn't been created yet, instead of crashing every merge.