Run milestone management automatically on release tag push - #36140
Conversation
Adds a push trigger to the Milestone Management workflow so that when a stable release tag (e.g. 10.0.80) is pushed, the workflow audits the entire tag cohort and applies milestone fixes, closing issues fixed by PRs that shipped in that tag. - New on.push.tags filter matches MAJOR.MINOR.PATCH only and is anchored to the full ref, so preview tags (11.0.0-preview.*) are excluded. - Job if now also runs for push events. - Run step gains a push branch that invokes tag-mode with -Apply -CloseFixedIssues, plus a defense-in-depth regex guard that rejects any ref that isn't a stable release tag. This is the same "-Tag <tag> -Apply -CloseFixedIssues" invocation already validated via manual workflow_dispatch. 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 -- 36140Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36140" |
Tags pushed using the default GITHUB_TOKEN do not trigger workflow runs. Add an inline note so a future change to tag-creation automation doesn't silently disable this trigger. Surfaced by adversarial multi-model review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial multi-model code review — PR #36140
Methodology: 3 independent reviewers (different models) reviewed this change in parallel, then findings were reconciled by adversarial consensus (agree / dispute / discard). Consensus strength is noted per finding as "X/3 reviewers". This review event is COMMENT — approval is a human decision.
Summary
Substantially clean. The script-injection surface is handled correctly: github.ref_name reaches the script only via the PUSH_TAG env var (never ${{ }}-interpolated into run:), is passed as a discrete bash-array element, and is re-validated with an anchored regex. The tag filter behaves as designed. The single ❌ "critical" finding turned out to be a false positive (below).
Findings
1. ❌→✅ Discarded — false positive (1/3): "tag glob never matches." One reviewer asserted GitHub Actions globs treat + as a literal, making the filter a no-op. The official filter-pattern cheat sheet and the other two reviewers confirm + = "one or more of the preceding character" and . = literal, anchored to the full ref — so [0-9]+.[0-9]+.[0-9]+ matches 10.0.80 and excludes previews. The reviewer confused GH Actions glob with POSIX shell glob. See inline note on the filter line. No action.
2. -Apply -CloseFixedIssues on every matching tag push. All three flagged the bulk, irreversible mutation surface (milestone moves + issue closures across the whole PrevTag..ReleaseTag cohort) with no dry-run gate. This is the deliberate posture you chose and it was validated live against 10.0.80, so it's by-design — but a tag-protection ruleset on [0-9]+.[0-9]+.[0-9]+ (optionally plus a github.ref_protected job gate) would bound the risk of a stray X.Y.Z tag firing the apply path. See inline. Intentionally not auto-applied — caveat in the inline comment.
3. ✅ Addressed (3/3): default GITHUB_TOKEN won't trigger this. All three noted that a tag pushed by the default GITHUB_TOKEN won't start a new run. Today SR tags are pushed by a maintainer, so it fires. Fix pushed (74da2ff): added an inline NOTE so a future change to tag-creation automation can't silently regress this.
4. 💡 Backport reality (1/3): workflow is read at the tagged commit. Tag-push runs the workflow as it exists at the tagged commit. Since SR tags are cut from release/* branches, this trigger only takes effect once the change flows into those branches; it won't run for tags on older commits. Already disclosed in the PR description — noted for completeness.
5. 💡 Concurrency (1/3): no concurrency: group. Back-to-back tag pushes could run concurrently. One reviewer raised it; another explicitly probed it and found no overlapping-range hazard (operations are idempotent). Low priority.
Fixes pushed in this review
74da2ff— documented theGITHUB_TOKENtag-push limitation inline (finding #3).
Test coverage
No automated tests accompany this change, which is reasonable: the modified file is workflow YAML only; the underlying Fix-MilestoneDrift.ps1 (with offline Pester tests) is unchanged. Behavior was validated empirically instead — dry-run + apply runs against tag 10.0.80 produced exactly 2 milestone corrections and 2 issue closures, matching live GitHub state.
Prior reviews
None to reconcile — no prior review bodies, inline comments, or human-review findings (only an automated dogfood-bot comment).
CI (non-gating for this skill)
license/cla pass; maui-pr is skipping for a workflow-config change, so CI coverage is undetermined — not a regression signal for YAML-only changes.
Verdict: COMMENT. No must-fix defects. Injection handling and the tag filter are correct; the one safe consensus fix is already pushed. Remaining items (blast-radius hardening via tag protection) are deliberate-posture calls left to you.
| # silently never fire (today SR tags are pushed by a maintainer, so it does). | ||
| push: | ||
| tags: | ||
| - '[0-9]+.[0-9]+.[0-9]+' |
There was a problem hiding this comment.
✅ Tag glob verified correct — one reviewer mis-flagged this as a no-op (false positive, resolved).
A reviewer claimed this pattern can never match because GitHub Actions globs treat + as a literal. That's wrong: the filter-pattern cheat sheet defines + as "matches one or more of the preceding character" and . as literal, anchored to the full ref. So [0-9]+.[0-9]+.[0-9]+ matches 10.0.80 and excludes 11.0.0-preview.5.26304.4. The reviewer confused GH Actions glob with POSIX shell glob; the other two reviewers + the docs confirm correctness. The bash ^[0-9]+\.[0-9]+\.[0-9]+$ guard below is correct defense-in-depth. No change — noting so the doubt is on record as resolved.
| echo "::error::push tag '$PUSH_TAG' is not a stable release tag (expected MAJOR.MINOR.PATCH)" | ||
| exit 1 | ||
| fi | ||
| ARGS+=('-Tag' "$PUSH_TAG" '-Apply' '-CloseFixedIssues') |
There was a problem hiding this comment.
-Apply -CloseFixedIssues on every matching tag push (3/3 reviewers)
All three reviewers independently flagged that this fires bulk, irreversible issue closures + milestone moves on any push of a MAJOR.MINOR.PATCH tag, with no dry-run gate. Tag-mode -CloseFixedIssues closes every linked open issue across the whole PrevTag..ReleaseTag cohort. This is your deliberate posture (validated live against 10.0.80), so flagging for visibility — not as a blocker. Two mitigations worth considering:
- a tag-protection ruleset on
[0-9]+.[0-9]+.[0-9]+so only trusted release automation/maintainers can create matching tags; - optionally gating the job on
github.ref_protected == true.
I deliberately did not add the ref_protected gate here: if matching tags aren't actually marked protected in the repo ruleset, that gate would silently disable the trigger entirely — your call, since you know the tag-ruleset config. As-is, a stray tag like 1.2.3 on the wrong commit would fire the full apply path.
kubaflo
left a comment
There was a problem hiding this comment.
Note
🤖 AI-generated approval (multi-model review automation, approving on @kubaflo's behalf).
✅ Approving — 3-model LGTM.
Adds a push: tags trigger so a stable release-tag push runs milestone management in tag-cohort mode (-Tag <tag> -Apply -CloseFixedIssues), auditing the cohort and closing issues fixed by PRs that shipped in the tag. Clean, well-documented, correctly gated.
Verified:
- The tag glob
[0-9]+.[0-9]+.[0-9]+is correct. GitHub Actions filter patterns support+= "one or more of the preceding character" (the docs' canonical examplev[12].[0-9]+.[0-9]+matchesv1.10.1/v2.0.0), so this matches10.0.80/10.0.100and excludes preview tags like11.0.0-preview.5...(a filter pattern must match the whole ref). One reviewer was initially unsure here; cross-checking GitHub's cheat sheet resolved it — all 3 models agree it matches. - Defense-in-depth step-level re-validation
^[0-9]+\.[0-9]+\.[0-9]+$backs up the glob. if:gating runs on push / dispatch / merged-PR only.- Auto-close is fenced by existing script safety gates (merge cutoff, version-branch filter, already-closed no-op, breadcrumb, warn-not-fail).
Non-blocking suggestions (no change required):
- GITHUB_TOKEN no-op caveat — you correctly document that a default-token tag push won't trigger this; worth a one-time confirmation SR tags are pushed by a human/PAT/App so it doesn't silently no-op.
- First run auto-applies/closes — tag-mode passes
-Apply -CloseFixedIssuesunconditionally (no first-run dry-run); worth eyeballing the first real SR-tag run. - Unrestricted major (see inline) — optionally pin to supported majors.
A stray or accidental MAJOR.MINOR.PATCH tag (e.g. 1.2.3, 7.0.0) would otherwise fire the bulk -Apply -CloseFixedIssues path. Pin the glob to 1[01].x.x so only currently-supported majors trigger; preview/rc tags remain excluded at every major via the full-ref match. Suggested by multi-model review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Milestone Management GitHub Actions workflow so it runs automatically when a stable release tag is pushed, in addition to the existing PR-merge (pull_request_target) and manual (workflow_dispatch) entry points.
Changes:
- Add a
pushtag trigger intended to run milestone drift auditing automatically on stable release tag pushes. - Expand the job-level
if:so the job runs forpushevents as well as manual dispatch and merged PR closures. - Add a
pushbranch in the run-step logic to invoke tag-mode (-Tag ... -Apply -CloseFixedIssues) with an additional safety guard.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/fix-milestone-drift.yml | Adds a tag-push trigger and corresponding run-step branch to execute milestone management automatically on stable release tag pushes. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
| push: | ||
| tags: | ||
| - '1[01].[0-9]+.[0-9]+' |
| if [[ ! "$PUSH_TAG" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
| echo "::error::push tag '$PUSH_TAG' is not a stable release tag (expected MAJOR.MINOR.PATCH)" | ||
| exit 1 | ||
| fi |
Expand the `on.push.tags` filter so the Milestone Management workflow also
fires for preview and rc release tags, not just stable SR/GA tags. The
PowerShell script and shared MauiReleaseVersioning module already fully
support preview/rc tag-mode (milestone mapping, sort keys, previous-tag
lookup); only the workflow trigger was restricting to stable tags.
Changes:
- on.push.tags: add major-pinned preview (`-preview.*`) and rc (`-rc.*`)
patterns alongside the stable pattern. Still pinned to majors 10/11 to
bound blast radius.
- bash guard: broaden the shape regex to accept the optional
`-preview.N` / `-rc.N` prerelease suffix (with numeric build tail).
Guard stays major-agnostic; the glob is the major gate.
- comment block: document that preview/rc are now included.
Add .github/scripts/MilestoneTrigger.Tests.ps1 (100 Pester tests):
- a documented GH-glob->regex translator (itself unit-tested) driving a
large match/no-match fixture of real stable/preview/rc tags plus
out-of-range majors and malformed shapes;
- the bash guard regex extracted verbatim from the YAML and exercised
through real `bash` against valid tags and shell-injection payloads;
- structure/injection-safety invariants (push trigger wired, tag flows
via PUSH_TAG env, run: body never interpolates ${{ github.* }}).
Validated end-to-end with a dry-run against the current 11.0.0-preview.5
tag: resolves ".NET 11.0-preview5", checks 22 PRs, skips 123 merge-ups,
clean exit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
left a comment
There was a problem hiding this comment.
🤖 AI-generated review — multi-model panel (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro), independent reviews + cross-pollination. Round 2.
✅ LGTM — unanimous (high confidence)
This round's single commit (da0aee65, "Pin tag trigger to supported majors (10, 11)") directly implements Round-1 suggestion #3 — bounding the blast radius of the bulk -Apply -CloseFixedIssues tag-cohort path. It's a strict, safe improvement over the already-approved Round 1.
Glob verified correct. 1[01].[0-9]+.[0-9]+ mirrors GitHub's own documented filter-pattern example (v[12].[0-9]+.[0-9]+). Empirically (and per all three models):
| Tag | Trigger glob | Result |
|---|---|---|
10.0.80, 11.2.3 |
matches | ✅ fires |
1.2.3, 7.0.0, 100.0.0, 12.0.0 |
no match | ⛔ excluded |
11.0.0-preview.5.x, 10.0.0-rc.1.x |
no match (whole-ref) | ⛔ excluded |
| Model | Verdict | Confidence |
|---|---|---|
| Claude Opus 4.8 | LGTM | high |
| GPT-5.5 | LGTM | high |
| Gemini 3.1 Pro | LGTM | high |
💡 One non-blocking suggestion (all three models)
The new note "bump this glob (and the bash guard below) when a new major ships" (L16) is slightly inaccurate: the defense-in-depth bash guard ^[0-9]+\.[0-9]+\.[0-9]+$ (L91) is major-agnostic and never needs per-major bumping — only the glob does. It's functionally harmless (the glob is the scope gate; the guard is a shape backstop that only ever sees glob-passed 10.x/11.x tags), but the comment could mislead a future maintainer into pinning the guard and accidentally omitting a new major. Fix either way: drop the (and the bash guard below) parenthetical, or pin the guard to ^1[01]\.[0-9]+\.[0-9]+$ to make code match the documented intent.
ℹ️ FYI (by design)
Pinning to 10/11 means a final tail-servicing 9.0.x SR tag wouldn't auto-trigger — but that matches the explicit "currently-supported major" intent, only major 10 is actively tagged today, and workflow_dispatch (with the tag input) remains the manual fallback for any major. Not a regression.
Reviewed at head da0aee65. CI: license/cla ✅ pass; maui-pr skipping (by-design path-exclusion for .github/**-only PRs) — same posture as approved R1.
The 2026-01-01 'merged before' safety cutoff was hardcoded, so the bulk -Apply / -CloseFixedIssues path could never reach back and process PRs that predate the automation. Extract it into a pure, unit-testable Resolve-MergedAfterCutoff function and expose it via a new -MergedAfter parameter (and a matching merged_after workflow_dispatch input), so an older release can be processed deliberately — e.g. to close linked issues for a historical SR. Defaults to 2026-01-01 when unset. Adds 18 Pester tests covering default/whitespace resolution, date-only and ISO-8601 (UTC + offset) parsing, invalid-input errors, and Get-PrInfo honoring the configured cutoff (skip-before, include-after, boundary, lowered/raised cutoff, and unmerged PRs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| push: | ||
| tags: | ||
| - '1[01].[0-9]+.[0-9]+' # stable SR / GA: 10.0.80, 11.0.0 | ||
| - '1[01].[0-9]+.[0-9]+-preview.*' # preview: 11.0.0-preview.5.26304.4 | ||
| - '1[01].[0-9]+.[0-9]+-rc.*' # rc: 10.0.0-rc.1.25424.2 |
| Safety: PRs merged before a cutoff date are always skipped. The cutoff | ||
| defaults to 2026-01-01 (when this automation went live) and is configurable | ||
| via -MergedAfter, so an older release can be processed deliberately. |
kubaflo
left a comment
There was a problem hiding this comment.
🤖 AI-generated review — multi-model panel (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro), independent reviews + cross-pollination. Round 3.
✅ LGTM — unanimous (high confidence)
R3 reworks the trigger to include preview/rc tags (reversing R2's stable-only scope), adds a configurable -MergedAfter cutoff, and — nicely — rewrites the bash-guard comment to explain why the guard is major-agnostic (directly resolving the R2 thread). It also ships 296 new/updated tests, which I ran.
Empirical verification (I ran both suites at the PR head)
| Suite | Result |
|---|---|
MilestoneTrigger.Tests.ps1 |
100 / 100 pass |
Fix-MilestoneDrift.Tests.ps1 |
196 / 196 pass |
MilestoneTrigger.Tests.ps1 is genuinely strong: it translates the on.push.tags globs to regex via a self-unit-tested translator, drives the bash guard through real bash against shell-injection payloads (10.0.0; rm -rf /, $(whoami), backticks, pipes), and asserts the injection-safety invariant that run: never interpolates ${{ github.* }}. All three globs resolve correctly (stable/preview/rc on major 10/11; 9.x, 12.x, malformed, and other majors excluded).
Resolve-MergedAfterCutoff is correct: UTC default 2026-01-01, AssumeUniversal | AdjustToUniversal parsing, clear throw on bad input. The bulk -Apply -CloseFixedIssues blast radius stays bounded (major-pin + cutoff + maintainer-pushed tags + idempotent close).
| Model | Verdict | Confidence |
|---|---|---|
| Claude Opus 4.8 | LGTM | high |
| GPT-5.5 | LGTM | medium |
| Gemini 3.1 Pro | LGTM | high |
💡 Non-blocking suggestions
- NOTE vs guard wording (all 3 models). The top NOTE (L17) still says "bump these globs (and the bash guard below) when a new major ships", but the guard is now documented + unit-tested as major-agnostic — a new major requires bumping only the globs. Suggest: "bump these globs when a new major ships (the bash guard below is intentionally major-agnostic and does not need changing)."
- CI doesn't run these 296 tests (Opus). The whole trigger/guard/cutoff safety net rests on
.github/scripts/*.Tests.ps1, which no CI job invokes. A lightweightpwsh + Invoke-Pesterjob gated on.github/scripts/**would guard against future regressions automatically. - No
concurrency:group (Opus). With preview/rc now firing too, overlapping runs can process the same cohort; operations are idempotent so the worst case is a benign duplicate close-comment via a TOCTOU window — aconcurrency:group keyed on the ref would remove even that.
Reviewed at head 5ccd2650. CI: license/cla ✅ pass; maui-pr skipping (by-design .github/** path-exclusion). The 296 Pester tests are not CI-gated, so the local run above is the verification of record.
The configurable cutoff is meant for deliberate, local runs against a historical release (e.g. closing linked issues for an old SR), not the unattended workflow path. Revert the workflow_dispatch input / env / arg wiring; the -MergedAfter parameter remains on Fix-MilestoneDrift.ps1 for someone to run locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A reviewer flagged that the tag globs accepted any minor (e.g. 10.1.0, 11.2.3), which would fire the bulk -Apply/-CloseFixedIssues path. The script itself only recognises MAJOR.0.PATCH tags — Test-IsReleaseTag is ^MAJOR\.0\. and Get-PatchVersion requires ^(\d+)\.0\.(\d+)$ — so a non-.0 minor tag could never resolve a milestone (Invoke-AnalyzeRelease would throw). All modern .NET MAUI release tags are MAJOR.0.PATCH, so pinning the glob minor to 0 only narrows the trigger to what actually ships and what the script can process; no real tag stops matching. Tightens the three on.push.tags globs to 1[01].0.[0-9]+ (+ -preview/-rc). The bash guard stays an intentionally major-/minor-agnostic shape backstop (it only ever sees push refs the glob already admitted); reword the comment to stop implying the guard needs per-major bumping (a prior 3/3 review suggestion). Adds 8 negative test fixtures asserting non-.0 minors (10.1.0, 11.2.3, 10.2.40, 11.1.0-preview.*, 10.3.0-rc.*, ...) do NOT trigger, and updates the glob-set assertions/translator examples. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| '[' { | ||
| # Copy the bracket expression verbatim (glob ranges == regex ranges). | ||
| $j = $i + 1 | ||
| if ($j -lt $n -and $Glob[$j] -eq '!') { $j++ } # negation (unused by our globs) | ||
| while ($j -lt $n -and $Glob[$j] -ne ']') { $j++ } |
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial multi-model review
Reviewed the full current diff with 3 independent reviewers under an adversarial-consensus protocol (each finding cross-checked against the actual code/tags, not just model opinion), across two rounds.
Round 1 — one substantive finding, now fixed
❌ → ✅ Config Impact / Data Loss (raised by 1/3, accepted after ground-truth verification). The on.push.tags globs pinned the major (10/11) but left the minor unconstrained (1[01].[0-9]+.[0-9]+), so a stray tag like 10.1.0 / 11.2.3 would have fired the bulk -Apply -CloseFixedIssues path.
I verified this against ground truth before acting (one model's opinion isn't enough): every real modern MAUI tag is MAJOR.0.PATCH, and the script itself only recognizes that shape — Test-IsReleaseTag is ^MAJOR\.0\. and Get-PatchVersion requires ^(\d+)\.0\.(\d+)$. So a non-.0 minor could never resolve a milestone anyway. Pinning the glob minor to 0 is therefore a strict narrowing to exactly what the script can process and what actually ships — no real tag stops matching.
Fixed in 960e54b: globs are now 1[01].0.[0-9]+, 1[01].0.[0-9]+-preview.*, 1[01].0.[0-9]+-rc.*; the explanatory comment was rewritten to document the MINOR pin and the script's ^MAJOR\.0\. contract; 8 negative fixtures (non-.0 minor → must-not-trigger) plus updated glob-set assertions were added.
Documentation (3/3). The PR description was stale (claimed preview was "deliberately excluded" and "no changes to the script"). Updated to reflect preview/RC inclusion, the MAJOR+MINOR pin, and the new local-only -MergedAfter cutoff.
Round 2 — fix validated, no regressions
Re-ran all 3 reviewers on the fixed code:
- The pinned globs still match every real release shape (
10.0.80,11.0.0,10.0.0-rc.1.25424.2,11.0.0-preview.5.26304.4) and now correctly exclude non-.0minors — confirmed by hand against GitHub Actions filter-pattern semantics and by enumerating the repo's actual tags. No real tag was dropped. - The glob/guard divergence (glob is
MAJOR.0-pinned; the bash guard is intentionally shape-only) is safe and unreachable as a hole: the guard runs only in thepushbranch and only ever sees refs the glob already admitted. The one true divergence — a malformed prerelease tail that the glob's*admits but the guard's\.[0-9]+rejects — produces a fail-safeexit 1red run, never a silent skip or pass-through to-Apply. - One reviewer ran both suites locally: 304/304 green. Tests source the globs/guard from the live YAML (so they can't silently drift), and the cutoff tests are non-vacuous and properly isolated (
$script:MergedAfterCutoffrestored inAfterAll).
💡 Non-blocking notes (already addressed or by-design)
- Injection surface is clean:
github.ref_namereaches the script only via thePUSH_TAGenv var, consumed as a discrete bash-array element — never${{ }}-interpolated intorun:(test-enforced). - Auto
-Apply -CloseFixedIssueson tag push is the author's deliberate, live-validated posture (validated against10.0.80); blast-radius hardening was kept non-blocking.
Verdict: no outstanding issues. The one substantive finding is fixed and re-validated; the rest is correct, well-tested, and safe by design. Sharing as a comment — approval is a human call.
Convert-GhTagGlobToRegex detected a glob negation marker ('!' after '[')
but then copied the bracket expression verbatim, so a '[!0]' glob would
mistranslate to a .NET regex matching '!' or '0' (regex negation is '[^...]',
not '[!...]'). None of the workflow globs use negation, so rather than
silently emit a wrong regex, throw a clear error if one is ever introduced.
Also reject regex-style '[^...]' for the same reason. Adds two tests
asserting the translator throws on both negation spellings.
Addresses a Copilot PR-reviewer inline finding on MilestoneTrigger.Tests.ps1.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Completes the fail-fast hardening of Convert-GhTagGlobToRegex: a glob with an unclosed bracket (e.g. '1[01') previously ran the scan index past the end of the string and threw a cryptic Substring index-out-of-range error. Throw a clear "unclosed bracket" message instead, mirroring the negated-bracket guard. Adds two tests covering genuinely-unclosed globs. Addresses a reviewer-flagged pre-existing edge case adjacent to the prior fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| # Capture the last `run: |` block (the milestone step) and assert it only | ||
| # consumes env vars, never raw ${{ github.* }} expansion in the shell. | ||
| $idx = ($script:WorkflowLines | Select-String -SimpleMatch 'run: |' | Select-Object -Last 1).LineNumber | ||
| $idx | Should -Not -BeNullOrEmpty | ||
| $runBody = ($script:WorkflowLines[$idx..($script:WorkflowLines.Count - 1)]) -join "`n" | ||
| $runBody | Should -Not -Match '\$\{\{' | ||
| } | ||
|
|
PureWeen
left a comment
There was a problem hiding this comment.
Follow-up — reviewer suggestions addressed
Worked through the outstanding inline findings and the multi-model panel's non-blocking suggestions, then re-ran the adversarial review on the result.
Fixed
- Negated-bracket mistranslation in the trigger-test translator (
Convert-GhTagGlobToRegex). It detected glob negation (!after[) but copied the bracket verbatim, so a[!0]glob would have produced a .NET regex matching!/0(regex negation is[^...], not[!...]). Now throws a clear error on[!/[^since no workflow glob uses negation, with tests on both spellings. (b153b012) - Unclosed-bracket crash in the same helper — a glob like
1[01ran the scan index off the end and threw a crypticSubstringindex error. Now throws a clear "unclosed bracket" message, mirroring the negation guard, with tests. (2a082563)
Both are test-only hardening — the production workflow and script are byte-unchanged by these commits, and all real globs translate identically. Full suite is 308/308 green.
Already addressed earlier in the branch
- PR description staleness (preview/RC inclusion,
-MergedAfter) → description updated. - "bump the bash guard when a new major ships" comment → reworded; the guard is intentionally shape-only/major-agnostic (documented + tested).
Evaluated and intentionally not changed
- The
'1[01].[0-9]+'-style "+is a literal, stable tags won't trigger" concern is a false positive — GitHub Actions filter patterns treat+as one-or-more (test-proven against real tags). concurrency:group — declined. A global group would let GitHub cancel superseded pending runs, silently skipping per-PR milestone corrections under a merge burst (a real regression); a per-ref group wouldn't prevent the cross-ref race it targets. The script is idempotent (already-closed = no-op, warn-not-fail), so the races are benign. Happy to revisit.- CI runner for these Pester suites — a real pre-existing gap, but a separate scope decision; not bundling it here.
Re-reviewed by 3 independent reviewers on the final HEAD (2a082563): no findings, no regressions. Sharing as a comment — approval is a human call.
…ger tests
Round 5 adversarial review fixes:
- Fix-MilestoneDrift.ps1: Get-PrInfo now returns a { SkippedPreCutoff }
sentinel for PRs merged before -MergedAfter instead of $null, so the
callers can distinguish a benign cutoff-skip from a real fetch failure.
Tag-mode and single-PR callers handle the sentinel; a new
PrsSkippedPreCutoff counter is reported. This prevents an all-pre-cutoff
tag cohort from inflating $report.Errors and tripping the top-level
'analysis failed' throw on what is actually a no-op (R1-B).
- MilestoneTrigger.Tests.ps1: add a '?' quantifier fidelity fixture;
add a defense-in-depth block asserting the bash guard rejects malformed
prerelease tags ('-preview.', '-preview.CAPS', '-rc.x') that the
permissive glob admits; anchor the injection-safety test to the named
'Run milestone management' step instead of a positional last-run:
heuristic; document the literal-']'-at-position-0 bracket limitation.
All 314 Pester tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial review — round 5
Ran the full adversarial pass again: 3 independent holistic reviewers over the current diff + PR body, followed by an adversarial-consensus dispute round on the two most severe findings. Then I verified every surviving claim against the source before deciding what to change. No ❌/
Findings raised → outcome
❌ candidates (both discarded by unanimous dispute):
- Checkout posture — "tag push checks out an unreviewed commit and runs with a write token." Two dispute reviewers independently disagreed:
pushevents only fire in the base repo (forks can't push tags intodotnet/maui), so the tag-pusher is necessarily a trusted write-collaborator who already holdsissues:write/pull-requests:write. The job token iscontents:read(can't push code), and the milestone/issue edits are reversible. Not a privilege escalation → hardening note only, no code change. - Wrong-cohort blast radius — "tag target isn't validated." The script already fails closed:
Test-IsReleaseTag/Get-PatchVersionthrow on anything that isn'tMAJOR.0.PATCH(-preview|-rc).N, the bash guard re-validates shape, and-MergedAfterbounds the cohort. Hardening note only.
💡 fixes applied (this commit):
- Pre-cutoff skips vs. real errors —
Get-PrInforeturned$nullfor both a real fetch failure and a benign pre--MergedAfterskip, so an all-pre-cutoff tag cohort inflated$report.Errorsand tripped the top-level "analysis failed" throw on what is actually a no-op. It now returns a{ SkippedPreCutoff }sentinel; callers count those separately (PrsSkippedPreCutoff) and genuine fetch failures still throw. Added 2 tests covering the all-skip and mixed skip+failure cohorts. - Trigger-test hardening — added a
?-quantifier fidelity fixture; added a defense-in-depth block asserting the bash guard rejects malformed prerelease tags the permissive glob admits (-preview.,-preview.CAPS,-rc.x); anchored the injection-safety test to the namedRun milestone managementstep instead of a positional "lastrun:" heuristic; documented the literal-]-at-position-0 bracket limitation.
💡 declined (with rationale):
- Trimming the default-case escape string — the extra escaped chars are harmless/defensive; trimming is churn + risk for no behavior change.
- "Fixing"
[datetime]::Parseto reject time-only-MergedAfterinput — it's a manual-only knob that already fails conservative; tightening risks rejecting valid inputs likeJan 2026.
Validation
All 314 Pester tests pass (MilestoneTrigger.Tests.ps1 + Fix-MilestoneDrift.Tests.ps1).
kubaflo
left a comment
There was a problem hiding this comment.
🤖 AI-generated review — multi-model panel (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro), independent reviews + cross-pollination. Round 4.
✅ LGTM — unanimous (high confidence)
R4 is a clean set of refinements over the approved R3, and it resolves my R3 inline suggestion (the NOTE now correctly states the guard is major-/minor-agnostic). 👍
What changed & why it's sound:
- Minor pinned to
0(1[01].[0-9]+.[0-9]+→1[01].0.[0-9]+). This aligns the trigger with the script's ownTest-IsReleaseTag(^MAJOR\.0\., L123), so a non-.0minor could never resolve a milestone anyway. Opus verified against the full tag history — only ancient Xamarin2.4/3.6ever used non-zero minors (far outside the 10/11 glob), so nothing that would work is dropped. merged_afterdispatch input dropped (the-MergedAfterscript param is retained) — a sensible foot-gun reduction on a bulk-mutation workflow.- Pre-cutoff skip sentinel —
Get-PrInfonow returns@{SkippedPreCutoff=$true}instead of$null, so an all-pre-cutoff cohort exits cleanly instead of reporting "0 checked, N errors". Both (and only) callers checkContainsKey('SkippedPreCutoff')before the-not $prbranch — the truthiness trap (a non-empty hashtable is truthy) is correctly avoided. - Translator hardening — fail-fast on negated/unclosed bracket globs.
Empirical verification (ran at head 161eb785)
| Suite | Result |
|---|---|
MilestoneTrigger.Tests.ps1 |
116 / 116 pass (+16) |
Fix-MilestoneDrift.Tests.ps1 |
198 / 198 pass (+2) |
| Model | Verdict | Confidence |
|---|---|---|
| Claude Opus 4.8 | LGTM | high |
| GPT-5.5 | LGTM | high |
| Gemini 3.1 Pro | LGTM | high |
💡 Non-blocking notes (all informational / pre-existing)
- The push trigger silently won't fire if release tags are ever pushed via the default
GITHUB_TOKEN(GitHub platform constraint) — already documented in the workflow comment. - The glob tests simulate GitHub's filter engine (can't run the real matcher offline); Opus independently cross-checked the semantics against the docs + real tag list — they agree on every case.
- No
concurrency:guard on overlapping tag-push runs — pre-existing, and operations are idempotent, so benign.
Reviewed at head 161eb785. CI: license/cla ✅ pass; maui-pr skipping (by-design .github/** path-exclusion). 314/314 Pester is the verification of record (not CI-gated).
…yet (#36398) > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) 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 the `pull_request_target` path (`Fix-MilestoneDrift.ps1 -PrNumber <N> -Apply [-CloseFixedIssues]`). Right now **every merge to a `net11.0` branch crashes the workflow** because the expected milestone `.NET 11.0-preview7` does not exist in GitHub yet (only `.NET 11.0-preview6` exists). Several recent runs failed with the identical error: ``` Expected milestone: .NET 11.0-preview7 WARNING: No GitHub milestone found matching ".NET 11.0-preview7". The milestone may not have been created yet. Skipping. Fix-MilestoneDrift.ps1: The property 'ResolvedMilestone' cannot be found on this object. Verify that the property exists. ##[error]Process completed with exit code 1. ``` 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 logs `No 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` / `ResolvedMsNumber` keys** that the success-path report carries. The caller then runs: - `Write-Report $report`, which reads `$Report.ResolvedMilestone` **unguarded** (no `ContainsKey` check), and - `Save-ReportJson`, which reads `$Report.ResolvedMilestone` **unguarded** too. During normal execution the script enables `Set-StrictMode -Version Latest`, under which reading a missing hashtable key throws `PropertyNotFound` → 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 in `Invoke-AnalyzeSinglePr` so it matches the success-path report shape: ```powershell ResolvedMilestone = $null ResolvedMsNumber = $null ``` This satisfies every unguarded downstream accessor. `Invoke-ApplyCorrections` iterates the empty `Corrections` list (no-op) and `New-GitHubIssue` only runs when `Corrections.Count > 0`, so no other changes are needed. ## Regression test Added tests to `.github/scripts/Fix-MilestoneDrift.Tests.ps1` covering 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 calls `Write-Report` on 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**: 1. Drive `Invoke-AnalyzeSinglePr` down the milestone-not-found path (mock `Find-MatchingMilestone` → `$null`) and assert the returned report `.ContainsKey('ResolvedMilestone')` and `.ContainsKey('ResolvedMsNumber')` are `$true` (and reading `.ResolvedMilestone` yields `$null`). 2. A faithful CI reproduction: inside the test, `Set-StrictMode -Version Latest`, then call `Write-Report` and `Save-ReportJson` on that report and assert it does **not** throw — reproducing the exact crash condition. **Before/after proof** (reverting only the 2-line fix): - **Fix reverted:** both new tests fail — test #2 throws `The property 'ResolvedMilestone' cannot be found on this object` from `Fix-MilestoneDrift.ps1:1326` (`Write-Report`), exactly matching the CI crash. - **Fix applied:** full suite green — **316 passed, 0 failed, 0 skipped** (`MilestoneTrigger.Tests.ps1` + `Fix-MilestoneDrift.Tests.ps1`). ## Note Creating the `.NET 11.0-preview7` milestone 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. 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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### What this does Adds a `push` tag trigger to the **Milestone Management** workflow (`fix-milestone-drift.yml`) so that when a release tag (e.g. `10.0.80`) is pushed, the workflow automatically audits the entire tag cohort, fixes milestone drift, and closes issues fixed by PRs that shipped in that tag. Previously this was a manual `workflow_dispatch` exercise — someone had to remember to run it against each tag. Now it runs as part of cutting a release. ### Changes **Workflow (`fix-milestone-drift.yml`)** - **New `on.push.tags` trigger**, scoped to the `MAJOR.0.PATCH` shape that .NET MAUI actually ships, with three globs so each ref shape gets its own line (GitHub Actions filter patterns match the whole ref): - `1[01].0.[0-9]+` — stable SR / GA (e.g. `10.0.80`, `11.0.0`) - `1[01].0.[0-9]+-preview.*` — preview (e.g. `11.0.0-preview.5.26304.4`) - `1[01].0.[0-9]+-rc.*` — rc (e.g. `10.0.0-rc.1.25424.2`) - **Preview and RC tags are included** (deliberately). MAJOR is pinned to a currently-supported major (10/11) and MINOR is pinned to `0` to bound the blast radius — a stray tag like `1.2.3`, `7.0.0`, `9.0.100-preview.1.9973` or `10.1.0` must **not** fire the bulk `-Apply` path. The minor pin mirrors the script's own contract (`Test-IsReleaseTag` is `^MAJOR\.0\.`), so a non-`.0` minor could never resolve a milestone anyway. - **Job `if`** now also runs for `push` events. - **New `push` branch** in the run step invokes tag-mode with `-Apply -CloseFixedIssues`, plus a defense-in-depth bash guard (`^[0-9]+\.[0-9]+\.[0-9]+(-(preview|rc)\.[0-9]+(\.[0-9]+)*)?$`) that hard-fails on any ref that isn't a release-tag shape (anti-injection backstop; intentionally major-/minor-agnostic since the glob owns scope). `github.ref_name` reaches the script only via the `PUSH_TAG` env var and is passed as a discrete bash-array element — never `${{ }}`-interpolated into the run script. - The existing **PR-merge** (`pull_request_target`) and **manual `workflow_dispatch`** paths are unchanged. **Script (`Fix-MilestoneDrift.ps1`)** - **New `-MergedAfter` parameter** makes the previously-hardcoded "merged-before" safety cutoff configurable (extracted into a pure, unit-tested `Resolve-MergedAfterCutoff`). PRs merged strictly before the cutoff are skipped; the default stays **2026-01-01** (when this automation went live) so the bulk path can't reach back and rewrite milestones for PRs that predate it. Override it to deliberately process an older release locally — e.g. `-MergedAfter '2024-01-01' -Apply -CloseFixedIssues` to close linked issues for a historical SR. This is a local/manual knob only; it is **not** exposed through the workflow. **Tests** - `MilestoneTrigger.Tests.ps1` (new) — ~100 Pester tests covering the tag-trigger: a self-tested GH-glob→regex translator driving a large match/no-match fixture (stable/preview/rc that should fire; out-of-range major, non-`.0` minor, and malformed shapes that should not), plus the bash guard regex extracted verbatim from the YAML and run through real `bash` against valid tags and injection payloads, plus structure/injection-safety invariants. - `Fix-MilestoneDrift.Tests.ps1` — +18 tests for `Resolve-MergedAfterCutoff` (default/whitespace → 2026-01-01 UTC, date-only and ISO-8601 parsing, invalid-input errors) and `Get-PrInfo` cutoff enforcement (skip-before, include-after, exact boundary, lowered/raised cutoff, unmerged PRs). ### Behavioral note Tag-push events run the workflow file **as it exists at the tagged commit**. So this trigger only takes effect for tags cut from commits that already contain this change — i.e. after it merges to `main` and flows into the `release/*` branches that SR tags are cut from. It will not retroactively run on existing tags, and manual `workflow_dispatch` remains available for any tag in the meantime. A tag pushed using the default `GITHUB_TOKEN` does **not** trigger this workflow (GitHub suppresses runs for events raised by the default token to avoid loops). Release tags must be pushed by a human or by automation using a PAT / GitHub App token. Today SR tags are pushed by a maintainer, so it fires — documented inline so a future change to tag-creation automation can't silently regress it. ### Validation - YAML parses; run-step bash syntax verified - Run-step branch logic verified for all cases: push of a release tag → `-Tag <tag> -Apply -CloseFixedIssues`; non-release / non-`.0`-minor / wrong-major refs → rejected; PR-to-`main` and PR-to-`net*.0` paths → unchanged - The underlying tag-mode `-Apply -CloseFixedIssues` invocation was validated live against `10.0.80` (2 milestone corrections + 2 issue closures, matching live GitHub state) - Full Pester suite green (314 tests across both files) --------- 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!
What this does
Adds a
pushtag trigger to the Milestone Management workflow (fix-milestone-drift.yml) so that when a release tag (e.g.10.0.80) is pushed, the workflow automatically audits the entire tag cohort, fixes milestone drift, and closes issues fixed by PRs that shipped in that tag.Previously this was a manual
workflow_dispatchexercise — someone had to remember to run it against each tag. Now it runs as part of cutting a release.Changes
Workflow (
fix-milestone-drift.yml)on.push.tagstrigger, scoped to theMAJOR.0.PATCHshape that .NET MAUI actually ships, with three globs so each ref shape gets its own line (GitHub Actions filter patterns match the whole ref):1[01].0.[0-9]+— stable SR / GA (e.g.10.0.80,11.0.0)1[01].0.[0-9]+-preview.*— preview (e.g.11.0.0-preview.5.26304.4)1[01].0.[0-9]+-rc.*— rc (e.g.10.0.0-rc.1.25424.2)0to bound the blast radius — a stray tag like1.2.3,7.0.0,9.0.100-preview.1.9973or10.1.0must not fire the bulk-Applypath. The minor pin mirrors the script's own contract (Test-IsReleaseTagis^MAJOR\.0\.), so a non-.0minor could never resolve a milestone anyway.ifnow also runs forpushevents.pushbranch in the run step invokes tag-mode with-Apply -CloseFixedIssues, plus a defense-in-depth bash guard (^[0-9]+\.[0-9]+\.[0-9]+(-(preview|rc)\.[0-9]+(\.[0-9]+)*)?$) that hard-fails on any ref that isn't a release-tag shape (anti-injection backstop; intentionally major-/minor-agnostic since the glob owns scope).github.ref_namereaches the script only via thePUSH_TAGenv var and is passed as a discrete bash-array element — never${{ }}-interpolated into the run script.pull_request_target) and manualworkflow_dispatchpaths are unchanged.Script (
Fix-MilestoneDrift.ps1)-MergedAfterparameter makes the previously-hardcoded "merged-before" safety cutoff configurable (extracted into a pure, unit-testedResolve-MergedAfterCutoff). PRs merged strictly before the cutoff are skipped; the default stays 2026-01-01 (when this automation went live) so the bulk path can't reach back and rewrite milestones for PRs that predate it. Override it to deliberately process an older release locally — e.g.-MergedAfter '2024-01-01' -Apply -CloseFixedIssuesto close linked issues for a historical SR. This is a local/manual knob only; it is not exposed through the workflow.Tests
MilestoneTrigger.Tests.ps1(new) — ~100 Pester tests covering the tag-trigger: a self-tested GH-glob→regex translator driving a large match/no-match fixture (stable/preview/rc that should fire; out-of-range major, non-.0minor, and malformed shapes that should not), plus the bash guard regex extracted verbatim from the YAML and run through realbashagainst valid tags and injection payloads, plus structure/injection-safety invariants.Fix-MilestoneDrift.Tests.ps1— +18 tests forResolve-MergedAfterCutoff(default/whitespace → 2026-01-01 UTC, date-only and ISO-8601 parsing, invalid-input errors) andGet-PrInfocutoff enforcement (skip-before, include-after, exact boundary, lowered/raised cutoff, unmerged PRs).Behavioral note
Tag-push events run the workflow file as it exists at the tagged commit. So this trigger only takes effect for tags cut from commits that already contain this change — i.e. after it merges to
mainand flows into therelease/*branches that SR tags are cut from. It will not retroactively run on existing tags, and manualworkflow_dispatchremains available for any tag in the meantime.A tag pushed using the default
GITHUB_TOKENdoes not trigger this workflow (GitHub suppresses runs for events raised by the default token to avoid loops). Release tags must be pushed by a human or by automation using a PAT / GitHub App token. Today SR tags are pushed by a maintainer, so it fires — documented inline so a future change to tag-creation automation can't silently regress it.Validation
-Tag <tag> -Apply -CloseFixedIssues; non-release / non-.0-minor / wrong-major refs → rejected; PR-to-mainand PR-to-net*.0paths → unchanged-Apply -CloseFixedIssuesinvocation was validated live against10.0.80(2 milestone corrections + 2 issue closures, matching live GitHub state)